[esnext] handle elements in FastObjectValuesOrEntries()

[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N

Committed: https://crrev.com/f5b85cb74c149c008ed1edd56870fd4218350a4d
Cr-Commit-Position: refs/heads/master@{#35081}

[caitp (gmail), 2016-03-14 19:37:54]

PTAL --- the ForEachElements thing didn't work out, so it's hard to share this
code, but I think it can still be worth taking the fast case sometimes
(particularly for FAST_*_ELEMENTS and DICTIONARY_ELEMENTS).

It might be worth removing the other element kind handlers, to save space.

[caitp (gmail), 2016-03-15 01:21:25]

Description was changed from

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N
==========

to

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N
==========

[caitp (gmail), 2016-03-15 01:21:25]

caitpotter88@gmail.com changed reviewers:
+ cbruni@chromium.org, verwaest@chromium.org

[Camillo Bruni, 2016-03-15 13:36:14]

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8750
src/objects.cc:8750: isolate->factory()->NewUninitializedFixedArray(2);
That's unsafe here, value is still a raw pointer.

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8750
src/objects.cc:8750: isolate->factory()->NewUninitializedFixedArray(2);
That's unsafe here, value is still a raw pointer.

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8764
src/objects.cc:8764: isolate->factory()->NewUninitializedFixedArray(2);
same here.

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8794
src/objects.cc:8794: ElementsKind kind = object->GetElementsKind();
I would feel more comfortable if this where in elements.cc :)

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8800
src/objects.cc:8800: DisallowHeapAllocation no_allocation;
again doens't hold... MakeEntryPair allocates an array, potentially triggering a
GC.
Same for all the loops further down.

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8847
src/objects.cc:8847: if (is_slow_elements) {
I think this would be automatically cleaner when pushed to elements.cc.
Also I think you can bail out to the slow path at this point... did you you
measure a difference?

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8867
src/objects.cc:8867: stable = object->map() == *map;
That's not used anywhere...

[caitp (gmail), 2016-03-15 15:55:09]

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8794
src/objects.cc:8794: ElementsKind kind = object->GetElementsKind();
On 2016/03/15 13:36:14, cbruni wrote:
> I would feel more comfortable if this where in elements.cc :)

I've moved it

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8847
src/objects.cc:8847: if (is_slow_elements) {
On 2016/03/15 13:36:14, cbruni wrote:
> I think this would be automatically cleaner when pushed to elements.cc.
> Also I think you can bail out to the slow path at this point... did you you
> measure a difference?

The fast elements case definitely makes a big difference --- I'll measure
DICT_ELEMENTS to see if it's really worth it there

https://codereview.chromium.org/1767113004/diff/20001/src/objects.cc#newcode8867
src/objects.cc:8867: stable = object->map() == *map;
On 2016/03/15 13:36:14, cbruni wrote:
> That's not used anywhere...

It's used when walking instance_descriptors() below

[Camillo Bruni, 2016-03-15 17:59:52]

some more comments... will do another thorough pass tomorrow.

https://codereview.chromium.org/1767113004/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1767113004/diff/40001/src/elements.cc#newcode...
src/elements.cc:1841: return Just(true);
I think you can combine the FixedArray and FixedDoubleArray version by using the
BackingStore typdef for the array type if you push this up to
FastElementsAccessor.

[caitp (gmail), 2016-03-15 18:24:30]

https://codereview.chromium.org/1767113004/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1767113004/diff/40001/src/elements.cc#newcode...
src/elements.cc:1841: return Just(true);
On 2016/03/15 17:59:52, cbruni wrote:
> I think you can combine the FixedArray and FixedDoubleArray version by using
the
> BackingStore typdef for the array type if you push this up to
> FastElementsAccessor.

Done

[Camillo Bruni, 2016-03-16 16:04:03]

lgtm with some more nits

https://codereview.chromium.org/1767113004/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1767113004/diff/100001/src/objects.cc#newcode...
src/objects.cc:8758: int number_of_own_elements =
object->NumberOfOwnElements(ALL_PROPERTIES);
This is a fairly costly operation for HOlEY elements.
I had the same issue in PrependElementIndices, so you're probably faster by
using ElementsAccessor::GetCapacity for this (which will be a slight
over-estimation).

[caitp (gmail), 2016-03-17 13:28:41]

https://codereview.chromium.org/1767113004/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1767113004/diff/100001/src/objects.cc#newcode...
src/objects.cc:8758: int number_of_own_elements =
object->NumberOfOwnElements(ALL_PROPERTIES);
On 2016/03/16 16:04:02, cbruni wrote:
> This is a fairly costly operation for HOlEY elements.
> I had the same issue in PrependElementIndices, so you're probably faster by
> using ElementsAccessor::GetCapacity for this (which will be a slight
> over-estimation).

Done

[caitp (gmail), 2016-03-28 15:28:50]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-28 15:28:50]

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/1767113004/#ps140001
(title: "Make ElementsAccessor::GetCapacity() public, and use it")

[commit-bot: I haz the power, 2016-03-28 15:29:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1767113004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1767113004/140001

[commit-bot: I haz the power, 2016-03-28 15:53:23]

Description was changed from

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N
==========

to

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N
==========

[commit-bot: I haz the power, 2016-03-28 15:53:24]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-28 15:54:30]

Description was changed from

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N
==========

to

==========
[esnext] handle elements in FastObjectValuesOrEntries()

Add support for optimizing objects with elements, which do not invoke JS and
cannot change the shape of the Object.

BUG=v8:4663
LOG=N

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

[commit-bot: I haz the power, 2016-03-28 15:54:31]

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