Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248893003:
Blink-compatible deserialization of old object format. (Closed)
DescriptionBlink-compatible deserialization of old object format.
The "version 0" format did not deal with references, and used a stack model to
deserialize objects (conceptually, a postorder tree traversal). This requires
an explicit stack, so special logic is added to decode this format.
All subsequent versions also put an object marker at the beginning, which is
equivalent to how the current version serializes.
BUG=chromium:148757
Committed: https://crrev.com/058a7ee01e6d1842fe284464fc390de5948ba75b
Cr-Commit-Position: refs/heads/master@{#38686}
Patch Set 1 #Patch Set 2 : correct to size_t #
Total comments: 16
Patch Set 3 : review comments, format #Patch Set 4 : Merge branch 'vs4' into vs5 #
Messages
Total messages: 24 (17 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...)
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
The very oldest format version ("version 0") requires some special handling;
here is code to support using its stack-based format. Version 0 predates
structured cloning supporting any kind of reference matching.
LGTM with nits https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:333: (void)actual_tag; nit: you can use our very pretty USE macro here :) https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:571: isolate->factory()->NewJSObject(isolate->object_function()); Performance Hint: This is a rather slow (but obviously correct) way to create a new object. Depending on how far down the optimization road you will go for future versions of the serialization format, this function is going to be crucial :). Not sure how familiar you are with all the internals of V8, so I apologize upfront for the detailed comment here. Objects have several backing stores for properties, the main distinction lies between array-indices which are stored in elements() and all the other properties which we usually just call properties. Inside there we have several distinctions. For elements we optimize depending on the keys we use and on the objects stored (you'll find all the implementation details in elements.cc). First of all, if we have Smi-ranged keys we're using fast-elements, if we have large keys and/or accessors for indices we resort to a dictionary elements kind (= slow elements). In the fast case we distinguish between packed and non packed depending on whether they contain a hole value or not. A hole appears when you use `delete arrayish[i]`. For properties (aka. non-indices keys) we have first have fast-in-object properties, which are, very much according to their name, directly in the object and typically used to store properties that need fast access (currently used on a first-come basis). Then we have the properties store which can either be fast (just a list) or slow (a dictionary). We resort to slow-properties (not related to slow-elements) when we have too many properties, if we have accessors and if you delete a property (based on the assumption that the object will be used as a dictionary henceforth). For fast properties we keep track of the object structure by means of hidden classes which are called (confusingly) Maps. This means whenever you add a new property to an object we transition the object internally to a new Map/hidden class (see Object::SetDataProperty). However, we only do this for fast properties (so not for element indices and not for slow/dictionary properties) In the context of serialization, you may want to keep track of the original property representation, especially for the elements (aka. it's ELEMENTS_KIND). This will be important to make sure you can quickly materialize objects without transitioning the backing stores from the uninitialized state via some intermediate version to the final version. For instance, each property added might require the properties store (list for fast-properties, dictionary for slow-properties) to be copied over. So simply making sure we reserve enough space upfront for properties and elements can save quite some work here (of course ideally you want to directly start off with the right elements-kind and right property type). Getting the right elements kind and size should be rather easy to achieve as you could simply encode this in the serialization format, or even deduce it from the keys you've seen. In this case you would simply transition to the new elements kind from the pristine object (note this will do a Map transition under the hood). Getting the properties right might be trickier, since for every fast property added we need the matching Map. So as a first step I simply suggest making sure that the object is set to dictionary mode (aka. slow properties) so we do not have to generate and check the maps along-side the property addition. This comes at the cost of optimizations being disabled where the new object is being used. Many optimizations require a stable object structure which can only be ensured with a precise Map attached to it, which is not the case for dict-mode objects. SUMMARY: - for now-ish (optimiztion phase) use JSObject::NormalizeProperties to reserve enough space for new properties and resort to cheap to add dictionary properties if there are too many properties (see Map::TooManyFastProperties) - check how JsonParser::ParseJsonObject shortcuts some of the transition logic (which would work if you had an JSObjectStart tag or so) https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:580: return MaybeHandle<JSObject>(); nit: please add braces here, we don't allow trailing statement after an if statement (unless on the same line). Could you check the other places in this file as well? They slipped through my last reviews... https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:605: return MaybeHandle<Object>(); nit: missing braces https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:612: return MaybeHandle<Object>(); nit: braces https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:627: DCHECK(*position_++ == static_cast<uint8_t>(SerializationTag::kPadding)); nit: braces https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:538: ASSERT_TRUE(value->IsObject()); nit: Can you add the prototype check from above as well? https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:562: } nit: could you add a simple test with properties and elements interleaved (yes, not much different than the existing ones :P)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:333: (void)actual_tag; On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: you can use our very pretty USE macro here :) I was looking for one, but didn't find it. Thanks. :) https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:571: isolate->factory()->NewJSObject(isolate->object_function()); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > Performance Hint: This is a rather slow (but obviously correct) way to create a new object. Depending on how far down the optimization road you will go for future versions of the serialization format, this function is going to be crucial :). > > Not sure how familiar you are with all the internals of V8, so I apologize upfront for the detailed comment here. No problem. I've learned a lot the last couple weeks, but there's a lot that I haven't yet learned. > Objects have several backing stores for properties, the main distinction lies between array-indices which are stored in elements() and all the other properties which we usually just call properties. Inside there we have several distinctions. > > > For elements we optimize depending on the keys we use and on the objects stored (you'll find all the implementation details in elements.cc). First of all, if we have Smi-ranged keys we're using fast-elements, if we have large keys and/or accessors for indices we resort to a dictionary elements kind (= slow elements). In the fast case we distinguish between packed and non packed depending on whether they contain a hole value or not. A hole appears when you use `delete arrayish[i]`. I saw that. AFAICT holes look the same as undefined to script, but I gather there's some subtlety here I'm missing. > For properties (aka. non-indices keys) we have first have fast-in-object properties, which are, very much according to their name, directly in the object and typically used to store properties that need fast access (currently used on a first-come basis). Then we have the properties store which can either be fast (just a list) or slow (a dictionary). We resort to slow-properties (not related to slow-elements) when we have too many properties, if we have accessors and if you delete a property (based on the assumption that the object will be used as a dictionary henceforth). > > For fast properties we keep track of the object structure by means of hidden classes which are called (confusingly) Maps. This means whenever you add a new property to an object we transition the object internally to a new Map/hidden class (see Object::SetDataProperty). However, we only do this for fast properties (so not for element indices and not for slow/dictionary properties) > > In the context of serialization, you may want to keep track of the original property representation, especially for the elements (aka. it's ELEMENTS_KIND). This will be important to make sure you can quickly materialize objects without transitioning the backing stores from the uninitialized state via some intermediate version to the final version. For instance, each property added might require the properties store (list for fast-properties, dictionary for slow-properties) to be copied over. So simply making sure we reserve enough space upfront for properties and elements can save quite some work here (of course ideally you want to directly start off with the right elements-kind and right property type). > > Getting the right elements kind and size should be rather easy to achieve as you could simply encode this in the serialization format, or even deduce it from the keys you've seen. In this case you would simply transition to the new elements kind from the pristine object (note this will do a Map transition under the hood). > > Getting the properties right might be trickier, since for every fast property added we need the matching Map. So as a first step I simply suggest making sure that the object is set to dictionary mode (aka. slow properties) so we do not have to generate and check the maps along-side the property addition. This comes at the cost of optimizations being disabled where the new object is being used. Many optimizations require a stable object structure which can only be ensured with a precise Map attached to it, which is not the case for dict-mode objects. > > SUMMARY: > - for now-ish (optimiztion phase) use JSObject::NormalizeProperties to reserve enough space for new properties and resort to cheap to add dictionary properties if there are too many properties (see Map::TooManyFastProperties) > - check how JsonParser::ParseJsonObject shortcuts some of the transition logic (which would work if you had an JSObjectStart tag or so) This CL (as opposed to the other one) deals with "version 0" objects, which Chrome has not written for over four years (but which Blink must still support because someone may have one stored in IndexedDB). I'd rather bias this code toward simplicity and focus optimization work on the path that's likely to be used much more in practice (especially for postMessage, which will always use the latest version). From what I understand, the JSON parser code does do map transitions the same way that DefineOwnPropertyIgnoreAttributes does, except that it has a fast path for the case where a simple transition is available. Versions afterwards do have a "begin object" tag (this is needed so that references to the object in its properties can be correctly resolved), though they don't currently write down any more information. I could imagine storing some sort of hint about whether fast or dictionary objects were likely to be useful (to avoid doing the transitions if we'll fall back to dictionary properties anyhow), but that seems a way down the optimization road to me (and obviously doesn't exist in currently stored IndexedDB data). https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:580: return MaybeHandle<JSObject>(); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: please add braces here, we don't allow trailing statement after an if statement (unless on the same line). > Could you check the other places in this file as well? They slipped through my last reviews... OK. As I mentioned in the other review, I couldn't find a reference to this in a style guide, so I'm not sure what other rules I could be missing. I don't want to waste reviewers' time. :) https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:605: return MaybeHandle<Object>(); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: missing braces Done. https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:612: return MaybeHandle<Object>(); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: braces Done. https://codereview.chromium.org/2248893003/diff/20001/src/value-serializer.cc... src/value-serializer.cc:627: DCHECK(*position_++ == static_cast<uint8_t>(SerializationTag::kPadding)); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: braces Done. https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:538: ASSERT_TRUE(value->IsObject()); On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: Can you add the prototype check from above as well? Done here, though in general I'd prefer to avoid duplicating checks (even though, of course, every object should have Object.prototype as its prototype). https://codereview.chromium.org/2248893003/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:562: } On 2016/08/16 at 09:04:07, Camillo Bruni (OOO) wrote: > nit: could you add a simple test with properties and elements interleaved (yes, not much different than the existing ones :P) OK, I'll do one with one property and one element.
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.
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
lgtm
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/2248893003/#ps60001 (title: "Merge branch 'vs4' into vs5")
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 ========== Blink-compatible deserialization of old object format. The "version 0" format did not deal with references, and used a stack model to deserialize objects (conceptually, a postorder tree traversal). This requires an explicit stack, so special logic is added to decode this format. All subsequent versions also put an object marker at the beginning, which is equivalent to how the current version serializes. BUG=chromium:148757 ========== to ========== Blink-compatible deserialization of old object format. The "version 0" format did not deal with references, and used a stack model to deserialize objects (conceptually, a postorder tree traversal). This requires an explicit stack, so special logic is added to decode this format. All subsequent versions also put an object marker at the beginning, which is equivalent to how the current version serializes. BUG=chromium:148757 Committed: https://crrev.com/058a7ee01e6d1842fe284464fc390de5948ba75b Cr-Commit-Position: refs/heads/master@{#38686} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/058a7ee01e6d1842fe284464fc390de5948ba75b Cr-Commit-Position: refs/heads/master@{#38686} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
