ValueSerializer: Take advantage of fast elements in dense array serialization.

ValueSerializer: Take advantage of fast elements in dense array serialization.

This yields a ~5% serialization time improvement on typical JSON-esque data.
The approach taken matches json-stringifier fairly closely.

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

[jbroman, 2016-09-06 19:40:21]

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

[commit-bot: I haz the power, 2016-09-06 19:40:24]

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

[jbroman, 2016-09-06 19:50:30]

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

[commit-bot: I haz the power, 2016-09-06 20:06:14]

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

[commit-bot: I haz the power, 2016-09-06 20:06:15]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-09-07 02:28:08]

lgtm

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

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:454: array->HasFastElements() &&
!array->HasFastHoleyElements();
This might be a future optimization point by serializing holey elements in a
dense fashion, which would require a special marker object for the_hole value
and some way to detect this case during materialization.

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:466: switch (array->GetElementsKind()) {
With only 3 cases present here there is currently no hurry. However, in the case
we need more cases here I'd prefer moving this to the ElementsAccessor which
provides more machinery to deal with the peculiarities of all the elements.

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:508: !WriteObject(element).FromMaybe(false)) {
edge-case nit: in the case where the array length gets changed to something <
length and > i you will fail with an exception even though you could properly
finish serialization (I'd like to see a test for exactly this case)

[jbroman, 2016-09-07 14:53:39]

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

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:454: array->HasFastElements() &&
!array->HasFastHoleyElements();
On 2016/09/07 at 02:28:07, Camillo Bruni wrote:
> This might be a future optimization point by serializing holey elements in a
dense fashion, which would require a special marker object for the_hole value
and some way to detect this case during materialization.

Yeah, it's possible, though it takes a little care, because we have to store a
bitmask or equivalent of where the holes were to begin with, and not slow down
the non-holey path which is probably much more common. (Though it's interesting
to note that json-stringifier still doesn't have a fast path for holey
elements.)

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:466: switch (array->GetElementsKind()) {
On 2016/09/07 at 02:28:08, Camillo Bruni wrote:
> With only 3 cases present here there is currently no hurry. However, in the
case we need more cases here I'd prefer moving this to the ElementsAccessor
which provides more machinery to deal with the peculiarities of all the
elements.

I'm not sure what operation could be added to ElementsAccessor that would work
well here, though. Iteration, maybe, though it would imply some overhead (both
for abstraction and for virtual dispatch blocking inlining and subsequent
optimization).

https://codereview.chromium.org/2311063004/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:508: !WriteObject(element).FromMaybe(false)) {
On 2016/09/07 at 02:28:07, Camillo Bruni wrote:
> edge-case nit: in the case where the array length gets changed to something <
length and > i you will fail with an exception even though you could properly
finish serialization (I'd like to see a test for exactly this case)

I've added such a test case, but it passes already. This works as follows:
- falls out of fast path (array length changed)
- GetProperty(&it) succeeds, and returns undefined (it.WasFound() is false, but
isn't checked)

[jbroman, 2016-09-07 14:56:41]

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

[commit-bot: I haz the power, 2016-09-07 14:56:49]

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

[jbroman, 2016-09-07 14:59:42]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-09-07 14:59:43]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-09-07 14:59:44]

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/2311063004/#ps20001
(title: "additional unit tests")

[commit-bot: I haz the power, 2016-09-07 14:59:51]

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-09-07 15:23:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-07 15:23:39]

Description was changed from

==========
ValueSerializer: Take advantage of fast elements in dense array serialization.

This yields a ~5% serialization time improvement on typical JSON-esque data.
The approach taken matches json-stringifier fairly closely.

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

to

==========
ValueSerializer: Take advantage of fast elements in dense array serialization.

This yields a ~5% serialization time improvement on typical JSON-esque data.
The approach taken matches json-stringifier fairly closely.

BUG=chromium:148757

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

[commit-bot: I haz the power, 2016-09-07 15:23:40]

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