Blink-compatible serialization of oddball values.

Blink-compatible serialization of oddball values.

BUG=chromium:148757
Committed: https://crrev.com/e6d1a80e790117dc27b20e9b14de4b6ac9f7512f
Cr-Commit-Position: refs/heads/master@{#38625}

[jbroman, 2016-08-11 18:10:18]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 18:10:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:12:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 18:12:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[jbroman, 2016-08-11 18:14:03]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 18:14:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:16:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 18:16:16]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[jbroman, 2016-08-11 18:23:28]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 18:23:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:27:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 18:27:05]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[jbroman, 2016-08-11 18:29:25]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 18:29:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-08-11 19:00:21]

jbroman@chromium.org changed reviewers:
+ cbruni@chromium.org

[jbroman, 2016-08-11 19:00:22]

In the spirit of getting this ball rolling, I'd like to start landing the
serialization logic in hopefully review-sized pieces. While discussion[1] is
ongoing, implementing something capable of reading the existing format and with
room for evolution seems like a practical approach for now.

[1]:
https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/9Rad...

[jbroman, 2016-08-11 19:00:39]

Description was changed from

==========
Legacy Blink-compatible serialization of oddball values.

BUG=chromium:148757
==========

to

==========
Blink-compatible serialization of oddball values.

BUG=chromium:148757
==========

[commit-bot: I haz the power, 2016-08-11 19:03:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 19:03:21]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-08-12 09:54:44]

LGTM with nits

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.cc...
src/value-serializer.cc:63: switch
(HeapObject::cast(*object)->map()->instance_type()) {
This is much slower than directly comparing against the heap constants (e.g.
isolate()->heap()->undefined_value() and friends).
Given that you will have to do 4 checks (true, false, null undefined) it might
still be faster to read out the maps' instance_type.

I definitely like the code better how it is currently, but just wanted to add
the for future reference once you start optimizing ;)

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h
File src/value-serializer.h (right):

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h#...
src/value-serializer.h:30: */
I wish all V8 classes were that nicely documented *sigh*  ;)

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h#...
src/value-serializer.h:55: template <typename T>
nit: Could you add a comment what WriteVarint does (or add inline comments in
the function definition.

[jbroman, 2016-08-12 14:27:23]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-08-12 14:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2232243003/#ps80001
(title: "Add comments explaining varints.")

[commit-bot: I haz the power, 2016-08-12 14:27:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 14:49:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-12 14:49:31]

Description was changed from

==========
Blink-compatible serialization of oddball values.

BUG=chromium:148757
==========

to

==========
Blink-compatible serialization of oddball values.

BUG=chromium:148757

Committed: https://crrev.com/e6d1a80e790117dc27b20e9b14de4b6ac9f7512f
Cr-Commit-Position: refs/heads/master@{#38625}
==========

[commit-bot: I haz the power, 2016-08-12 14:49:32]

Patchset 5 (id:??) landed as
https://crrev.com/e6d1a80e790117dc27b20e9b14de4b6ac9f7512f
Cr-Commit-Position: refs/heads/master@{#38625}

[jbroman, 2016-08-12 15:16:34]

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.cc...
src/value-serializer.cc:63: switch
(HeapObject::cast(*object)->map()->instance_type()) {
On 2016/08/12 at 09:54:44, Camillo Bruni wrote:
> This is much slower than directly comparing against the heap constants (e.g.
isolate()->heap()->undefined_value() and friends).
> Given that you will have to do 4 checks (true, false, null undefined) it might
still be faster to read out the maps' instance_type.
> 
> I definitely like the code better how it is currently, but just wanted to add
the for future reference once you start optimizing ;)

I'll be branching on the instance type of all objects anyhow, so I'm not sure
whether it'll be faster to check for oddballs first separately, or just have
them go through the same switch.

For what it's worth, this is how json-stringifier does it, and I've assumed that
it's fairly optimized already.
https://chromium.googlesource.com/v8/v8/+/bbd0a09/src/json-stringifier.cc#294

Nonetheless I'll definitely keep that in mind when I later try to
micro-optimize.

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h
File src/value-serializer.h (right):

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h#...
src/value-serializer.h:55: template <typename T>
On 2016/08/12 at 09:54:44, Camillo Bruni wrote:
> nit: Could you add a comment what WriteVarint does (or add inline comments in
the function definition.

Added comments inline, with a link to the protobuf encoding page (this, and
zigzag which is used for signed integers, are encoded the same way as in
protobuf, and they have a nice page that explains it clearly).

https://codereview.chromium.org/2232243003/diff/60001/src/value-serializer.h#...
src/value-serializer.h:55: template <typename T>
On 2016/08/12 at 09:54:44, Camillo Bruni wrote:
> nit: Could you add a comment what WriteVarint does (or add inline comments in
the function definition.

Added comments inline, with a link to the protobuf encoding page (this, and
zigzag which is used for signed integers, are encoded the same way as in
protobuf, and they have a nice page that explains it clearly).