New issue
Advanced search Search tips

Issue 686159 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 148757



Sign in to add a comment

Improve serialized script value wire format

Project Member Reported by jbroman@chromium.org, Jan 27 2017

Issue description

The V8-based serialized shipped in M57. For M58+, we can now make changes to the wire format to improve speed, correctness and maintainability. This bug aggregates that work.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 27 2017

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bf511b426ee5d65976e1b87c7bac134585f0fc9f

commit bf511b426ee5d65976e1b87c7bac134585f0fc9f
Author: jbroman <jbroman@chromium.org>
Date: Sat Jan 28 05:52:05 2017

ValueSerializer: Support efficiently reading and writing one-byte strings.

memcpy is faster than UTF-8 encoding/decoding. This yields 10-20% wins on
serializing and deserializing long ASCII strings, according to
blink_perf.bindings -- and these are already in a fast path where the entire
string is known to be ASCII (but this has to be checked). The win may be
larger for strings in Latin-1 but not ASCII (though I suspect this is an
uncommon case).

A change is also made to make ValueSerializerTest.EncodeTwoByteStringUsesPadding
survive wire format version number changes.

This is the first of a series of wire format changes from the previous Blink
format. The deserializer continues to be able to read the old format, but
Chromium M56 will no longer be able to read the messages written by this, in M58.

BUG= chromium:686159 

Review-Url: https://codereview.chromium.org/2658793004
Cr-Commit-Position: refs/heads/master@{#42753}

[modify] https://crrev.com/bf511b426ee5d65976e1b87c7bac134585f0fc9f/src/value-serializer.cc
[modify] https://crrev.com/bf511b426ee5d65976e1b87c7bac134585f0fc9f/src/value-serializer.h
[modify] https://crrev.com/bf511b426ee5d65976e1b87c7bac134585f0fc9f/test/unittests/value-serializer-unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b861a8400991976e6fd7d7abc9843e4fe7dc576e

commit b861a8400991976e6fd7d7abc9843e4fe7dc576e
Author: machenbach <machenbach@chromium.org>
Date: Mon Jan 30 19:35:28 2017

Revert of ValueSerializer: Distinguish between 'undefined' and an absent property. (patchset #2 id:20001 of https://codereview.chromium.org/2660093002/ )

Reason for revert:
Seems to break layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/13146

https://github.com/v8/v8/wiki/Blink-layout-tests

Original issue's description:
> ValueSerializer: Distinguish between 'undefined' and an absent property.
>
> Dealing with this case requires a wire format change. It is possible that an
> element can be absent even in an array where the dense format was chosen
> (because the array initially had no holes), if the elements are modified while
> they are being serialized. In this case, a new tag for the "hole" is emitted.
>
> The logic to treat undefined in dense arrays as an absent property is restricted
> to versions of the wire format that this tag did not exist.
>
> BUG= chromium:686159 , chromium:665820 
>
> Review-Url: https://codereview.chromium.org/2660093002
> Cr-Commit-Position: refs/heads/master@{#42784}
> Committed: https://chromium.googlesource.com/v8/v8/+/dc85f4c8338c1c824af4f7ee3274dc9f95d14e49

TBR=jkummerow@chromium.org,jbroman@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:686159 , chromium:665820 

Review-Url: https://codereview.chromium.org/2667553003
Cr-Commit-Position: refs/heads/master@{#42788}

[modify] https://crrev.com/b861a8400991976e6fd7d7abc9843e4fe7dc576e/src/value-serializer.cc
[modify] https://crrev.com/b861a8400991976e6fd7d7abc9843e4fe7dc576e/test/unittests/value-serializer-unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6f1639ed16a21b2395b98e8c61de268a275ee920

commit 6f1639ed16a21b2395b98e8c61de268a275ee920
Author: jbroman <jbroman@chromium.org>
Date: Tue Jan 31 01:54:26 2017

ValueSerializer: Distinguish between 'undefined' and an absent property.

Dealing with this case requires a wire format change. It is possible that an
element can be absent even in an array where the dense format was chosen
(because the array initially had no holes), if the elements are modified while
they are being serialized. In this case, a new tag for the "hole" is emitted.

The logic to treat undefined in dense arrays as an absent property is restricted
to versions of the wire format that this tag did not exist.

BUG= chromium:686159 , chromium:665820 

Review-Url: https://codereview.chromium.org/2660093002
Cr-Original-Commit-Position: refs/heads/master@{#42784}
Committed: https://chromium.googlesource.com/v8/v8/+/dc85f4c8338c1c824af4f7ee3274dc9f95d14e49
Review-Url: https://codereview.chromium.org/2660093002
Cr-Commit-Position: refs/heads/master@{#42800}

[modify] https://crrev.com/6f1639ed16a21b2395b98e8c61de268a275ee920/src/value-serializer.cc
[modify] https://crrev.com/6f1639ed16a21b2395b98e8c61de268a275ee920/test/unittests/value-serializer-unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/591cc0b4ccdb6934d76ea163569b5194faef4dcf

commit 591cc0b4ccdb6934d76ea163569b5194faef4dcf
Author: jbroman <jbroman@chromium.org>
Date: Wed Feb 01 22:27:02 2017

ValueSerializer: Share string encoding code with String and RegExp objects.

This avoids the need to pull in the UTF-8 encoding code from the public API,
and allows it to take advantage of any supported way that i::String can be
encoded (one- or two-byte).

Backward compatibility is maintained, but this is the behavior beginning
with this version.

BUG= chromium:686159 

Review-Url: https://codereview.chromium.org/2665653004
Cr-Commit-Position: refs/heads/master@{#42872}

[modify] https://crrev.com/591cc0b4ccdb6934d76ea163569b5194faef4dcf/src/value-serializer.cc
[modify] https://crrev.com/591cc0b4ccdb6934d76ea163569b5194faef4dcf/src/value-serializer.h
[modify] https://crrev.com/591cc0b4ccdb6934d76ea163569b5194faef4dcf/test/unittests/value-serializer-unittest.cc

Components: Blink>Messaging
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faff3132ee3d5a89a85baab94d99d80fa54559d1

commit faff3132ee3d5a89a85baab94d99d80fa54559d1
Author: jbroman <jbroman@chromium.org>
Date: Sun Feb 26 20:48:45 2017

V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG= 686159 
TBR=skyostil@chromium.org

Review-Url: https://codereview.chromium.org/2712783002
Cr-Commit-Position: refs/heads/master@{#453123}

[modify] https://crrev.com/faff3132ee3d5a89a85baab94d99d80fa54559d1/content/browser/android/string_message_codec.cc
[modify] https://crrev.com/faff3132ee3d5a89a85baab94d99d80fa54559d1/third_party/WebKit/LayoutTests/crypto/subtle/resources/common.js
[modify] https://crrev.com/faff3132ee3d5a89a85baab94d99d80fa54559d1/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
[modify] https://crrev.com/faff3132ee3d5a89a85baab94d99d80fa54559d1/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
[modify] https://crrev.com/faff3132ee3d5a89a85baab94d99d80fa54559d1/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6543519977b2012b58a4ffef28b8527db404fbdb

commit 6543519977b2012b58a4ffef28b8527db404fbdb
Author: jbroman <jbroman@chromium.org>
Date: Tue Feb 28 02:16:30 2017

ValueSerializer: Add an explicit tag for host objects.

This makes it no longer necessary to ensure that V8 and Blink have non-colliding
tags, which makes it easier for them to evolve independently, and also makes
the wire format more suitable for other V8 embedders, who would not
necessarily be surveyed before V8 introduced a new tag that might collide
with theirs.

BUG= chromium:686159 

Review-Url: https://codereview.chromium.org/2709023003
Cr-Commit-Position: refs/heads/master@{#43466}

[modify] https://crrev.com/6543519977b2012b58a4ffef28b8527db404fbdb/src/value-serializer.cc
[modify] https://crrev.com/6543519977b2012b58a4ffef28b8527db404fbdb/test/unittests/value-serializer-unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e96e467116cd7e573911400b9b1d383a46e775f

commit 3e96e467116cd7e573911400b9b1d383a46e775f
Author: jbroman <jbroman@chromium.org>
Date: Wed Mar 01 03:24:38 2017

Rebaseline crypto/subtle/*/cloneKey.html.

This V8-side change:
  https://codereview.chromium.org/2709023003
added an explicit one-byte "host object" prefix to Blink's objects.

BUG= 686159 

Review-Url: https://codereview.chromium.org/2726573002
Cr-Commit-Position: refs/heads/master@{#453822}

[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/aes-cbc/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/aes-ctr/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/aes-gcm/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/aes-kw/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/ecdh/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/ecdsa/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/hkdf/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/hmac/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/pbkdf2/cloneKey-expected.txt
[modify] https://crrev.com/3e96e467116cd7e573911400b9b1d383a46e775f/third_party/WebKit/LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5/cloneKey-expected.txt

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf016e6d001abcfe472090c052c98c8e6165c09b

commit bf016e6d001abcfe472090c052c98c8e6165c09b
Author: jonross <jonross@chromium.org>
Date: Thu Mar 02 17:40:22 2017

Revert of Rebaseline crypto/subtle/*/cloneKey.html. (patchset #1 id:1 of https://codereview.chromium.org/2726573002/ )

Reason for revert:
The v8 version which these tests are based on was rolled back: https://codereview.chromium.org/2728453005

So the tests are now failing.

Original issue's description:
> Rebaseline crypto/subtle/*/cloneKey.html.
>
> This V8-side change:
>   https://codereview.chromium.org/2709023003
> added an explicit one-byte "host object" prefix to Blink's objects.
>
> BUG= 686159 
>
> Review-Url: https://codereview.chromium.org/2726573002
> Cr-Commit-Position: refs/heads/master@{#453822}
> Committed: https://chromium.googlesource.com/chromium/src/+/3e96e467116cd7e573911400b9b1d383a46e775f

TBR=haraken@chromium.org,jbroman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 686159 

Review-Url: https://codereview.chromium.org/2725763004
Cr-Commit-Position: refs/heads/master@{#454296}

[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/aes-cbc/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/aes-ctr/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/aes-gcm/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/aes-kw/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/ecdh/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/ecdsa/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/hkdf/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/hmac/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/pbkdf2/cloneKey-expected.txt
[modify] https://crrev.com/bf016e6d001abcfe472090c052c98c8e6165c09b/third_party/WebKit/LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5/cloneKey-expected.txt

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a1788dd92171c1f9384a3ebd6b751e1aa239926

commit 4a1788dd92171c1f9384a3ebd6b751e1aa239926
Author: jbroman <jbroman@chromium.org>
Date: Mon Mar 06 00:48:18 2017

Rebaseline crypto/subtle/*/cloneKey.html.

This V8-side change:
  https://codereview.chromium.org/2709023003
added an explicit one-byte "host object" prefix to Blink's objects.

BUG= 686159 

Review-Url: https://codereview.chromium.org/2726573002
Cr-Original-Commit-Position: refs/heads/master@{#453822}
Committed: https://chromium.googlesource.com/chromium/src/+/3e96e467116cd7e573911400b9b1d383a46e775f
Review-Url: https://codereview.chromium.org/2726573002
Cr-Commit-Position: refs/heads/master@{#454804}

[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/aes-cbc/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/aes-ctr/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/aes-gcm/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/aes-kw/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/ecdh/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/ecdsa/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/hkdf/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/hmac/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/pbkdf2/cloneKey-expected.txt
[modify] https://crrev.com/4a1788dd92171c1f9384a3ebd6b751e1aa239926/third_party/WebKit/LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5/cloneKey-expected.txt

Project Member

Comment 12 by bugdroid1@chromium.org, Mar 15 2017

Labels: merge-merged-5.8
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/41e12ea699bfb7ab564606b8764b3dcd5d12244e

commit 41e12ea699bfb7ab564606b8764b3dcd5d12244e
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Wed Mar 15 12:12:55 2017

Merged: ValueSerializer: Add an explicit tag for host objects.

Revision: 6543519977b2012b58a4ffef28b8527db404fbdb

BUG= chromium:686159 , chromium:700603 , v8:6080 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=hablich@chromium.org

Review-Url: https://codereview.chromium.org/2749263002 .
Cr-Commit-Position: refs/branch-heads/5.8@{#29}
Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1}
Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429}

[modify] https://crrev.com/41e12ea699bfb7ab564606b8764b3dcd5d12244e/src/value-serializer.cc
[modify] https://crrev.com/41e12ea699bfb7ab564606b8764b3dcd5d12244e/test/unittests/value-serializer-unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/7e60bc33783b9cddd87aa64d8da075776f111807

commit 7e60bc33783b9cddd87aa64d8da075776f111807
Author: jbroman <jbroman@chromium.org>
Date: Sat Apr 01 01:40:39 2017

ValueSerializer: add kOneByteString to expected key fast path.

This was missed when Latin-1 encoding replaced UTF-8 encoding when one-byte
strings (like most keys) are serialized.

BUG= chromium:686159 

Review-Url: https://codereview.chromium.org/2784423002
Cr-Commit-Position: refs/heads/master@{#44320}

[modify] https://crrev.com/7e60bc33783b9cddd87aa64d8da075776f111807/src/value-serializer.cc

Status: Fixed (was: Started)
Done, for now, at least.

Sign in to add a comment