Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334353002:
Follow object map transitions when deserializing object properties. (Closed)
DescriptionFollow object map transitions when deserializing object properties.
Similar to json-parser.
BUG=chromium:148757
Committed: https://crrev.com/2a4699058725ebdb781d190d6d3ecdbcfd6b1343
Cr-Commit-Position: refs/heads/master@{#39429}
Patch Set 1 #Patch Set 2 : static_cast to avoid signed/unsigned mismatch #Patch Set 3 : static_cast #
Total comments: 4
Patch Set 4 : update unit tests per cbruni #
Messages
Total messages: 26 (20 generated)
The CQ bit was checked by jbroman@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: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/14180) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/14216)
The CQ bit was checked by jbroman@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: 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...)
The CQ bit was checked by jbroman@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: This issue passed the CQ dry run.
jbroman@chromium.org changed reviewers: + cbruni@chromium.org, jkummerow@chromium.org
This yield about 6% improvement on deserialization time of large JSON-esque data. It follows json-parser pretty quickly (but it doesn't seem trivial to extract out a simple common abstraction, aside from the stuff already in src/transitions.h).
lgtm with nits https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:680: // - no known transition nit: can you update the order of the comment, so it matches the test? 1. xyz (no known transition as this is the first time we create this kind of object) 2. xyz (full simple transition matches) 3. xyw (transition partially matches, w is not found, causing fallback) 4. xyz (transition matches, with xy being simple transition, and w a full transition) 5. xyw (same) https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:683: "\"w\":7},{\"x\":6,\"y\":7,\"z\":8},{\"x\":0,\"y\":0,\"w\":0}]"); late question: single quotes doesn't work for the properties here? nit: could you force newlines whenever a new object starts? This makes reading this way easier. There are two more cases I would like to have a test for, just in case :) nit: can you test the path of adding more properties after a complex transition succeeded (xywz) int: can you test the path of adding more properties after a complex transition wasn't found (zykz)
The CQ bit was checked by jbroman@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:680: // - no known transition On 2016/09/14 at 16:26:20, Camillo Bruni wrote: > nit: can you update the order of the comment, so it matches the test? > > 1. xyz (no known transition as this is the first time we create this kind of object) > 2. xyz (full simple transition matches) > 3. xyw (transition partially matches, w is not found, causing fallback) > 4. xyz (transition matches, with xy being simple transition, and w a full transition) > 5. xyw (same) Done. https://codereview.chromium.org/2334353002/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:683: "\"w\":7},{\"x\":6,\"y\":7,\"z\":8},{\"x\":0,\"y\":0,\"w\":0}]"); On 2016/09/14 at 16:26:20, Camillo Bruni wrote: > late question: single quotes doesn't work for the properties here? > nit: could you force newlines whenever a new object starts? This makes reading this way easier. This uses JSON::Parse/Stringify to compare the object before/after doing a serialization round trip (because there aren't many easy ways to do a deep comparison), so this needs to match the JSON output verbatim, and that means double quotes. But I can wrap more nicely than clang-format's default, and I've done that. > There are two more cases I would like to have a test for, just in case :) > nit: can you test the path of adding more properties after a complex transition succeeded (xywz) > int: can you test the path of adding more properties after a complex transition wasn't found (zykz) Done.
The CQ bit was checked by jbroman@chromium.org
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/2334353002/#ps60001 (title: "update unit tests per cbruni")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Follow object map transitions when deserializing object properties. Similar to json-parser. BUG=chromium:148757 ========== to ========== Follow object map transitions when deserializing object properties. Similar to json-parser. BUG=chromium:148757 Committed: https://crrev.com/2a4699058725ebdb781d190d6d3ecdbcfd6b1343 Cr-Commit-Position: refs/heads/master@{#39429} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2a4699058725ebdb781d190d6d3ecdbcfd6b1343 Cr-Commit-Position: refs/heads/master@{#39429} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
