Bug 102230 – [v8] Improve worker.postMessage() string performance: avoid utf8 conversion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[noel gordon]

SerializedScriptValue uses of v8::String::Utf8Value stringValue(value) is hot code transferring strings to/from workers.

[noel gordon]

Created attachment 174155 [details]
Patch

[Adam Barth]

Comment on attachment 174155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174155&action=review

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:334
> +        doWriteUint32(static_cast<uint32_t>(length * 2));

2 -> sizeof(UChar) ?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:335
> +        ensureSpace(length * 2);

Maybe we should put "length * 2" into a local variable since we use it a bunch?  Perhaps size?

[Joshua Bell]

I filed https://bugs.webkit.org/show_bug.cgi?id=102243 to track the issue this will cause with IndexedDB. We could land this first, but ideally 102243 would land at the same time or within a few days.

[Joshua Bell]

wireFormatVersion (line 244) needs to be incremented.

[Alec Flett]

Please wait for bug 96818 so you have a place to add tests and support backwards-compatible changes.

[noel gordon]

(In reply to comment #4)
> wireFormatVersion (line 244) needs to be incremented.

Considering bug 72198 and bug 76803, both of which added new serialization tag definitions, then why increment?  Seems we've never done so before.

[Joshua Bell]

(In reply to comment #6)
> (In reply to comment #4)
> > wireFormatVersion (line 244) needs to be incremented.
> 
> Considering bug 72198 and bug 76803, both of which added new serialization tag definitions, then why increment?  Seems we've never done so before.

The scenario we need to account for is:

* User installs build N+1
* User visits site using IndexedDB which writes SSVs to disk
* User "downgrades" to build N, without removing the "profile" (cookies, DBs, etc)
* User visits site; IndexedDB opens the backing store; some SSVs can't be parsed, surfaced as seemingly random data errors or inconsistencies

This scenario is unfortunately common enough that we've had to account for it by detecting the downgrade scenario. Our response in the IndexedDB code is rather severe (blow away the database) but the alternative is to block that origin's use of IDB, or surface what appears to web apps as data corruption.

The frequency of this scenario was not apparent to us until recently, or we would have implemented code such as https://bugs.webkit.org/show_bug.cgi?id=102243 earlier.

Suggestions for alternate approaches are welcome.

[noel gordon]

Thanks Josuha, I suppose IDB is like a really big cookie and users can blow their cookies (and IDB's) away any time. Applications have to deal with it: IDB storage is persistent, but it's not permanent.

[noel gordon]

(In reply to comment #2)
> (From update of attachment 174155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174155&action=review
> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:334
> > +        doWriteUint32(static_cast<uint32_t>(length * 2));
> 
> 2 -> sizeof(UChar) ?

Done.

> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:335
> > +        ensureSpace(length * 2);
> 
> Maybe we should put "length * 2" into a local variable since we use it a bunch?  Perhaps size?

Good suggestion, I used size.  I also had to account for the length being VarInt encoded when working out if a paddingTag should be added.

[noel gordon]

Created attachment 175667 [details]
Patch

[Alec Flett]

Comment on attachment 175667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175667&action=review

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:247
> +// Version 1: Version numbers manifested in space-time.

How about 
Version 0: Initial version, without explicit support for version numbers.
Version 1: support for object references (cycle detection)
Version 2:...

> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:149
> +var unicodeObject = {a: 'a', u: String.fromCharCode(0x03B1,0x03B2)};

you're going to need to add a backwards-compatible test here too - you'll have to run the test without your patch to generate the value first, then set the 3rd parameter to testSerialization to 'true'

[noel gordon]

(In reply to comment #11)
> (From update of attachment 175667 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175667&action=review
> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:247
> > +// Version 1: Version numbers manifested in space-time.
> 
> How about 
> Version 0: Initial version, without explicit support for version numbers.
> Version 1: support for object references (cycle detection)
> Version 2:...

How about (refer comment 6) I just write // Version 2: Added StringUCharTag for UChar v8 strings. since that's the only real fact I have.  "support for object references (cycle detection)" does not cover what happened before version 2.

[noel gordon]

Created attachment 176668 [details]
Patch

[noel gordon]

(In reply to comment #11)

> > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:149
> > +var unicodeObject = {a: 'a', u: String.fromCharCode(0x03B1,0x03B2)};
> 
> you're going to need to add a backwards-compatible test here too - you'll have to run the test without your patch to generate the value first, then set the 3rd parameter to testSerialization to 'true'

ok, I grabbed data for the test without my patch, and added that old data as the third argument of the test.  Didn't use a 'true', hope this is correct.

[noel gordon]

(In reply to comment #14)
Also fixed the existing test on http://trac.webkit.org/changeset/136085 so I could grab the data.

[noel gordon]

Created attachment 176875 [details]
Patch

[Alec Flett]

thanks for updating the tests - they lgtm now. (and yes, not sure why I thought the 3rd param was true/false)

(I'm realizing that what we really need is to support a whole list of "oldFormat" rather than just one - i.e. we should be supporting all the 0x01-prefixed values here. But you don't need to address that in this patch)

[noel gordon]

(In reply to comment #17)
> thanks for updating the tests - they lgtm now. (and yes, not sure why I thought the 3rd param was true/false)

: ) no worries.

> (I'm realizing that what we really need is to support a whole list of "oldFormat" rather than just one - i.e. we should be supporting all the 0x01-prefixed values here. But you don't need to address that in this patch)

Indeed.

[noel gordon]

(In reply to comment #3)
> I filed https://bugs.webkit.org/show_bug.cgi?id=102243 to track the issue this will cause with IndexedDB. We could land this first, but ideally 102243 would land at the same time or within a few days.

So is there anything more I need do here Josuha?

[Joshua Bell]

(In reply to comment #19)
> So is there anything more I need do here Josuha?

Not that I know of - I'd say go ahead and land it! I have 102243 ready to go... apart from tests and those will probably have to live in Chromium anyway. Users downgrading w/o deleting their profiles is not supported, just common enough that I want to be proactive so we don't have to land these together.

[Eric Seidel (no email)]

This looks reasonable, but I would love to have Dan Carney comment, given that he's been working in the v8-specific 8-bit string code so much lately.

[Eric Seidel (no email)]

Actually, this has little to do with v8-8bit strings, so Dan's comments although welcome, are not necessary.

[Eric Seidel (no email)]

Comment on attachment 176875 [details]
Patch

If Alec says LGTM, then LGTM too.

[WebKit Review Bot]

Comment on attachment 176875 [details]
Patch

Clearing flags on attachment: 176875

Committed r136624: <http://trac.webkit.org/changeset/136624>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.