Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246093003:
Blink-compatible serialization of dictionary-like objects. (Closed)
DescriptionBlink-compatible serialization of dictionary-like objects.
As part of this CL, object reference tracking is implemented (and tested with a
self-referential object). This sort of reference tracking will be shared with
other receivers (array, date, regexp and host objects).
Not included in this CL is compatibility with version-0 objects (which don't
support a non-tree object graph, and require a little stack to correctly
deserialize).
BUG=chromium:148757
Committed: https://crrev.com/1031a79f6031d694efbd245515e814d61fe4d487
Cr-Commit-Position: refs/heads/master@{#38683}
Patch Set 1 #Patch Set 2 : suppress unused variable warning due to DCHECK #Patch Set 3 : cleanup and clang-format #
Total comments: 26
Patch Set 4 : bug fix, fixes requested by cbruni, more edge cases #Patch Set 5 : s/(void)/USE/ #Patch Set 6 : OptionalRescheduleException #
Total comments: 10
Patch Set 7 : nits #
Messages
Total messages: 37 (27 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_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...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
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.
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
https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:306: GlobalHandles::Destroy(Handle<Object>::cast(id_map_).location()); This seems to be how such handles are currently managed in v8::internal, though an internal handle equivalent to v8::Global would naturally be more convenient than explicitly writing the create/destroy invocations.
LGTM with nits. As I outlined in my mail, please send the follow-up CLs to jkummerow@ https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:246: auto instance_type = receiver->map()->instance_type(); nit: we don't use the auto type in V8 https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:248: return Nothing<bool>(); nit: missing braces https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:268: if (!KeyAccumulator::GetKeys(object, KeyCollectionMode::kOwnOnly, I'm sure you already had a look at JsonStringifier::SerializeJSObject, I'd presume you'll use the same structure having a FastVersion that bails out and falls back to this Slow path. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:272: return Nothing<bool>(); nit: missing braces https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:283: if (!WriteObject(key).FromMaybe(false)) return Nothing<bool>(); I don't know the details of the post message serialization format, but here you differ from the the JSON.stringify way of skipping properties that are set to undefined. So probably this is fine :) https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:306: GlobalHandles::Destroy(Handle<Object>::cast(id_map_).location()); If I see this correctly, I think you can get rid of the GlobalHandle if you make sure that you have an outer HandleScope. From what I've seen is that many API helper methods create one and then call into an internal V8 method. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:479: return ReadJSReceiver<JSObject>([this](uint32_t id) -> MaybeHandle<JSObject> { nit: I think V8 is not too fond of functors (call us old fashioned), could you rewrite this more explicitly (I think I'd be fine with some more duplication here). I consulted with jkummerow who's going to take over and he's not too fond of it either ;). Feel free to convince us though! https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:490: return MaybeHandle<JSObject>(); nit: missing braces https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:542: return MaybeHandle<JSReceiver>(); nit: missing braces https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:561: } nit: I'd drop the global handle (as I outlined above), they are quite costly in comparison to normal handles anyway. https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:411: // Integer key (treated as a string, but may be encoded differently). nit: can you add a copy of this test with the corner-case array indices that can no longer be encoded as Smis? The properties correspond to the enumeration index: {a: 2, 0xFFFFFFFE: 1, 1: 0} vs. {a: 1, 0xFFFFFFFF: 2, 1: 0} https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:439: }); future-nit: once you can handle accessors and custom prototypes serialize a self-modifying object (these are a the fun parts of JS!) {a: 0, get b() { delete this.c; return 1 }, c: 2} {a: 0, get b() { this.d = 4; return 1 }, c: 2} {a: 0, get b() { this.__proto__ = {d:4}; return 1 }, c: 2} {a: 0, get b() { delete this.c; this.__proto__ = {c:-2}; return 1 }, c: 2} To continue on this, what's the take on modifying enumerability on the fly? { a: 0, get b() { Object.defineProperty(this, 'c', {value:-2, enumerable: false}); return 1 }, c: 2}
jbroman@chromium.org changed reviewers: + jkummerow@chromium.org
jkummerow, PTAL. cbruni has already reviewed this, but I've now made significant enough changes in response to his feedback that I'd like another review. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:246: auto instance_type = receiver->map()->instance_type(); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: we don't use the auto type in V8 Done. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:268: if (!KeyAccumulator::GetKeys(object, KeyCollectionMode::kOwnOnly, On 2016/08/16 at 11:26:56, Camillo Bruni (OOO) wrote: > I'm sure you already had a look at JsonStringifier::SerializeJSObject, I'd presume you'll use the same structure having a FastVersion that bails out and falls back to this Slow path. I've definitely considered it, though I figured I'd get things working first. (Incidentally, I already expect a significant performance win even before evaluating such fast paths.) I'm not sure I understand the JSON implementation well enough to understand why it works. It looks like it should behave in ways I don't want (w.r.t. things messing with object structure during getters), but I haven't been able to actually cause that to happen. Is a new Map created whenever a JSObject's structure changes (addition/removal/reconfiguration of properties)? There's no comment on i::Map, but I'm having difficulty making sense of it otherwise. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:272: return Nothing<bool>(); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: missing braces Done. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:283: if (!WriteObject(key).FromMaybe(false)) return Nothing<bool>(); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > I don't know the details of the post message serialization format, but here you differ from the the JSON.stringify way of skipping properties that are set to undefined. > > So probably this is fine :) The structured clone algorithm reproduces properties with value "undefined" on the other side (unlike JSON, which doesn't have a representation of undefined). https://html.spec.whatwg.org/multipage/infrastructure.html#structuredclone https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:306: GlobalHandles::Destroy(Handle<Object>::cast(id_map_).location()); On 2016/08/16 at 11:26:56, Camillo Bruni (OOO) wrote: > If I see this correctly, I think you can get rid of the GlobalHandle if you make sure that you have an outer HandleScope. From what I've seen is that many API helper methods create one and then call into an internal V8 method. I couldn't figure out how to do this. We might need to enlarge the dictionary during some deep recursive call, and if I understand correctly, the handle that will be allocated then will be part of the current (deeper) HandleScope. I'd need to lift it (potentially up several levels) to a HandleScope that will survive until ~ValueDeserializer. The two ways I could think of to do that were to use a global handle, or to hold a handle on a heap object which itself contains a reference to the current dictionary (adding another level of indirection). I didn't see any code doing the latter, so I did the former. What's the right way to do this? https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:479: return ReadJSReceiver<JSObject>([this](uint32_t id) -> MaybeHandle<JSObject> { On 2016/08/16 at 11:26:56, Camillo Bruni (OOO) wrote: > nit: I think V8 is not too fond of functors (call us old fashioned), could you rewrite this more explicitly (I think I'd be fine with some more duplication here). I consulted with jkummerow who's going to take over and he's not too fond of it either ;). Feel free to convince us though! My only reservation is making sure that all JSReceivers do this properly. There are a few cases that will need to need the same boilerplate: object, host object, array, map, set, regexp, date, and the various JSValue wrappers. That said, the boilerplate isn't too bad and about half of those are leaves for which it's OK to skip the HandleScope and StackLimitCheck, so I'll just go ahead and inline it. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:490: return MaybeHandle<JSObject>(); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: missing braces Done. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:542: return MaybeHandle<JSReceiver>(); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: missing braces Done. The other cases you've pointed out had multi-line conditions, but this makes me wonder what the condition is that you're looking for braces in. https://github.com/v8/v8/wiki/Cpp%20Style%20Guide just points to the Chromium and Google style guides, both of which allow braces to be omitted if the body is one line. Can you point me where I can find the style rules I'm unaware of? https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:561: } On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: I'd drop the global handle (as I outlined above), they are quite costly in comparison to normal handles anyway. Replied above. Happy to eliminate it if there's a better solution, but just using plain handles here with no other changes doesn't work -- it results in accessing a handle outside its HandleScope. (That could possibly be resolved by making sure not to create inner HandleScopes, but deep recursion could lead to many open handles in that case.) https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:411: // Integer key (treated as a string, but may be encoded differently). On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > nit: can you add a copy of this test with the corner-case array indices that can no longer be encoded as Smis? > > The properties correspond to the enumeration index: > {a: 2, 0xFFFFFFFE: 1, 1: 0} > vs. > {a: 1, 0xFFFFFFFF: 2, 1: 0} I've incorporated such an example. It took me awhile to find the spec reference for why this behaves the way it does (2^31 - 1 is explicitly ruled out as being considered an index, though I don't understand why). https://codereview.chromium.org/2246093003/diff/40001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:439: }); On 2016/08/16 at 11:26:57, Camillo Bruni (OOO) wrote: > future-nit: once you can handle accessors and custom prototypes serialize a self-modifying object (these are a the fun parts of JS!) I can handle them now, aside from a bug which you caught. There's an infinite series of possible tests that could be had here, so it's hard to judge just how many to add. :) > {a: 0, get b() { delete this.c; return 1 }, c: 2} This case actually caught me forgetting to check that the property was still found (as an own property). It's a one-line check, but fixed. :) > {a: 0, get b() { this.d = 4; return 1 }, c: 2} > {a: 0, get b() { this.__proto__ = {d:4}; return 1 }, c: 2} > {a: 0, get b() { delete this.c; this.__proto__ = {c:-2}; return 1 }, c: 2} Added variants on these tests, and a few more. > To continue on this, what's the take on modifying enumerability on the fly? > { a: 0, get b() { Object.defineProperty(this, 'c', {value:-2, enumerable: false}); return 1 }, c: 2} The spec says that properties are enumerated once before all [[Get]]s are done, except for a check that the object still has that own property key before [[Get]].
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...
https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:268: if (!KeyAccumulator::GetKeys(object, KeyCollectionMode::kOwnOnly, On 2016/08/16 at 18:16:29, jbroman wrote: > Is a new Map created whenever a JSObject's structure changes (addition/removal/reconfiguration of properties)? There's no comment on i::Map, but I'm having difficulty making sense of it otherwise. Ah, this is partially answered by your comment on my followup CL: https://codereview.chromium.org/2248893003
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 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 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.
LGTM with nits. https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/40001/src/value-serializer.cc... src/value-serializer.cc:306: GlobalHandles::Destroy(Handle<Object>::cast(id_map_).location()); I think your understanding is correct, and GlobalHandles are indeed the way to solve this. https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.c... src/value-serializer.cc:248: return Nothing<bool>(); nit: braces are still missing. (V8 style differs from Chromium style there: multi-line if-statements always require braces, only all-on-one-line "if (foo) bar;" are allowed to skip them.) https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.h File src/value-serializer.h (right): https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.h... src/value-serializer.h:149: Handle<SeededNumberDictionary> id_map_; // always a global handle nit: Capitalization and punctuation please. https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:57: return Just(serializer.ReleaseBuffer()); nit: braces please https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:59: internal_isolate->OptionalRescheduleException(true); nit: braces please https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:448: // Indexes first, in order (but not 2^31 - 1, which is not an index), then the nit: you mean "2^32 - 1".
https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.c... src/value-serializer.cc:248: return Nothing<bool>(); On 2016/08/17 at 13:00:14, Jakob wrote: > nit: braces are still missing. > (V8 style differs from Chromium style there: multi-line if-statements always require braces, only all-on-one-line "if (foo) bar;" are allowed to skip them.) Done. https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.h File src/value-serializer.h (right): https://codereview.chromium.org/2246093003/diff/100001/src/value-serializer.h... src/value-serializer.h:149: Handle<SeededNumberDictionary> id_map_; // always a global handle On 2016/08/17 at 13:00:15, Jakob wrote: > nit: Capitalization and punctuation please. Done. https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:57: return Just(serializer.ReleaseBuffer()); On 2016/08/17 at 13:00:15, Jakob wrote: > nit: braces please Done. https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:59: internal_isolate->OptionalRescheduleException(true); On 2016/08/17 at 13:00:15, Jakob wrote: > nit: braces please Done. (I wish clang-format fixed this.) https://codereview.chromium.org/2246093003/diff/100001/test/unittests/value-s... test/unittests/value-serializer-unittest.cc:448: // Indexes first, in order (but not 2^31 - 1, which is not an index), then the On 2016/08/17 at 13:00:15, Jakob wrote: > nit: you mean "2^32 - 1". Indeed. 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, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2246093003/#ps120001 (title: "nits")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Blink-compatible serialization of dictionary-like objects. As part of this CL, object reference tracking is implemented (and tested with a self-referential object). This sort of reference tracking will be shared with other receivers (array, date, regexp and host objects). Not included in this CL is compatibility with version-0 objects (which don't support a non-tree object graph, and require a little stack to correctly deserialize). BUG=chromium:148757 ========== to ========== Blink-compatible serialization of dictionary-like objects. As part of this CL, object reference tracking is implemented (and tested with a self-referential object). This sort of reference tracking will be shared with other receivers (array, date, regexp and host objects). Not included in this CL is compatibility with version-0 objects (which don't support a non-tree object graph, and require a little stack to correctly deserialize). BUG=chromium:148757 Committed: https://crrev.com/1031a79f6031d694efbd245515e814d61fe4d487 Cr-Commit-Position: refs/heads/master@{#38683} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1031a79f6031d694efbd245515e814d61fe4d487 Cr-Commit-Position: refs/heads/master@{#38683} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
