Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803593005:
Avoid unnecessary byte-swapping in blink's SerializedScriptValue. (Closed)
DescriptionAvoid unnecessary byte-swapping in blink's SerializedScriptValue.
For historical reasons, SerializedStringValue byte-swaps its buffer when
serializing to a string. This CL removes the unnecessary flipping for
current values, and sets up a legacy deserialization code path, so
byte-swapped SSVs can still be read correctly.
BUG=
Review-Url: https://codereview.chromium.org/2803593005
Cr-Commit-Position: refs/heads/master@{#464833}
Committed: https://chromium.googlesource.com/chromium/src/+/9e27be00af394259afe4fd40b0c5955a29b29c40
Patch Set 1 #Patch Set 2 : Fix test bug highlighted by ASAN. #
Total comments: 2
Patch Set 3 : Correct handling for SSV version 0. #
Total comments: 22
Patch Set 4 : Address jsbell feedback. #Patch Set 5 : Rebased past the Blink rename. #
Total comments: 3
Patch Set 6 : Addressed jbroman feedback. #
Total comments: 10
Patch Set 7 : Addressed jbroman feedback. #
Messages
Total messages: 74 (47 generated)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + jbroman@chromium.org
PTAL? I stumbled upon the byte-flipping code while writing an IndexedDB serialization test, and it seems to me that we can (almost) get rid of it. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tl;dr: I think it's slightly more complicated than it seems, but I'd be happy to see this code go away. https://codereview.chromium.org/2803593005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:114: // will be >= 128 even when the version number exceeds 128. So, the check The potential issue here is that we didn't always have a version tag. IIRC, at version 0 in ancient times, a string could be encoded as (before byte-swapping): 0x53 0xff 0x01 [255 bytes of string data here] That is, [UTF-8 string] [varint-encoded 255] [data]. After byte-swapping, this is: 0xff 0x53 [first byte of string] 0x01 [remaining string data, byte-swapped] But when we're reading this, it could hypothetically be version 83 (0x53) of the format (without byte-swapping), from the distant future. How can we know for certain? I don't know for certain what the status of IndexedDB was at the time, and I'd need to double-check the particulars in history, but this concern is why I didn't immediately do this (not to say it can't be done -- I didn't spend a lot of time thinking about it). One thing we could certainly do is store the endianness in the IndexedDB metadata, e.g. by changing the IDB schema version (and either "upgrade" data by byte-swapping it back, or just do a swap on read for all database backing stores created before this change), which would at least move this further down the stack. Or possibly you can determine with research that my concern above is immaterial (maybe I'm misremembering the wire format, or maybe that was before Chrome shipped IndexedDB, or maybe there's an even more clever trick).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
jsbell: Can you please also weigh in on the SSV version burning idea below, and on the timing around IndexedDB? https://codereview.chromium.org/2803593005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:114: // will be >= 128 even when the version number exceeds 128. So, the check On 2017/04/06 01:04:37, jbroman wrote: > The potential issue here is that we didn't always have a version tag. IIRC, at > version 0 in ancient times, a string could be encoded as (before byte-swapping): > > 0x53 0xff 0x01 [255 bytes of string data here] > > That is, [UTF-8 string] [varint-encoded 255] [data]. After byte-swapping, this > is: > > 0xff 0x53 [first byte of string] 0x01 [remaining string data, byte-swapped] > > But when we're reading this, it could hypothetically be version 83 (0x53) of the > format (without byte-swapping), from the distant future. How can we know for > certain? > > I don't know for certain what the status of IndexedDB was at the time, and I'd > need to double-check the particulars in history, but this concern is why I > didn't immediately do this (not to say it can't be done -- I didn't spend a lot > of time thinking about it). > > One thing we could certainly do is store the endianness in the IndexedDB > metadata, e.g. by changing the IDB schema version (and either "upgrade" data by > byte-swapping it back, or just do a swap on read for all database backing stores > created before this change), which would at least move this further down the > stack. > > Or possibly you can determine with research that my concern above is immaterial > (maybe I'm misremembering the wire format, or maybe that was before Chrome > shipped IndexedDB, or maybe there's an even more clever trick). The trick that I suggest is that we "burn" future versions that could conflict with tags in the SSV serialization version 0 which could be followed by a 0xFF. The burning would be done via a comment near the Blink wire version that explains the whole situation. We would need to burn the following 13 versions (obtained from the negative diffs in [1]): 35 - 0x23 - # - ImageDataTag 64 - 0x40 - @ - SparseArrayTag 68 - 0x44 - D - DateTag 73 - 0x49 - I - Int32Tag 78 - 0x4E - N - NumberTag 82 - 0x52 - R - RegExpTag 83 - 0x53 - S - StringTag 85 - 0x55 - U - Uint32Tag 91 - 0x5B - [ - ArrayTag 98 - 0x62 - b - BlobTag 102 - 0x66 - f - FileTag 108 - 0x6C - l - FileListTag 123 - 0x7B - { - ObjectTag When skipping a "burned" version, we'd probably want to consider whether disks with SSV version 0 data can still be around. For this purpose, I'd add the following information to the burning comment. SSV serialization version 1 was introduced in WebKit r91300 [1], which shipped in Chrome 14 [2], which was released on September 16, 2011 [3]. IndexedDB was shipped in Chrome 11 [4], which was released on April 27, 2011. I care about the timing information because, given the current development speed, I'd imagine that we'll hit version 35 in ~11 years, around 2028. If I'm still working on this piece of code in 2028, I'll probably argue that it's highly unlikely that disk drives sold in 2011 are still operating :) WDYT about version burning? [1] https://trac.webkit.org/changeset/91300/webkit [2] "git show 14.0.835.0" shows WebKit r91698 [3] https://en.wikipedia.org/wiki/Google_Chrome_version_history [4] http://www.chromium.org/developers/web-platform-status#TOC-Indexed-Database-API-
While I ponder your questions, here's another idea to throw out: is there an unambiguous prefix byte (or bytes) we can introduce that could never have been previously serialized? We could use that as the signal "no need to flip this, but skip this byte", and the absence of the prefix indicates we need to flip. That would act as a "meta" version byte.
On 2017/04/06 at 17:07:47, jsbell wrote: > While I ponder your questions, here's another idea to throw out: is there an unambiguous prefix byte (or bytes) we can introduce that could never have been previously serialized? We could use that as the signal "no need to flip this, but skip this byte", and the absence of the prefix indicates we need to flip. That would act as a "meta" version byte. Something like the standard BOM (FE FF) would work, I think. It does make our prelude even longer (though still only 6 bytes in total at the moment).
On 2017/04/06 17:07:47, jsbell wrote: > While I ponder your questions, here's another idea to throw out: is there an > unambiguous prefix byte (or bytes) we can introduce that could never have been > previously serialized? We could use that as the signal "no need to flip this, > but skip this byte", and the absence of the prefix indicates we need to flip. > That would act as a "meta" version byte. ... and we could do this at the IDB level - and rip the flipping out of toWireBytes() - so no-one else (e.g. postMessage) is paying the penalty. Basically, introduce an IDB envelope around the Blink envelope (using jbroman@'s terms from the bindings code) From https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... it looks like we'd need a prefix that byte-swaps into 0xFF followed by a varint that we reserve as an invalid blink envelope version. If seen we'd slice this off at the IDB level and not bother byte swapping before deserializing. If not seen we byte swap before deserializing. That burns a "future version" and means we're prefixing serialized values with dummy bytes, and we know all these string concatenations churn memory, so maybe this is not a win, but just throwing the idea out.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
On 2017/04/06 17:45:25, jsbell wrote: > On 2017/04/06 17:07:47, jsbell wrote: > > While I ponder your questions, here's another idea to throw out: is there an > > unambiguous prefix byte (or bytes) we can introduce that could never have been > > previously serialized? We could use that as the signal "no need to flip this, > > but skip this byte", and the absence of the prefix indicates we need to flip. > > That would act as a "meta" version byte. > > ... and we could do this at the IDB level - and rip the flipping out of > toWireBytes() - so no-one else (e.g. postMessage) is paying the penalty. > Basically, introduce an IDB envelope around the Blink envelope (using jbroman@'s > terms from the bindings code) > > From > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > it looks like we'd need a prefix that byte-swaps into 0xFF followed by a varint > that we reserve as an invalid blink envelope version. If seen we'd slice this > off at the IDB level and not bother byte swapping before deserializing. If not > seen we byte swap before deserializing. > > That burns a "future version" and means we're prefixing serialized values with > dummy bytes, and we know all these string concatenations churn memory, so maybe > this is not a win, but just throwing the idea out. I would prefer that we take on a bit of mental burden and not add more envelopes and string operations :) I fleshed out what my proposal would look like in a new patch set. I also added a test that we decode one of the weird cases. If it's desirable, I'd prefer to add tests for all the v0 edge cases (tag + 0xFF), rather than add another prefix. PTAL and tell me your thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looking good, I think... https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:93: // Versions 16 and below (prior to April 2017) used ntohs() to byte-flip SSV nit: "byte flip" or "byte swap"? "swap" seems more common https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:101: // endianness yet. ARCH_CPU_LITTLE_ENDIAN seems used in several places? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:108: if (static_cast<uint8_t>(data[0]) == VersionTag) { Maybe precede this with a list of the different cases we are detecting: // v0 (byte-fliped): [ d, t, ...] where t is a tag byte, d is first data byte // v1 - 16 (byte-swapped): [ v, 0xFF, ... ] where v is the version // v17+ (non-byte-swapped): [ 0xFF, v, ... ] where v is the version https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:113: // cannot be used as version numbers in the Blink-side SSV envelope. nit: remove extra leading space https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: for (size_t i = 0; i < 13; ++i) { ARRAY_SIZE(version0Tags) (Also, could binary search. Probably overkill.) https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:224: static const uint8_t kNullData[] = {0xff, 17, 0xff, 13, '0', 0x00}; Maybe document what this is, while we're here? Is it VersionTag, blink version, VersionTag, v8 version, NullTag, padding? Can we use some of those consts here? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:244: size_t resultSize = (m_dataBufferSize + 1) & ~1; How feasible is follow-on work to not expose the results via String and thus require padding to an even size? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:82: // DO NOT USE: 35, 64, 68, 73, 78, 82, 83, 85, 91, 98, 102, 108, 123. static_assert() please!
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Avoid unnecessary byte-flipping in blink's SerializedScriptValue. For historical reasons, SerializedStringValue byte-flips its buffer when serializing to a string. This CL removes the unnecessary flipping for current values, and sets up a legacy deserialization code path, so byte-flipped SSVs can still be read correctly. BUG= ========== to ========== Avoid unnecessary byte-swapping in blink's SerializedScriptValue. For historical reasons, SerializedStringValue byte-swaps its buffer when serializing to a string. This CL removes the unnecessary flipping for current values, and sets up a legacy deserialization code path, so byte-swapped SSVs can still be read correctly. BUG= ==========
jsbell: Thank you very much for the super-useful feedback! PTAL? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:93: // Versions 16 and below (prior to April 2017) used ntohs() to byte-flip SSV On 2017/04/07 23:40:34, jsbell wrote: > nit: "byte flip" or "byte swap"? "swap" seems more common Done. After reading "byte-swap", my brain instantly agreed that it is the name used by pretty much every old doc I've read. Thank you! https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:101: // endianness yet. On 2017/04/07 23:40:34, jsbell wrote: > ARCH_CPU_LITTLE_ENDIAN seems used in several places? Sadly, it seems like we're not allowed to use it in Blink? ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp Illegal include: "build/build_config.h" Because of "-build" from third_party's include_rules. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:108: if (static_cast<uint8_t>(data[0]) == VersionTag) { On 2017/04/07 23:40:34, jsbell wrote: > Maybe precede this with a list of the different cases we are detecting: > > // v0 (byte-fliped): [ d, t, ...] where t is a tag byte, d is first data > byte > // v1 - 16 (byte-swapped): [ v, 0xFF, ... ] where v is the version > // v17+ (non-byte-swapped): [ 0xFF, v, ... ] where v is the version > Done. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:113: // cannot be used as version numbers in the Blink-side SSV envelope. On 2017/04/07 23:40:34, jsbell wrote: > nit: remove extra leading space Done. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: for (size_t i = 0; i < 13; ++i) { On 2017/04/07 23:40:34, jsbell wrote: > ARRAY_SIZE(version0Tags) Done. > (Also, could binary search. Probably overkill.) I'm afraid that binary search would be slightly worse for this small set, because it doesn't work with branch predictors and cache prefetchers. Although computer architecture might be quite different by 2028 :), I doubt we'll witness the kind of miraculous RAM improvement that would make the concerns above irrelevant. Fun aside, TBH I expect that we'll kill version 0 support by the time this code path actually makes it into the binary. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:224: static const uint8_t kNullData[] = {0xff, 17, 0xff, 13, '0', 0x00}; On 2017/04/07 23:40:34, jsbell wrote: > Maybe document what this is, while we're here? > > Is it VersionTag, blink version, VersionTag, v8 version, NullTag, padding? Can > we use some of those consts here? Done. kNull is not exported from V8, so I couldn't use it. After a bit of thinking, I realized that we probably don't want to use V8's constants directly anyway, and I documented why that's the case. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:244: size_t resultSize = (m_dataBufferSize + 1) & ~1; On 2017/04/07 23:40:34, jsbell wrote: > How feasible is follow-on work to not expose the results via String and thus > require padding to an even size? Seems fairly feasible to me. There aren't a lot of calls to toWireString(). I'd like to do the follow-up, mostly because I'm hoping it opens up the way to exposing the SSV buffer and avoiding a copy altogether. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:82: // DO NOT USE: 35, 64, 68, 73, 78, 82, 83, 85, 91, 98, 102, 108, 123. On 2017/04/07 23:40:34, jsbell wrote: > static_assert() please! Done. Thanks for the suggestion! That's a great idea! I placed the static_assert near the code that uses the assumption, to help a curious developer who might tinker with this code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I rebased past the Blink rename. Sorry, the diff between patch sets 4 and 5 is horrible.
lgtm https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:136: constexpr static size_t kSSVHeaderBlinkVersionOffset = 1; Should we note that versions are (now) varints, so when > 127 the offsets (and test logic) will need to change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:148: // Slower path that would kick in after version 35, assuming we don't remove This is dead code. Is it actually useful to have it here before version 35? Maybe we should just assert that it is. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:151: static uint8_t version0Tags[] = {35, 64, 68, 73, 78, 82, 83, const https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: for (size_t i = 0; i < 13; ++i) { This loop is equivalent to: return std::count(std::begin(version0Tags), std::end(version0Tags), data[1]); or return std::find(std::begin(version0Tags), std::end(version0Tags), data[1]) != std::end(version0Tags);
On 2017/04/11 at 15:01:47, jbroman wrote: > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): > > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:148: // Slower path that would kick in after version 35, assuming we don't remove > This is dead code. Is it actually useful to have it here before version 35? Maybe we should just assert that it is. > > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:151: static uint8_t version0Tags[] = {35, 64, 68, 73, 78, 82, 83, > const > > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: for (size_t i = 0; i < 13; ++i) { > This loop is equivalent to: > > return std::count(std::begin(version0Tags), std::end(version0Tags), data[1]); > > or > > return std::find(std::begin(version0Tags), std::end(version0Tags), data[1]) != std::end(version0Tags); Urgh, I thought I'd published these comments earlier.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by pwnall@chromium.org
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
jbroman: Thank you very much for the review! PTAL? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:148: // Slower path that would kick in after version 35, assuming we don't remove On 2017/04/11 15:01:47, jbroman wrote: > This is dead code. Is it actually useful to have it here before version 35? > Maybe we should just assert that it is. I felt bad leaving a static_assert that'd force a future maintainer to write the code below at some point in the future. This is completely up to you... if you prefer, I'm happy to replace the code with a static_assert :) https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:151: static uint8_t version0Tags[] = {35, 64, 68, 73, 78, 82, 83, On 2017/04/11 15:01:47, jbroman wrote: > const Done. https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: for (size_t i = 0; i < 13; ++i) { On 2017/04/11 15:01:47, jbroman wrote: > This loop is equivalent to: > > return std::count(std::begin(version0Tags), std::end(version0Tags), data[1]); > > or > > return std::find(std::begin(version0Tags), std::end(version0Tags), data[1]) != > std::end(version0Tags); Done. https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:136: constexpr static size_t kSSVHeaderBlinkVersionOffset = 1; On 2017/04/10 22:49:39, jsbell wrote: > Should we note that versions are (now) varints, so when > 127 the offsets (and > test logic) will need to change? Ack. The versions were always varints, at least as far as the serialization / deserialization code is concerned. Is it OK if I add static_asserts in a follow-up?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:136: constexpr static size_t kSSVHeaderBlinkVersionOffset = 1; On 2017/04/11 19:10:55, pwnall wrote: > Is it OK if I add static_asserts in a follow-up? SGTM. (I was just thinking about adding a comment, not code. static_assert in a follow-up is even better.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm It might be wise to wait until after the M59 branch to land, to be safe (since this is rather subtle). https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:187: // which was byte-flipped. change this comment and others to say "swapped" rather than "flipped", for consistency? https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:242: kVersionTag, kWireFormatVersion, // Blink envelope I don't strenuously object, but this level of detail isn't really necessary because we expect to be able to decode old versions into the distant future (and there's a test that this value deserializes properly, so if that changes we'll know). Using the current wire format version might actually make it more brittle, because it means this code will need to change if we change the format in a way that makes this not the way null is written. So I think having a fixed 4-6 byte thing with the comment "this is how we wrote null at some point in the past" is probably enough. https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp:23: v8::Boolean::New(scope.GetIsolate(), true); More concisely: v8::True(scope.GetIsolate()); At that point, might as well inline it at it's sole use, below. https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp:123: TEST(SerializedScriptValueTest, NullValue) { Is this substantially different from V8ScriptValueSerializerTest.DecodeHardcodedNullValue? https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:168: ASSERT_EQ(v8::ValueSerializer::GetCurrentDataFormatVersion(), I'm going to move this value around soon, because it's needed at compile time on another CL. Just FYI (doesn't really matter who resolves the merge conflict).
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@chromium.org: Please review the change to third_party/WebKit/public/web/WebSerializedScriptValueVersion.h https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:187: // which was byte-flipped. On 2017/04/12 18:34:04, jbroman wrote: > change this comment and others to say "swapped" rather than "flipped", for > consistency? Done. Thanks for catching this! I did a find-replace and got rid of a few instances that managed to hide from me. https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:242: kVersionTag, kWireFormatVersion, // Blink envelope On 2017/04/12 18:34:04, jbroman wrote: > I don't strenuously object, but this level of detail isn't really necessary > because we expect to be able to decode old versions into the distant future (and > there's a test that this value deserializes properly, so if that changes we'll > know). > > Using the current wire format version might actually make it more brittle, > because it means this code will need to change if we change the format in a way > that makes this not the way null is written. So I think having a fixed 4-6 byte > thing with the comment "this is how we wrote null at some point in the past" is > probably enough. Done. I'll explain my reasoning for the previous approach, and I can bring it back in a follow-up patch if the explanation changes your mind. FWIW, there is a minor performance advantage to using the current wire version -- it can make the branch predictor happier. While I agree that hard-wiring the Blink-side version can make the code a bit more brittle, I think the opportunity is quite low, because the V8 serialization version is fixed. There's still the possibility that the whole wrapper changes with a Blink version bump, but we don't seem to do that very often. https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp:23: v8::Boolean::New(scope.GetIsolate(), true); On 2017/04/12 18:34:04, jbroman wrote: > More concisely: v8::True(scope.GetIsolate()); > > At that point, might as well inline it at it's sole use, below. Done. Thank you! https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp:123: TEST(SerializedScriptValueTest, NullValue) { On 2017/04/12 18:34:04, jbroman wrote: > Is this substantially different from > V8ScriptValueSerializerTest.DecodeHardcodedNullValue? No. If the intent of the test is to make sure that SerializedScriptValue::NullValue() returns something reasonable, I'd argue that it belongs in SerializedScriptValueTest, because NullValue is defined on SerializedScriptValue. I removed the test. I can add it back in a follow-up if you agree with my argument. https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:168: ASSERT_EQ(v8::ValueSerializer::GetCurrentDataFormatVersion(), On 2017/04/12 18:34:04, jbroman wrote: > I'm going to move this value around soon, because it's needed at compile time on > another CL. Just FYI (doesn't really matter who resolves the merge conflict). Acknowledged. Thank you very much for the heads-up!
On 2017/04/12 18:34:04, jbroman wrote: > lgtm > > It might be wise to wait until after the M59 branch to land, to be safe (since > this is rather subtle). I was optimistically hoping I'd get this in M59, but I'll defer to your wisdom.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/13 03:53:29, kinuko wrote: > lgtm jsbell, jbroman, kinuko: Thank you very much for the review!
On 2017/04/12 at 21:44:42, pwnall wrote: > On 2017/04/12 18:34:04, jbroman wrote: > > lgtm > > > > It might be wise to wait until after the M59 branch to land, to be safe (since > > this is rather subtle). > > I was optimistically hoping I'd get this in M59, but I'll defer to your wisdom. I've just recently been bitten by a wire format change having an effect I didn't expect, hence my reticence to not give this a little more time in canary before branch. :) (And I don't think there's a ton of upside to shipping this in 59 as opposed to 60.)
On 2017/04/13 at 06:14:34, pwnall wrote: > On 2017/04/13 03:53:29, kinuko wrote: > > lgtm > > jsbell, jbroman, kinuko: Thank you very much for the review! Thanks for helping make our wire format make slightly more sense!
On 2017/04/13 14:48:31, jbroman wrote: > On 2017/04/13 at 06:14:34, pwnall wrote: > > On 2017/04/13 03:53:29, kinuko wrote: > > > lgtm > > > > jsbell, jbroman, kinuko: Thank you very much for the review! > > Thanks for helping make our wire format make slightly more sense!
Sorry about the dummy reply, hit the wrong button... On 2017/04/13 14:47:57, jbroman wrote: > > (And I don't think there's a ton of upside to shipping this in 59 as opposed to > 60.) +1 - I'd wait. (I'm generally a huge fan of "bake time" - comes from being a release manager for several years.)
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2803593005/#ps180001 (title: "Addressed jbroman feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/13 15:34:18, jsbell wrote: > Sorry about the dummy reply, hit the wrong button... > > On 2017/04/13 14:47:57, jbroman wrote: > > > > (And I don't think there's a ton of upside to shipping this in 59 as opposed > to > > 60.) > > +1 - I'd wait. (I'm generally a huge fan of "bake time" - comes from being a > release manager for several years.) jsbell, jbroman: Thank you very much for the advice and for your patience with this review! M59 is branched, so I'm submitting the CL.
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492208386848890,
"parent_rev": "bec44505ea17b6b8fbd366ab22ca724f3c448fe6", "commit_rev":
"9e27be00af394259afe4fd40b0c5955a29b29c40"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary byte-swapping in blink's SerializedScriptValue. For historical reasons, SerializedStringValue byte-swaps its buffer when serializing to a string. This CL removes the unnecessary flipping for current values, and sets up a legacy deserialization code path, so byte-swapped SSVs can still be read correctly. BUG= ========== to ========== Avoid unnecessary byte-swapping in blink's SerializedScriptValue. For historical reasons, SerializedStringValue byte-swaps its buffer when serializing to a string. This CL removes the unnecessary flipping for current values, and sets up a legacy deserialization code path, so byte-swapped SSVs can still be read correctly. BUG= Review-Url: https://codereview.chromium.org/2803593005 Cr-Commit-Position: refs/heads/master@{#464833} Committed: https://chromium.googlesource.com/chromium/src/+/9e27be00af394259afe4fd40b0c5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/9e27be00af394259afe4fd40b0c5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
