Consider merging https://chromium.googlesource.com/chromium/blink/+/49c9632ac135f6f06e623a7a81d9da1f6bb7196f Mobile Gmail should use 100 kB less memory Previously, string results from WebSQL were always 16 bit strings even if they could fit into 8 bit strings. This CL tries to create 8 bit strings from sqlite results when possible. This CL halves the total number of 16 bit strings retained by Mobile Gmail. It doesn't halve the total memory used by 16 bit strings, however, because there appears to be another source of 16 bit strings that tend to be longer on average. Even so, this CL appears to be worth about 100 kB of memory.
Also https://chromium.googlesource.com/chromium/blink/+/ce35a544d09e6cb907457535340eb0e9984e57b8
Created attachment 203305 [details] Patch
Comment on attachment 203305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203305&action=review > Source/WebCore/ChangeLog:8 > + Merge chromium ce35a544d09e6cb907457535340eb0e9984e57b8. Maybe include the full URL so that we can just copy & paste it?
Comment on attachment 203305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203305&action=review I’d love to see an optimized path for the common case where no characters need escaping. It also seems a little strange to put the 8/16 bit case inside the loop. > Source/WebCore/platform/FileSystem.cpp:84 > + const StringImpl* stringImpl = inputString.impl(); No reason for const StringImpl, since StringImpl is an immutable class. You should take out the const.
(In reply to comment #4) > I’d love to see an optimized path for the common case where no characters need escaping. It also seems a little strange to put the 8/16 bit case inside the loop. Is this code really hot enough to justify that?
Created attachment 203307 [details] Patch
Comment on attachment 203307 [details] Patch Attachment 203307 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/665562
(In reply to comment #5) > (In reply to comment #4) > > I’d love to see an optimized path for the common case where no characters need escaping. It also seems a little strange to put the 8/16 bit case inside the loop. > > Is this code really hot enough to justify that? Probably not.
Comment on attachment 203307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203307&action=review r=me, but it looks like this broke the Windows build > Source/WTF/wtf/text/StringImpl.cpp:296 > + unsigned length = lengthOfNullTerminatedString(string); > + return StringImpl::create8BitIfPossible(string, length); I think this would be better without the local variable “length”. > Source/WTF/wtf/text/WTFString.cpp:60 > + size_t length = lengthOfNullTerminatedString(str); > + m_impl = StringImpl::create(str, length); I think this would be better without the local variable “length”. > Source/WebCore/platform/sql/SQLiteStatement.cpp:335 > + case SQLITE_TEXT: { > + const UChar* string = reinterpret_cast<const UChar*>(sqlite3_value_text16(value)); > + return SQLValue(StringImpl::create8BitIfPossible(string)); > + } Why the local variable. Just to keep the source lines shorter?
Committed r150985: <http://trac.webkit.org/changeset/150985>
Created attachment 203385 [details] Patch
(In reply to comment #9) > > Source/WebCore/platform/sql/SQLiteStatement.cpp:335 > > + case SQLITE_TEXT: { > > + const UChar* string = reinterpret_cast<const UChar*>(sqlite3_value_text16(value)); > > + return SQLValue(StringImpl::create8BitIfPossible(string)); > > + } > > Why the local variable. Just to keep the source lines shorter? That's probably why the author did it. In this case I think it is better for WebKit as well. Many nested calls on a single line make things harder to debug. I fixed the other cases you mentioned.
Created attachment 203649 [details] Patch
Committed r151248: <http://trac.webkit.org/changeset/151248>