Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304563004:
Take advantage of fast properties in ValueSerializer when JSObject has them. (Closed)
DescriptionTake advantage of fast properties in ValueSerializer when JSObject has them.
This yields a ~20% serialization time improvement on typical JSON-esque data.
BUG=chromium:148757
Committed: https://crrev.com/11f74547f8eba199bbbb2dc46213bc16ced47cf5
Cr-Commit-Position: refs/heads/master@{#39221}
Patch Set 1 #
Total comments: 9
Patch Set 2 : cbruni comments #
Total comments: 4
Patch Set 3 : cbruni review #
Messages
Total messages: 25 (16 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: This issue passed the CQ dry run.
jbroman@chromium.org changed reviewers: + jkummerow@chromium.org
First fast path. Pretty close match to how json-stringifier does it.
cbruni@chromium.org changed reviewers: + cbruni@chromium.org
back from holidays for a couple of days :) https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:378: DCHECK(object->map()->instance_type() > LAST_CUSTOM_ELEMENTS_RECEIVER); nit: Use DCHECK_GT(left..., right..) https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:380: object->HasFastProperties() && object->elements()->length() == 0; I guess this is WIP, elements() can be easily serialized if they have no accessors (there's object->GetElementsAccessor()->HasAccessors()) And you may want to offload serialization of elements to the ElementsAccessor (in elements.cc) as well. https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:407: isolate_, object, key, &success, LookupIterator::OWN); You can bypass the arrayIndex check in PropertyOrElement and the superfluous ToName conversion (you can only have internalized strings in the descriptor array) by directly instantiating the LookupIterator: LookupIterator(object, key, LookupIterator::OWN) If you feel courageous maybe add a new constructor to the LookupIterator which takes the isolate as first argument (currently that only exists for element lookups). https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:413: if (!it.IsFound()) continue; You can push the IsFound() check above the GetProperty call.
https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:380: object->HasFastProperties() && object->elements()->length() == 0; On 2016/09/01 20:50:32, Camillo Bruni wrote: > I guess this is WIP, elements() can be easily serialized if they have no > accessors (there's object->GetElementsAccessor()->HasAccessors()) > And you may want to offload serialization of elements to the ElementsAccessor > (in elements.cc) as well. It's not that easy. Any element that's an object itself could in turn have getters with arbitrary side effects. Skipping non-empty elements here is probably necessary; it would be rather involved to get the behavior correct and safe otherwise.
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/2304563004/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:378: DCHECK(object->map()->instance_type() > LAST_CUSTOM_ELEMENTS_RECEIVER); On 2016/09/01 at 20:50:32, Camillo Bruni wrote: > nit: Use DCHECK_GT(left..., right..) Done. https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:380: object->HasFastProperties() && object->elements()->length() == 0; On 2016/09/02 at 10:44:23, Jakob wrote: > On 2016/09/01 20:50:32, Camillo Bruni wrote: > > I guess this is WIP, elements() can be easily serialized if they have no > > accessors (there's object->GetElementsAccessor()->HasAccessors()) > > And you may want to offload serialization of elements to the ElementsAccessor > > (in elements.cc) as well. > > It's not that easy. Any element that's an object itself could in turn have getters with arbitrary side effects. Skipping non-empty elements here is probably necessary; it would be rather involved to get the behavior correct and safe otherwise. +1. And this matches what json-stringifier does (which seems reasonable; I'd expect that most objects have no own elements and most arrays have no own properties). https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:407: isolate_, object, key, &success, LookupIterator::OWN); On 2016/09/01 at 20:50:32, Camillo Bruni wrote: > You can bypass the arrayIndex check in PropertyOrElement and the superfluous ToName conversion (you can only have internalized strings in the descriptor array) by directly instantiating the LookupIterator: > > LookupIterator(object, key, LookupIterator::OWN) > > If you feel courageous maybe add a new constructor to the LookupIterator which takes the isolate as first argument (currently that only exists for element lookups). Done. https://codereview.chromium.org/2304563004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:413: if (!it.IsFound()) continue; On 2016/09/01 at 20:50:32, Camillo Bruni wrote: > You can push the IsFound() check above the GetProperty call. Done.
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/2304563004/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2304563004/diff/20001/src/value-serializer.cc... src/value-serializer.cc:385: DCHECK(!object->HasNamedInterceptor()); nit: you can replace the DCHECKS above with DCHECK_GT(object->instance_type(), LAST_SPECIAL_RECEIVER_TYPE) (a bit obsure... we should probably have a helper method). https://codereview.chromium.org/2304563004/diff/20001/src/value-serializer.cc... src/value-serializer.cc:399: if (V8_LIKELY(details.type() == DATA && *map == object->map())) { performance-nit: you may want to use a bool stable var which you only update the first time on the slow-path. see https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-object.cc?sq=pa...
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/2304563004/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2304563004/diff/20001/src/value-serializer.cc... src/value-serializer.cc:385: DCHECK(!object->HasNamedInterceptor()); On 2016/09/05 at 16:49:41, Camillo Bruni wrote: > nit: you can replace the DCHECKS above with DCHECK_GT(object->instance_type(), LAST_SPECIAL_RECEIVER_TYPE) (a bit obsure... we should probably have a helper method). Removed, then. Such a check already occurs at the top of this function, and LAST_CUSTOM_ELEMENTS_RECEIVER > LAST_SPECIAL_RECEIVER_TYPE. https://codereview.chromium.org/2304563004/diff/20001/src/value-serializer.cc... src/value-serializer.cc:399: if (V8_LIKELY(details.type() == DATA && *map == object->map())) { On 2016/09/05 at 16:49:41, Camillo Bruni wrote: > performance-nit: you may want to use a bool stable var which you only update the first time on the slow-path. > see https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-object.cc?sq=pa... OK, done. (As above, I was matching json-stringifier. I suspect the performance difference is small, though, since most object don't change shape during serialization, and so we'll still have to do the comparison each time.)
The CQ bit was unchecked by jbroman@chromium.org
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/2304563004/#ps40001 (title: "cbruni review")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Take advantage of fast properties in ValueSerializer when JSObject has them. This yields a ~20% serialization time improvement on typical JSON-esque data. BUG=chromium:148757 ========== to ========== Take advantage of fast properties in ValueSerializer when JSObject has them. This yields a ~20% serialization time improvement on typical JSON-esque data. BUG=chromium:148757 Committed: https://crrev.com/11f74547f8eba199bbbb2dc46213bc16ced47cf5 Cr-Commit-Position: refs/heads/master@{#39221} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/11f74547f8eba199bbbb2dc46213bc16ced47cf5 Cr-Commit-Position: refs/heads/master@{#39221} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
