Bug 116935 – WebSQL forces 16-bit strings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also https://chromium.googlesource.com/chromium/blink/+/ce35a544d09e6cb907457535340eb0e9984e57b8

[Benjamin Poulain]

Created attachment 203305 [details]
Patch

[Ryosuke Niwa]

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?

[Darin Adler]

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.

[Benjamin Poulain]

(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?

[Benjamin Poulain]

Created attachment 203307 [details]
Patch

[Build Bot]

Comment on attachment 203307 [details]
Patch

Attachment 203307 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/665562

[Darin Adler]

(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.

[Darin Adler]

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?

[Benjamin Poulain]

Committed r150985: <http://trac.webkit.org/changeset/150985>

[Benjamin Poulain]

Created attachment 203385 [details]
Patch

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 203649 [details]
Patch

[Benjamin Poulain]

Committed r151248: <http://trac.webkit.org/changeset/151248>