[v8] prepare SerializedScriptValue for transition to Latin-1
Created attachment 184190 [details] Patch
Comment on attachment 184190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184190&action=review > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:13 > +var curVer = 0x02ff; Nit: curVer => currentVersion WebKit uses a fully qualified variable name. > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:28 > - [ 0x02ff, 0x003f, 0x3f6f, 0x5301, 0x6603, > + [ curVer, 0x003f, 0x3f6f, 0x5301, 0x6603, Won't this change lose compatibility for already stored serialized script values (e.g. localStorage)? (I just wanted to confirm that it's OK to change the serialized results.)
Comment on attachment 184190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184190&action=review >> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:13 >> +var curVer = 0x02ff; > > Nit: curVer => currentVersion > > WebKit uses a fully qualified variable name. I used curVer to have 6 characters and keep all the byte arrays nicely lined up. >> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:28 >> + [ curVer, 0x003f, 0x3f6f, 0x5301, 0x6603, > > Won't this change lose compatibility for already stored serialized script values (e.g. localStorage)? (I just wanted to confirm that it's OK to change the serialized results.) This change is backwards but not forwards compatible. If i'm reading SerializedScriptValue.cpp correctly, this bumping the version number is the correct way to do this transition.
(In reply to comment #3) > I used curVer to have 6 characters and keep all the byte arrays nicely lined up. Ah, makes sense. > This change is backwards but not forwards compatible. If i'm reading SerializedScriptValue.cpp correctly, this bumping the version number is the correct way to do this transition. Looks like so. jsbell, alecflett: would you check that the patch is safe in terms of compatibility?
jsbell, alecflett: just to clarify what will happen. Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf. This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value. Version 3 can read version 2 without issue.
(In reply to comment #5) > jsbell, alecflett: just to clarify what will happen. Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf. This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value. Version 3 can read version 2 without issue. Thanks. Conceptually, this looks correct. There is a problem, though - this conceptually *removes* test coverage for parsing v2 SSVs, since with this patch and once the new flag is defined, the test will only exercise parsing v3 SSVs. You'll need to add tests that use the (optional) oldFormat param of _testSerialization and explicitly test 0x02ff, or expand testSerialization() to insert the version prefix since most v2 and v3 SSVs will serialize identically. I'm not sure which is better. Or perhaps oldFormat should turn into a map of version/serializations? Not sure what is best here. (alecflett's not going to be able to look at this until tomorrow, unfortunately.)
(In reply to comment #6) > (In reply to comment #5) > > jsbell, alecflett: just to clarify what will happen. Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf. This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value. Version 3 can read version 2 without issue. > > Thanks. Conceptually, this looks correct. > > There is a problem, though - this conceptually *removes* test coverage for parsing v2 SSVs, since with this patch and once the new flag is defined, the test will only exercise parsing v3 SSVs. > > You'll need to add tests that use the (optional) oldFormat param of _testSerialization and explicitly test 0x02ff, or expand testSerialization() to insert the version prefix since most v2 and v3 SSVs will serialize identically. I'm not sure which is better. Or perhaps oldFormat should turn into a map of version/serializations? Not sure what is best here. > > (alecflett's not going to be able to look at this until tomorrow, unfortunately.) Good point. I'll work something into the test to test both v2 and v3, including something that differs between v2 and v3.
Created attachment 184470 [details] Patch
After looking into it, I realized that only utf8 and two byte strings are supported by the current system. I've adjusted the patch to handle this, without needing to bump the version. It comes at a performance cost, but once v8 latin-1 support is stable, I'll implement a version 3 SSV which handles one byte strings directly, without the need to unnecessarily jump in and out of utf8.
Yes, the test should be purely additive - all existing tests should remain or be tweaked to indicate that its testing an old version (to avoid the bidirectional serialization tests for those) (The "old version" parameter should probably be tweaked to take a list of old versioned data, so we can keep adding to the test with each subsequent version)
Comment on attachment 184470 [details] Patch Attachment 184470 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16119005 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
jsbell: Does it look fine?
Comment on attachment 184470 [details] Patch Attachment 184470 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16112351 New failing tests: svg/as-image/img-preserveAspectRatio-support-2.html
Comment on attachment 184470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184470&action=review Looks fine to me. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:324 > + ensureSpace(utf8Length); Could move this up and use ensureSpace(sizeof(StringTag) + sizeof(uint32_t) + utf8Length) (probably premature optimization, though)
Comment on attachment 184470 [details] Patch Please apply jsbell's comment before landing.
(In reply to comment #15) > (From update of attachment 184470 [details]) > Please apply jsbell's comment before landing. kentaro, I'm marking this cq? for now as I have no access to a dev environment this weekend. I have to do a bunch of optimization in this class after latin-1 is stable in v8, so I'll apply jsbell's optimization there instead.
Comment on attachment 184470 [details] Patch Clearing flags on attachment: 184470 Committed r140911: <http://trac.webkit.org/changeset/140911>
All reviewed patches have been landed. Closing bug.
(In reply to comment #9) > After looking into it, I realized that only utf8 and two byte strings are supported by the current system. huh? > I've adjusted the patch to handle this, without needing to bump the version. It comes at a performance cost, but once v8 latin-1 support is stable, I'll implement a version 3 SSV which handles one byte strings directly, without the need to unnecessarily jump in and out of utf8. And did we file about that somewhere?
This bug is clearly dead since we've removed V8 from WebKit.
Remove yourself from the bug then, seeing as it's dead to you.