Mobile Gmail should use 1.3 MB less memory

Source/bindings/v8/V8StringResource.cpp

Index: Source/bindings/v8/V8StringResource.cpp
diff --git a/Source/bindings/v8/V8StringResource.cpp b/Source/bindings/v8/V8StringResource.cpp
index 47f4d9b3fb406fba748cb4287bd860f12c06327f..55b52c8b8f88b630e8e9f0cf4888cccb61edcdb5 100644
--- a/Source/bindings/v8/V8StringResource.cpp
+++ b/Source/bindings/v8/V8StringResource.cpp
@@ -52,7 +52,6 @@ void WebCoreStringResourceBase::visitStrings(ExternalStringVisitor* visitor)
 
 template<class StringClass> struct StringTraits {
     static const StringClass& fromStringResource(WebCoreStringResourceBase*);
-    static bool is16BitAtomicString(StringClass&);
     template<bool oneByte>
     static StringClass fromV8String(v8::Handle<v8::String>, int);
 };
@@ -63,10 +62,6 @@ struct StringTraits<String> {
     {
         return resource->webcoreString();
     }
-    static bool is16BitAtomicString(String& string)
-    {
-        return false;
-    }
     template<bool oneByte>
     static String fromV8String(v8::Handle<v8::String>, int);
 };
@@ -77,10 +72,6 @@ struct StringTraits<AtomicString> {
     {
         return resource->atomicString();
     }
-    static bool is16BitAtomicString(AtomicString& string)
-    {
-        return !string.string().is8Bit();
-    }
     template<bool oneByte>
     static AtomicString fromV8String(v8::Handle<v8::String>, int);
 };
@@ -92,6 +83,8 @@ String StringTraits<String>::fromV8String<false>(v8::Handle<v8::String> v8String
     UChar* buffer;
     String result = String::createUninitialized(length, buffer);
     v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
+    if (result.containsOnlyLatin1())

[eseidel, 2013/06/06 05:49:08]

Every time we hit this it seems this is an error from v8?  Should we have a
FIXME here?

[abarth-chromium, 2013/06/06 06:01:55]

I wouldn't say there is an error in V8.  It's fine to represent Latin-1 strings
using 16 bits per character.  It's just not as memory-efficient as using 8 bits
per character.

As an example, suppose you have a long string with many non-Latin-1 characters. 
If you take a substring, you'll probably end up with a 16 bit wide string both
in V8 and with WTFString.  With WTFString, you can even end up sharing the
underlying string buffer, so it's actually more memory-efficient to use a 16 bit
representation.

That said, in the common case, you'd hope that entirely Latin-1 strings would
only use 8 bits per character.  I agree that this CL suggests that we should do
some more investigation on the V8 side to understand why we're getting all these
16 bit strings here.  I'm secretly hoping dcarney is interested in doing that
investigation.  :)

[dcarney, 2013/06/06 06:45:20]

there are plenty on cases where the cost of checking 8 bitness is too high in v8
to do, but the root cause is often the concatenation of many small strings, one
of which fulfills this condition

+        return String::make8BitFrom16BitSource(result.characters16(), result.length());
     return result;
 }
 
@@ -108,6 +101,11 @@ AtomicString StringTraits<AtomicString>::fromV8String<false>(v8::Handle<v8::Stri
     UChar* buffer;
     String result = String::createUninitialized(length, buffer);
     v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
+    if (result.containsOnlyLatin1()) {

[dcarney, 2013/06/06 06:45:20]

maybe this check should be moved to the atomicstring table, so that all 8 bit
strings in the atomic string table are actually 8 bit and not 16? last time i
investigated, there were tons of such strings in the atomic string tables

+        AtomicString result(result.characters16(), result.length());
+        ASSERT(result.is8Bit());
+        return result;
+    }
     return AtomicString(result);
 }
 
@@ -157,7 +155,7 @@ StringType v8StringToWebCoreString(v8::Handle<v8::String> v8String, ExternalMode
 
     int length = v8String->Length();
     if (UNLIKELY(!length))
-        return String("");
+        return String(""); // FIXME: Should return emptyString();

[haraken, 2013/06/06 05:22:11]

Nit: Why is it hard to fix it now? I'm just curious.

[abarth-chromium, 2013/06/06 05:24:02]

I'd prefer to fix it in a second CL.  It should be fine to fix, but I didn't
want to add risk to this CL.

 
     bool oneByte = v8String->IsOneByte();
     StringType result(oneByte ? StringTraits<StringType>::template fromV8String<true>(v8String, length) : StringTraits<StringType>::template fromV8String<false>(v8String, length));
@@ -165,7 +163,7 @@ StringType v8StringToWebCoreString(v8::Handle<v8::String> v8String, ExternalMode
     if (external != Externalize || !v8String->CanMakeExternal())
         return result;
 
-    if (oneByte && !StringTraits<StringType>::is16BitAtomicString(result)) {
+    if (result.is8Bit()) {
         WebCoreStringResource8* stringResource = new WebCoreStringResource8(result);
         if (UNLIKELY(!v8String->MakeExternal(stringResource)))
             delete stringResource;
@@ -176,7 +174,7 @@ StringType v8StringToWebCoreString(v8::Handle<v8::String> v8String, ExternalMode
     }
     return result;
 }
-    
+
 // Explicitly instantiate the above template with the expected parameterizations,
 // to ensure the compiler generates the code; otherwise link errors can result in GCC 4.4.
 template String v8StringToWebCoreString<String>(v8::Handle<v8::String>, ExternalMode);