Mobile Gmail should use 1.3 MB less memory

Source/bindings/v8/V8StringResource.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 34 matching lines @@
 
 void WebCoreStringResourceBase::visitStrings(ExternalStringVisitor* visitor)
 {
     visitor->visitJSExternalString(m_plainString.impl());
     if (m_plainString.impl() != m_atomicString.impl() && !m_atomicString.isNull())
         visitor->visitJSExternalString(m_atomicString.impl());
 }
 
 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);
 };
 
 template<>
 struct StringTraits<String> {
     static const String& fromStringResource(WebCoreStringResourceBase* resource)
     {
         return resource->webcoreString();
     }
-    static bool is16BitAtomicString(String& string)
-    {
-        return false;
-    }
     template<bool oneByte>
     static String fromV8String(v8::Handle<v8::String>, int);
 };
 
 template<>
 struct StringTraits<AtomicString> {
     static const AtomicString& fromStringResource(WebCoreStringResourceBase* resource)
     {
         return resource->atomicString();
     }
-    static bool is16BitAtomicString(AtomicString& string)
-    {
-        return !string.string().is8Bit();
-    }
     template<bool oneByte>
     static AtomicString fromV8String(v8::Handle<v8::String>, int);
 };
 
 template<>
 String StringTraits<String>::fromV8String<false>(v8::Handle<v8::String> v8String, int length)
 {
     ASSERT(v8String->Length() == length);
     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;
 }
 
 template<>
 AtomicString StringTraits<AtomicString>::fromV8String<false>(v8::Handle<v8::String> v8String, int length)
 {
     ASSERT(v8String->Length() == length);
     static const int inlineBufferSize = 16;
     if (length <= inlineBufferSize) {
         UChar inlineBuffer[inlineBufferSize];

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

are small strings not targeted here for a reason?

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

Yes.  The AtomicString(inlineBuffer, length) constructor already checks whether
the buffer is 8 bit.

         v8String->Write(reinterpret_cast<uint16_t*>(inlineBuffer), 0, length);
         return AtomicString(inlineBuffer, length);
     }
     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);
 }
 
 template<>
 String StringTraits<String>::fromV8String<true>(v8::Handle<v8::String> v8String, int length)
 {
     ASSERT(v8String->Length() == length);
     LChar* buffer;
     String result = String::createUninitialized(length, buffer);
     v8String->WriteOneByte(buffer, 0, length);
@@ skipping 29 matching lines @@
             if (encoding == v8::String::ONE_BYTE_ENCODING)
                 base = static_cast<WebCoreStringResource8*>(resource);
             else
                 base = static_cast<WebCoreStringResource16*>(resource);
             return StringTraits<StringType>::fromStringResource(base);
         }
     }
 
     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));
 
     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;
     } else {
         WebCoreStringResource16* stringResource = new WebCoreStringResource16(result);
         if (UNLIKELY(!v8String->MakeExternal(stringResource)))
             delete stringResource;
     }
     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);
 template AtomicString v8StringToWebCoreString<AtomicString>(v8::Handle<v8::String>, ExternalMode);
 
 // Fast but non thread-safe version.
 String int32ToWebCoreStringFast(int value)
 {
     // Caching of small strings below is not thread safe: newly constructed AtomicString
     // are not safely published.
@@ skipping 17 matching lines @@
 
 String int32ToWebCoreString(int value)
 {
     // If we are on the main thread (this should always true for non-workers), call the faster one.
     if (isMainThread())
         return int32ToWebCoreStringFast(value);
     return String::number(value);
 }
 
 } // namespace WebCore