[key-accumulator] Starting to reimplement the key-accumulator

[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}

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

Committed: https://crrev.com/065ae3dd5ee5d3d7fc6dc1311be57958b19e1d0d
Cr-Commit-Position: refs/heads/master@{#34558}

[Camillo Bruni, 2016-02-17 16:58:01]

cbruni@chromium.org changed reviewers:
+ verwaest@chromium.org

[Camillo Bruni, 2016-02-17 16:58:02]

PTAL
WIP, might make sense to land incrementally...

[Camillo Bruni, 2016-02-17 17:01:07]

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

[commit-bot: I haz the power, 2016-02-17 17:01:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707743002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707743002/1

[commit-bot: I haz the power, 2016-02-17 17:14:01]

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

[commit-bot: I haz the power, 2016-02-17 17:14:02]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[Camillo Bruni, 2016-02-24 13:40:56]

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

[commit-bot: I haz the power, 2016-02-24 13:40:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707743002/60001

[Camillo Bruni, 2016-02-24 13:43:04]

Description was changed from

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=
==========

to

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503
LOG=y
==========

[commit-bot: I haz the power, 2016-02-24 13:45:48]

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

[commit-bot: I haz the power, 2016-02-24 13:45:49]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/3390)
  v8_win_compile_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...)

[Toon Verwaest, 2016-02-24 15:07:07]

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc#newcode900
src/elements.cc:900: if (IsFastPackedElementsKind(kind())) {
HasElementImpl should just return true for packed kind(). Similarly, EnsureSize
should be a noop for packed kind(). In that case you don't need to duplicate the
branches.

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc#newcode936
src/elements.cc:936: if (!IsDictionaryElementsKind(kind()) && filter &
ONLY_ALL_CAN_READ) {
There are no ALL_CAN_READ indexed properties that aren't handled by an
all-can-read indexed interceptor. We should probably add a (D)CHECK to the API
that this never happens, rather than implement support here. Those properties
are only relevant for browsers and browser-like settings; and in chrome we don't
use it.

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc#newcode945
src/elements.cc:945: int initial_list_length = 10;
Why not immediately use Capacity + nof_property_keys, and then always trim
later? I doubt you'll be able to avoid much memory allocation in practice; you
probably just make it worse by using a list + expanding.

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc#newcode963
src/elements.cc:963: bool operator()(Object* a, Object* b) {
return a->Number()  < b->Number()? Seems like double-encoded indices aren't
going to be sorted otherwise ...

https://codereview.chromium.org/1707743002/diff/60001/src/elements.cc#newcode986
src/elements.cc:986: ElementsKind kind = FAST_HOLEY_ELEMENTS;
I guess it doesn't change anything, but this should be FAST_ELEMENTS right?

https://codereview.chromium.org/1707743002/diff/60001/src/key-accumulator.cc
File src/key-accumulator.cc (right):

https://codereview.chromium.org/1707743002/diff/60001/src/key-accumulator.cc#...
src/key-accumulator.cc:382: // TODO(cbruni): avoud HasEnumerableElements for
very large holey elements.
avoid

https://codereview.chromium.org/1707743002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1707743002/diff/60001/src/objects.cc#newcode8358
src/objects.cc:8358: if (!elements->is_the_hole(i)) return true;
is it really worthwhile to find the first non-hole element? Once you have a
fast-non-empty backing store, I'd just assume there might be one and fall to the
slow path. This precheck probably just makes the common case of the slow path
even more slow.

https://codereview.chromium.org/1707743002/diff/60001/src/objects.cc#newcode8487
src/objects.cc:8487: // stati
static

[Camillo Bruni, 2016-03-04 09:40:42]

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

[commit-bot: I haz the power, 2016-03-04 09:40:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707743002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707743002/120001

[commit-bot: I haz the power, 2016-03-04 09:45:44]

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

[commit-bot: I haz the power, 2016-03-04 09:45:45]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Camillo Bruni, 2016-03-04 13:47:50]

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

[commit-bot: I haz the power, 2016-03-04 13:47:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707743002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707743002/200001

[commit-bot: I haz the power, 2016-03-04 13:50:44]

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

[commit-bot: I haz the power, 2016-03-04 13:50:45]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/2374)

[Camillo Bruni, 2016-03-04 14:06:57]

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

[commit-bot: I haz the power, 2016-03-04 14:07:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707743002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707743002/220001

[commit-bot: I haz the power, 2016-03-04 14:31:09]

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

[commit-bot: I haz the power, 2016-03-04 14:31:10]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-03-04 15:33:36]

lgtm with minor comments

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc#newcode317
src/keys.cc:317: if (JSObject::cast(object)->HasEnumerableElements()) return
false;
What about just setting the length? Otherwise you need to recalculate later
anyway...

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc#newcode328
src/keys.cc:328: return !JSObject::cast(object)->HasEnumerableElements();
And in that case you can just

if (enum_length == invalid) TrySetting();
if (enum_length == 0) return !...HasEnumerableElements();

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc#newcode343
src/keys.cc:343: PrototypeIterator::START_AT_PROTOTYPE);
START_AT_PROTOTYPE is the default right?

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc#newcode354
src/keys.cc:354: if (receiver_->map()->EnumLength() !=
kInvalidEnumCacheSentinel) {
is_receiver_simple_enum == EnumLength() != kInvalid && !HasEnumerable

https://codereview.chromium.org/1707743002/diff/220001/src/keys.cc#newcode402
src/keys.cc:402: if (!own_only || receiver_->IsJSProxy() ||
JSProxy is already included in OnlyHasSimpleProperties now.

[Camillo Bruni, 2016-03-07 11:49:36]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-07 11:49:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1707743002/#ps260001
(title: "git cl format is too eager")

[commit-bot: I haz the power, 2016-03-07 11:49:41]

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

[Camillo Bruni, 2016-03-07 11:50:51]

Description was changed from

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503
LOG=y
==========

to

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y
==========

[commit-bot: I haz the power, 2016-03-07 12:16:11]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-03-07 12:16:34]

Description was changed from

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y
==========

to

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}
==========

[commit-bot: I haz the power, 2016-03-07 12:16:40]

Patchset 14 (id:??) landed as
https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}

[Camillo Bruni, 2016-03-07 12:44:06]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1773593003/ by cbruni@chromium.org.

The reason for reverting is: gcmole failure
https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/8598.

[Camillo Bruni, 2016-03-07 15:50:49]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-07 15:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1707743002/#ps280001
(title: "fixing gc mole issues")

[commit-bot: I haz the power, 2016-03-07 15:51:02]

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

[commit-bot: I haz the power, 2016-03-07 16:12:14]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-03-07 16:13:09]

Description was changed from

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}
==========

to

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}

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

[commit-bot: I haz the power, 2016-03-07 16:13:10]

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

[Camillo Bruni, 2016-03-07 16:35:58]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1769043003/ by cbruni@chromium.org.

The reason for reverting is: gcmole again.

[Camillo Bruni, 2016-03-07 16:40:44]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-07 16:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1707743002/#ps300001
(title: "be happy gcmole")

[commit-bot: I haz the power, 2016-03-07 16:40:57]

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

[commit-bot: I haz the power, 2016-03-07 16:43:26]

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

[commit-bot: I haz the power, 2016-03-07 16:43:28]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2477)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16598)

[Camillo Bruni, 2016-03-07 18:54:27]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-07 18:54:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1707743002/#ps320001
(title: "use proper type")

[commit-bot: I haz the power, 2016-03-07 18:54:38]

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

[commit-bot: I haz the power, 2016-03-07 19:25:18]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-03-07 19:25:59]

Description was changed from

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}

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

to

==========
[key-accumulator] Starting to reimplement the key-accumulator

Introducing the KeyAccumulator accidentally removed some crucial fast-paths.
This CL starts rewriting the KeyAccumulator, step-by-step introducing the
special cases again.

BUG=chromium:545503, v8:4758
LOG=y

Committed: https://crrev.com/9c61327ecb2ee41f34232632e0cac93202bae6b7
Cr-Commit-Position: refs/heads/master@{#34532}

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

Committed: https://crrev.com/065ae3dd5ee5d3d7fc6dc1311be57958b19e1d0d
Cr-Commit-Position: refs/heads/master@{#34558}
==========

[commit-bot: I haz the power, 2016-03-07 19:26:00]

Patchset 17 (id:??) landed as
https://crrev.com/065ae3dd5ee5d3d7fc6dc1311be57958b19e1d0d
Cr-Commit-Position: refs/heads/master@{#34558}