[keys] Simplify KeyAccumulator

[keys] Simplify KeyAccumulator

- Use KeyAccumulator::GetKeys directly instead of JSReceiver::GetKeys
- Revert KeyAccumulator to single OrderedHashSet implementation.
- Convert the OrderedHashSet in-place to a FixedArray
- IndexedInterceptor indices are no longer combined and sorted with the object indices

BUG=

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

[Camillo Bruni, 2016-05-20 14:21:05]

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

[commit-bot: I haz the power, 2016-05-20 14:21:11]

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

[commit-bot: I haz the power, 2016-05-20 15:40:43]

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

[commit-bot: I haz the power, 2016-05-20 15:40:44]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/6104)
  v8_linux_dbg_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Camillo Bruni, 2016-05-23 08:51:23]

Description was changed from

==========
[keys] Simplify KeyAccumulator

- Revert KeyAccumulator to single OrderedHashSet implementation.
- Convert the OrderedHashSet in-place to a FixedArray

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

to

==========
[keys] Simplify KeyAccumulator

- Use KeyAccumulator::GetKeys directly instead of JSReceiver::GetKeys
- Revert KeyAccumulator to single OrderedHashSet implementation.
- Convert the OrderedHashSet in-place to a FixedArray
- IndexedInterceptor indices are no longer combined and sorted with the object
indices

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

[Camillo Bruni, 2016-05-23 13:32:32]

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

[Camillo Bruni, 2016-05-23 13:32:33]

PTAL

[Camillo Bruni, 2016-05-23 14:08:37]

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

[commit-bot: I haz the power, 2016-05-23 14:08:44]

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

[commit-bot: I haz the power, 2016-05-23 14:37:53]

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

[commit-bot: I haz the power, 2016-05-23 14:37:54]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2016-05-23 16:25:22]

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:449: bool emit_write_barrier = true) {
Consider using the existing enum WriteBarrierMode instead of a bool.

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:456: return !b->IsUndefined();
I'm pretty sure this condition is inverted, i.e. you want s/!//.

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1095: return dict->NumberOfElements();
In existing code, GetIterationLength is clearly expected to return the array
length, not the number of actually existing elements, and in the superclass
implementation in line 731ff this is still the case. Having an identically named
overridden method with different semantics seems dangerous.
Maybe "GetIterationLength" is just a too ambiguous name, and we should have
"GetArrayLength" and "GetCapacity"/"GetNumberOfElements" instead?
Or just inline it?

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode78
src/keys.cc:78: key = isolate_->factory()->NewHeapNumber(index);
Shouldn't this use NewNumberFromUint so Smis will be returned when possible?

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode451
src/keys.cc:451: accumulator->AddKeys(result, CONVERT_TO_ARRAY_INDEX);
nit: could shorten this further now:

accumulator->AddKeys(result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX
                                              : DO_NOT_CONVERT);

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode474
src/keys.cc:474: int CollectOwnStringPropertyNames(Handle<JSObject> object,
KeyAccumulator* keys,
nit: this name is confusing, the function can collect symbols too. Maybe just
"CollectOwnPropertyNamesInternal"?

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode511
src/keys.cc:511: // First Collect the Strings,
nit: s/Collect/collect/, s/Strings/strings/

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode514
src/keys.cc:514: // then the Symbols.
nit: s/Symbols/symbols/

https://codereview.chromium.org/1995263002/diff/140001/src/keys.h
File src/keys.h (right):

https://codereview.chromium.org/1995263002/diff/140001/src/keys.h#newcode39
src/keys.h:39: // Computes the enumerable keys for a JSObject. Used for
implementing
I think this comment is outdated (and overly restrictive). I'd just drop it; the
function's name is as descriptive as it gets here.

https://codereview.chromium.org/1995263002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/objects.cc#newcode...
src/objects.cc:17466: // From this point on keys_ is invalid.
What's keys_? This is a static method!

https://codereview.chromium.org/1995263002/diff/140001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:203: // No need to separate prototype levels since
we only get element keys.
outdated comment?

https://codereview.chromium.org/1995263002/diff/140001/test/mjsunit/es6/refle...
File test/mjsunit/es6/reflect.js (right):

https://codereview.chromium.org/1995263002/diff/140001/test/mjsunit/es6/refle...
test/mjsunit/es6/reflect.js:544: // Force dict-mode elements
nit: trailing full stop. Again below.

[Camillo Bruni, 2016-05-23 19:10:13]

PTAL again, addressed all nits.
The only one left for discussion is the GetIterationLength (see my inline
comment).

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:449: bool emit_write_barrier = true) {
On 2016/05/23 at 16:25:21, Jakob wrote:
> Consider using the existing enum WriteBarrierMode instead of a bool.

updated.

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:456: return !b->IsUndefined();
On 2016/05/23 at 16:25:21, Jakob wrote:
> I'm pretty sure this condition is inverted, i.e. you want s/!//.

good catch!
you're right, if b is smaller than a return false.

https://codereview.chromium.org/1995263002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1095: return dict->NumberOfElements();
On 2016/05/23 at 16:25:21, Jakob wrote:
> In existing code, GetIterationLength is clearly expected to return the array
length, not the number of actually existing elements, and in the superclass
implementation in line 731ff this is still the case. Having an identically named
overridden method with different semantics seems dangerous.
> Maybe "GetIterationLength" is just a too ambiguous name, and we should have
"GetArrayLength" and "GetCapacity"/"GetNumberOfElements" instead?
> Or just inline it?

Renamed to GetMaxNumberOfElements, though the rest of the code stays the same.
In all the cases where we could get a dictionary we do the indirection via
Get/HasEntry which makes this method work in both cases.

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode78
src/keys.cc:78: key = isolate_->factory()->NewHeapNumber(index);
On 2016/05/23 at 16:25:21, Jakob wrote:
> Shouldn't this use NewNumberFromUint so Smis will be returned when possible?

indeed.

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode451
src/keys.cc:451: accumulator->AddKeys(result, CONVERT_TO_ARRAY_INDEX);
On 2016/05/23 at 16:25:21, Jakob wrote:
> nit: could shorten this further now:
> 
> accumulator->AddKeys(result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX
>                                               : DO_NOT_CONVERT);

done.

https://codereview.chromium.org/1995263002/diff/140001/src/keys.cc#newcode474
src/keys.cc:474: int CollectOwnStringPropertyNames(Handle<JSObject> object,
KeyAccumulator* keys,
On 2016/05/23 at 16:25:21, Jakob wrote:
> nit: this name is confusing, the function can collect symbols too. Maybe just
"CollectOwnPropertyNamesInternal"?

changed.

https://codereview.chromium.org/1995263002/diff/140001/src/keys.h
File src/keys.h (right):

https://codereview.chromium.org/1995263002/diff/140001/src/keys.h#newcode39
src/keys.h:39: // Computes the enumerable keys for a JSObject. Used for
implementing
On 2016/05/23 at 16:25:21, Jakob wrote:
> I think this comment is outdated (and overly restrictive). I'd just drop it;
the function's name is as descriptive as it gets here.

removed.

https://codereview.chromium.org/1995263002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1995263002/diff/140001/src/objects.cc#newcode...
src/objects.cc:17466: // From this point on keys_ is invalid.
On 2016/05/23 at 16:25:22, Jakob wrote:
> What's keys_? This is a static method!

updated and moved the comment.

[Jakob Kummerow, 2016-05-24 12:13:10]

LGTM, but as discussed offline, I'd be happier if you split what's now
GetMaxNumberOfElements into two distinct functions:
- one that returns the maximum element *index* (a.k.a. array length)
- one that returns the maximum *entry*, on in other words an estimate of the
number of elements that are present. When an accessor knows what kind of
elements it's operating on, this is trivial and can be inlined.

Otherwise code that pretends to be organized into reusable modules isn't
actually reusable because the same function implemented by different
ElementsAccessors has different semantics.

[Camillo Bruni, 2016-05-24 13:16:40]

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

[Toon Verwaest, 2016-05-24 13:17:36]

src/debug lgtm

[Camillo Bruni, 2016-05-24 13:17:43]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-24 13:17:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1995263002/#ps180001
(title: "addressing comments")

[commit-bot: I haz the power, 2016-05-24 13:17:48]

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

[Jakob Kummerow, 2016-05-24 13:19:58]

https://codereview.chromium.org/1995263002/diff/180001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1995263002/diff/180001/src/elements.cc#newcod...
src/elements.cc:1099: DCHECK(JSArray::cast(receiver)->length()->IsSmi());
Nope! Dictionary arrays can have heap number lengths (see original
implementation of this function that you've ripped out).

[Jakob Kummerow, 2016-05-24 13:20:14]

The CQ bit was unchecked by jkummerow@chromium.org

[Camillo Bruni, 2016-05-24 13:33:22]

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

[commit-bot: I haz the power, 2016-05-24 13:33:29]

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

[commit-bot: I haz the power, 2016-05-24 14:04:26]

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

[commit-bot: I haz the power, 2016-05-24 14:04:27]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-05-24 14:23:28]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-24 14:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1995263002/#ps200001
(title: "mark GetMaxIndex as UNREACHABLE for dictionaries")

[commit-bot: I haz the power, 2016-05-24 14:23:35]

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

[commit-bot: I haz the power, 2016-05-24 14:25:46]

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

[commit-bot: I haz the power, 2016-05-24 14:25:47]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/15890)

[Camillo Bruni, 2016-05-24 14:53:08]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-24 14:53:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1995263002/#ps220001
(title: "rebase master")

[commit-bot: I haz the power, 2016-05-24 14:53:16]

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

[commit-bot: I haz the power, 2016-05-24 14:56:06]

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

[commit-bot: I haz the power, 2016-05-24 14:56:07]

Try jobs failed on following builders:
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/2155)

[Camillo Bruni, 2016-05-24 15:07:29]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-24 15:07:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1995263002/#ps240001
(title: "rebase master for real")

[commit-bot: I haz the power, 2016-05-24 15:07:37]

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

[commit-bot: I haz the power, 2016-05-24 15:25:46]

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

[commit-bot: I haz the power, 2016-05-24 15:25:47]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Camillo Bruni, 2016-05-24 16:06:36]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-24 16:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1995263002/#ps280001
(title: "fixing wrong merge artifact")

[commit-bot: I haz the power, 2016-05-24 16:06:44]

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

[commit-bot: I haz the power, 2016-05-24 16:40:18]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-05-24 16:41:24]

Description was changed from

==========
[keys] Simplify KeyAccumulator

- Use KeyAccumulator::GetKeys directly instead of JSReceiver::GetKeys
- Revert KeyAccumulator to single OrderedHashSet implementation.
- Convert the OrderedHashSet in-place to a FixedArray
- IndexedInterceptor indices are no longer combined and sorted with the object
indices

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

to

==========
[keys] Simplify KeyAccumulator

- Use KeyAccumulator::GetKeys directly instead of JSReceiver::GetKeys
- Revert KeyAccumulator to single OrderedHashSet implementation.
- Convert the OrderedHashSet in-place to a FixedArray
- IndexedInterceptor indices are no longer combined and sorted with the object
indices

BUG=

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

[commit-bot: I haz the power, 2016-05-24 16:41:26]

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

[Michael Achenbach, 2016-05-24 17:35:35]

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

The reason for reverting is:
https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds....