Reimplement Maps and Sets in JS

Reimplement Maps and Sets in JS

Previously, the only optimized code path for Maps and Sets was for String keys.
This was achieved through an implementation of various complex operations
in Hydrogen. This approach was neither scalable nor forward-compatible.

This patch adds the necessary intrinsics to implement Maps and Sets almost entirely
in JS. The added intrinsics are:

  %_FixedArrayGet
  %_FixedArraySet
  %_TheHole
  %_JSCollectionGetTable
  %_StringGetRawHashField

With these additions, as well as a few changes to what's exposed as runtime functions,
most of the C++ code backing Maps and Sets is gone (including both runtime code in
objects.cc and Crankshaft in hydrogen.cc).

Committed: https://crrev.com/909500aa1db9789b68e101045a6359a7fcb30e83
Cr-Commit-Position: refs/heads/master@{#27605}

[arv (Not doing code reviews), 2015-02-21 16:20:14]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-02-21 16:20:16]

Nice.

https://codereview.chromium.org/947683002/diff/20001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/20001/src/collection.js#newcode40
src/collection.js:40: if (keyIsNaN && IS_NUMBER(candidate) &&
NUMBER_IS_NAN(candidate)) {
Nice optimization to get around the NaN SameValueZero inefficiency.

https://codereview.chromium.org/947683002/diff/20001/src/collection.js#newcode40
src/collection.js:40: if (keyIsNaN && IS_NUMBER(candidate) &&
NUMBER_IS_NAN(candidate)) {
Nice optimization to get around the NaN SameValueZero inefficiency.

https://codereview.chromium.org/947683002/diff/20001/src/collection.js#newcod...
src/collection.js:218: while (%SetIteratorNext(iterator, value_array)) {
This one can get much faster now that we have access to the underlying elements.
Lets revisit.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12105: DCHECK(call->arguments()->length() == 3);
2

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

https://codereview.chromium.org/947683002/diff/20001/src/objects.cc#newcode810
src/objects.cc:810: return Smi::FromInt(FastD2I(num))->GetHash();
Why not call ComputeIntegerHash here directly so we do not have to check if it
is a Smi again?

[Sven Panne, 2015-02-23 09:26:52]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2015-02-23 09:26:53]

The general direction looks fine, I just made a few remarks about the
granularity of intrinsics. The general direction should be that we only provide
what can't be expressed in JavaScript, avoiding the need for hundreds of highly
specialized intrinsics.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1628
src/hydrogen.cc:1628: HValue* HGraphBuilder::BuildComputeIntegerHash(HValue*
value,
I think this can be expressed purely in JavaScript.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12104: void
HOptimizedGraphBuilder::GenerateFixedArraySetTheHole(CallRuntime* call) {
Hmmm, perhaps a %_Hole() intrinsic is nicer, you could use a combination of this
with the %_FixedArraySet intrinsic above then. As it is, this looks a bit too
specialized.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12117: void
HOptimizedGraphBuilder::GenerateJSCollectionGetTable(CallRuntime* call) {
We can leave it as it is for now, but I think that in the future we will have
something more general like %_GetInternalField.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12151: IfBuilder string_checker(this);
As a general rule of thumb, we should not have any non-trivial control flow in
intrinsics. This should be expressed on the JavaScript level.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h#newcode1852
src/hydrogen.h:1852: HValue* BuildElementIndexHash(HValue* index) {
When we have %_HashSeed() (for isolate()->heap()->HashSeed()), this can be
expressed purely in JavaScript. As it is, it is too specialized.

[adamk, 2015-02-23 18:43:35]

Thanks for taking a look!

https://codereview.chromium.org/947683002/diff/20001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/20001/src/collection.js#newcod...
src/collection.js:218: while (%SetIteratorNext(iterator, value_array)) {
On 2015/02/21 16:20:16, arv wrote:
> This one can get much faster now that we have access to the underlying
elements.
> Lets revisit.

Yeah, there are definitely opportunities to speed up iterators too, but this
patch is already huge :)

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12104: void
HOptimizedGraphBuilder::GenerateFixedArraySetTheHole(CallRuntime* call) {
On 2015/02/23 09:26:53, Sven Panne wrote:
> Hmmm, perhaps a %_Hole() intrinsic is nicer, you could use a combination of
this
> with the %_FixedArraySet intrinsic above then. As it is, this looks a bit too
> specialized.

Yeah, I should have left a TODO on this.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12117: void
HOptimizedGraphBuilder::GenerateJSCollectionGetTable(CallRuntime* call) {
On 2015/02/23 09:26:53, Sven Panne wrote:
> We can leave it as it is for now, but I think that in the future we will have
> something more general like %_GetInternalField.

So this is an interesting one. The Hydrogen version would actually have no
problem with %_GetInternalField, but I ran into a lot of existing Runtime code
that tries to be safe about accessing internal fields. Given that I only needed
access to a single internal field in this patch, this seemed the path of least
resistance. But I agree that the real version should probably not expose this.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12151: IfBuilder string_checker(this);
On 2015/02/23 09:26:53, Sven Panne wrote:
> As a general rule of thumb, we should not have any non-trivial control flow in
> intrinsics. This should be expressed on the JavaScript level.

Thanks, this is good feedback. "No non-trivial control flow" is a guideline I
didn't have in mind for these, but now I do.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h#newcode1852
src/hydrogen.h:1852: HValue* BuildElementIndexHash(HValue* index) {
On 2015/02/23 09:26:53, Sven Panne wrote:
> When we have %_HashSeed() (for isolate()->heap()->HashSeed()), this can be
> expressed purely in JavaScript. As it is, it is too specialized.

Yeah, I only reused this for expediency, there's no reason we couldn't compute
Smi hashes in JS (String hashes are more complex, and I think there are more
tradeoffs there).

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

https://codereview.chromium.org/947683002/diff/20001/src/objects.cc#newcode810
src/objects.cc:810: return Smi::FromInt(FastD2I(num))->GetHash();
On 2015/02/21 16:20:16, arv wrote:
> Why not call ComputeIntegerHash here directly so we do not have to check if it
> is a Smi again?

Laziness, happy to clean this up.

[adamk, 2015-02-23 19:49:06]

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12151: IfBuilder string_checker(this);
On 2015/02/23 18:43:34, adamk wrote:
> On 2015/02/23 09:26:53, Sven Panne wrote:
> > As a general rule of thumb, we should not have any non-trivial control flow
in
> > intrinsics. This should be expressed on the JavaScript level.
> 
> Thanks, this is good feedback. "No non-trivial control flow" is a guideline I
> didn't have in mind for these, but now I do.

One more issue here: the hash field in Strings uses an Integer32 representation.
What's your recommended way to return such a thing to JS?

[adamk, 2015-03-16 17:35:06]

Updated a few bits, and made this hopefully easier to diff against master by
avoiding unnecessary indentation changes (note that diffing against the last
patchset won't work well, though).

I'm inclined to move forward on this, despite the slight drop in String
performance.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1628
src/hydrogen.cc:1628: HValue* HGraphBuilder::BuildComputeIntegerHash(HValue*
value,
On 2015/02/23 09:26:53, Sven Panne wrote:
> I think this can be expressed purely in JavaScript.

Done.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12117: void
HOptimizedGraphBuilder::GenerateJSCollectionGetTable(CallRuntime* call) {
On 2015/02/23 18:43:34, adamk wrote:
> On 2015/02/23 09:26:53, Sven Panne wrote:
> > We can leave it as it is for now, but I think that in the future we will
have
> > something more general like %_GetInternalField.
> 
> So this is an interesting one. The Hydrogen version would actually have no
> problem with %_GetInternalField, but I ran into a lot of existing Runtime code
> that tries to be safe about accessing internal fields. Given that I only
needed
> access to a single internal field in this patch, this seemed the path of least
> resistance. But I agree that the real version should probably not expose this.

As discussed offline, it seems like this will stay as-is for now: after we
collect more examples of internal fields accessed from script, we might be able
to generalize more.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12151: IfBuilder string_checker(this);
On 2015/02/23 19:49:06, adamk wrote:
> On 2015/02/23 18:43:34, adamk wrote:
> > On 2015/02/23 09:26:53, Sven Panne wrote:
> > > As a general rule of thumb, we should not have any non-trivial control
flow
> in
> > > intrinsics. This should be expressed on the JavaScript level.
> > 
> > Thanks, this is good feedback. "No non-trivial control flow" is a guideline
I
> > didn't have in mind for these, but now I do.
> 
> One more issue here: the hash field in Strings uses an Integer32
representation.
> What's your recommended way to return such a thing to JS?

Turns out this works fine if it just gets returned directly to script. Changed
to %_StringGetRawHashField.

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/947683002/diff/20001/src/hydrogen.h#newcode1852
src/hydrogen.h:1852: HValue* BuildElementIndexHash(HValue* index) {
On 2015/02/23 18:43:35, adamk wrote:
> On 2015/02/23 09:26:53, Sven Panne wrote:
> > When we have %_HashSeed() (for isolate()->heap()->HashSeed()), this can be
> > expressed purely in JavaScript. As it is, it is too specialized.
> 
> Yeah, I only reused this for expediency, there's no reason we couldn't compute
> Smi hashes in JS (String hashes are more complex, and I think there are more
> tradeoffs there).

I've now written a JS version of this.

[Michael Starzinger, 2015-03-16 22:54:13]

https://codereview.chromium.org/947683002/diff/40001/src/runtime/runtime-coll...
File src/runtime/runtime-collections.cc (right):

https://codereview.chromium.org/947683002/diff/40001/src/runtime/runtime-coll...
src/runtime/runtime-collections.cc:26: return isolate->heap()->the_hole_value();
This will trigger debug code in CEntryStub::Generate that asserts that no
runtime function returns the hole. We probably will need to drop that check.

[adamk, 2015-03-17 09:05:31]

https://codereview.chromium.org/947683002/diff/40001/src/runtime/runtime-coll...
File src/runtime/runtime-collections.cc (right):

https://codereview.chromium.org/947683002/diff/40001/src/runtime/runtime-coll...
src/runtime/runtime-collections.cc:26: return isolate->heap()->the_hole_value();
On 2015/03/16 22:54:13, Michael Starzinger wrote:
> This will trigger debug code in CEntryStub::Generate that asserts that no
> runtime function returns the hole. We probably will need to drop that check.

%FixedArrayGet() can already return the hole (which is largely why I need it
here), so indeed it seems like we'd need to drop that check.

[adamk, 2015-03-18 08:45:41]

Patchset #4 (id:60001) has been deleted

[adamk, 2015-03-18 09:29:46]

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

[adamk, 2015-03-18 09:29:47]

Cleaned up a bit more, and removed checks pointed out by mstarzinger. +verwaest
in case he has opinions on how this thing interacts with the runtime.

This patch is getting closer to reality.

[adamk, 2015-03-20 11:50:44]

Patch now ready for serious review. Who should be on the R line?

[adamk, 2015-03-20 11:51:33]

Also, I realize it's a bit big. Could try to split out Map and Set separately if
a smaller CL would be easier to review.

[adamk, 2015-03-25 10:44:35]

Ping? I think Sven and Toon are reasonable folks to start looking at this. If
you guys can think of someone more appropriate, please let me know.

[arv (Not doing code reviews), 2015-03-25 11:17:01]

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:128: if (IS_NUMBER(key) && key === 0) {
Is this a bug in crankshaft?

[caitp (gmail), 2015-03-25 12:56:26]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-03-25 12:56:27]

https://1001-9732e57c756f-tainted-dot-chromiumcodereview.appspot.com/94768300...
File src/harmony-templates.js (right):

https://1001-9732e57c756f-tainted-dot-chromiumcodereview.appspot.com/94768300...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
Couldn't we just call MapGet() (and others) directly here?

[Sven Panne, 2015-03-25 14:45:56]

Looks OK from my (intrinsic-)side, let's wait for other reviews.

The FooGrow/BarShrink intrinsics are probably too complex to be anything else
than runtime calls, but that's OK for now.

[adamk, 2015-03-25 15:42:11]

On 2015/03/25 14:45:56, Sven Panne wrote:
> The FooGrow/BarShrink intrinsics are probably too complex to be anything else
> than runtime calls, but that's OK for now.

Agreed, I have no intention of trying to rewrite those anytime soon. Those paths
should not execute very often.

[adamk, 2015-03-25 15:43:07]

https://1001-9732e57c756f-tainted-dot-chromiumcodereview.appspot.com/94768300...
File src/harmony-templates.js (right):

https://1001-9732e57c756f-tainted-dot-chromiumcodereview.appspot.com/94768300...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
On 2015/03/25 12:56:27, caitp wrote:
> Couldn't we just call MapGet() (and others) directly here?

MapGet is now inside a closure, so it's not visible here.

[caitp (gmail), 2015-03-25 15:46:24]

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.j...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
On 2015/03/25 15:43:06, adamk wrote:
> On 2015/03/25 12:56:27, caitp wrote:
> > Couldn't we just call MapGet() (and others) directly here?
> 
> MapGet is now inside a closure, so it's not visible here.

I mean, you have a reference to $MapGet, otherwise %_CallFunction() wouldn't
work either.

[adamk, 2015-03-25 16:13:37]

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:462: $MapGet = MapGet;
This is where I copy MapGet into the global scope.

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.j...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
On 2015/03/25 15:46:24, caitp wrote:
> On 2015/03/25 15:43:06, adamk wrote:
> > On 2015/03/25 12:56:27, caitp wrote:
> > > Couldn't we just call MapGet() (and others) directly here?
> > 
> > MapGet is now inside a closure, so it's not visible here.
> 
> I mean, you have a reference to $MapGet, otherwise %_CallFunction() wouldn't
> work either.

Not sure what you're saying. I could have called "$MapGet" "MapGet" instead, but
$MapGet seemed a little clearer to me.

[adamk, 2015-03-25 16:14:24]

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.j...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
On 2015/03/25 16:13:36, adamk wrote:
> On 2015/03/25 15:46:24, caitp wrote:
> > On 2015/03/25 15:43:06, adamk wrote:
> > > On 2015/03/25 12:56:27, caitp wrote:
> > > > Couldn't we just call MapGet() (and others) directly here?
> > > 
> > > MapGet is now inside a closure, so it's not visible here.
> > 
> > I mean, you have a reference to $MapGet, otherwise %_CallFunction() wouldn't
> > work either.
> 
> Not sure what you're saying. I could have called "$MapGet" "MapGet" instead,
but
> $MapGet seemed a little clearer to me.

Or are you asking about the use of %_CallFunction()? That's necessary because
MapGet expects its first argument to be in |this|.

[caitp (gmail), 2015-03-25 16:31:50]

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/harmony-templates.j...
src/harmony-templates.js:24: var obj = %_CallFunction(callSiteCache, hash,
$MapGet);
On 2015/03/25 16:14:24, adamk wrote:
> On 2015/03/25 16:13:36, adamk wrote:
> > On 2015/03/25 15:46:24, caitp wrote:
> > > On 2015/03/25 15:43:06, adamk wrote:
> > > > On 2015/03/25 12:56:27, caitp wrote:
> > > > > Couldn't we just call MapGet() (and others) directly here?
> > > > 
> > > > MapGet is now inside a closure, so it's not visible here.
> > > 
> > > I mean, you have a reference to $MapGet, otherwise %_CallFunction()
wouldn't
> > > work either.
> > 
> > Not sure what you're saying. I could have called "$MapGet" "MapGet" instead,
> but
> > $MapGet seemed a little clearer to me.
> 
> Or are you asking about the use of %_CallFunction()? That's necessary because
> MapGet expects its first argument to be in |this|.

OH --- derp, you're right. Course, this map here is private, so $MapGet could be
assigned to the map instance --- anyway, do it how you want to do it

[Toon Verwaest, 2015-03-30 12:55:42]

LGTM
Sorry for the long delay, I've been a bit swamped.

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:128: if (IS_NUMBER(key) && key === 0) {
On 2015/03/25 11:17:01, arv wrote:
> Is this a bug in crankshaft?

Why is this necessary?

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12042: HInstruction* result = New<HLoadKeyed>(object, index,
nullptr, FAST_ELEMENTS);
Seems like it would be more honest to FAST_HOLEY_ELEMENTS and ALLOW_RETURN_HOLE

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12056: Add<HStoreKeyed>(object, index, value, FAST_ELEMENTS);
FAST_HOLEY_ELEMENTS?

[adamk, 2015-03-30 18:44:54]

Thanks, Toon. I've removed the IS_NUMBER optimization for now, can re-add later
(will also see if the perf trybots notice the difference as I did on my mac).

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:128: if (IS_NUMBER(key) && key === 0) {
On 2015/03/30 12:55:42, Toon Verwaest wrote:
> On 2015/03/25 11:17:01, arv wrote:
> > Is this a bug in crankshaft?
> 
> Why is this necessary?

It's not necessary for correctness, but it does help performance on the String
case. However I'd be OK with leaving it out of this patch.

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12042: HInstruction* result = New<HLoadKeyed>(object, index,
nullptr, FAST_ELEMENTS);
On 2015/03/30 12:55:42, Toon Verwaest wrote:
> Seems like it would be more honest to FAST_HOLEY_ELEMENTS and
ALLOW_RETURN_HOLE

Done.

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12056: Add<HStoreKeyed>(object, index, value, FAST_ELEMENTS);
On 2015/03/30 12:55:42, Toon Verwaest wrote:
> FAST_HOLEY_ELEMENTS?

Done.

[arv (Not doing code reviews), 2015-03-30 18:55:22]

LGTM

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:21: "use strict";
This extra use strict is not need (it also does no harm)

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:58: if (keyIsNaN && IS_NUMBER(candidate) &&
NUMBER_IS_NAN(candidate)) {
IS_NUMBER(x) && NUMBER_IS_NAN(x)

is repeated in a few places. Maybe we can introduce a new macro for this?

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:88: return (field >>> 2 /* Name::kHashShift */);
skip parens here

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:128: if (IS_NUMBER(key) && key === 0) {
This needs a comment and preferably a bug number.

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12042: HInstruction* result = New<HLoadKeyed>(object, index,
nullptr, FAST_ELEMENTS);
On 2015/03/30 18:44:54, adamk wrote:
> On 2015/03/30 12:55:42, Toon Verwaest wrote:
> > Seems like it would be more honest to FAST_HOLEY_ELEMENTS and
> ALLOW_RETURN_HOLE
> 
> Done.

Would that impact the performance?

[adamk, 2015-03-30 22:31:50]

https://codereview.chromium.org/947683002/diff/140001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:21: "use strict";
On 2015/03/30 18:55:22, arv wrote:
> This extra use strict is not need (it also does no harm)

Isn't this in fact the only one that _does_ have an effect? I thought we
concatenated all the bits of builtin JS together and so the "use strict" at the
top of each file was a lie.

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:58: if (keyIsNaN && IS_NUMBER(candidate) &&
NUMBER_IS_NAN(candidate)) {
On 2015/03/30 18:55:22, arv wrote:
> IS_NUMBER(x) && NUMBER_IS_NAN(x)
> 
> is repeated in a few places. Maybe we can introduce a new macro for this?

Happy to do that in a followup, these aren't the only places where this shows
up.

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:88: return (field >>> 2 /* Name::kHashShift */);
On 2015/03/30 18:55:22, arv wrote:
> skip parens here

Done.

https://codereview.chromium.org/947683002/diff/140001/src/collection.js#newco...
src/collection.js:128: if (IS_NUMBER(key) && key === 0) {
On 2015/03/30 18:55:22, arv wrote:
> This needs a comment and preferably a bug number.

Removed this in the latest version, the perf differences (now?) seem to be in
the noise. It's possible this causes a mostly-harmless deopt when running the
benchmark.

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/947683002/diff/140001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12042: HInstruction* result = New<HLoadKeyed>(object, index,
nullptr, FAST_ELEMENTS);
On 2015/03/30 18:55:22, arv wrote:
> On 2015/03/30 18:44:54, adamk wrote:
> > On 2015/03/30 12:55:42, Toon Verwaest wrote:
> > > Seems like it would be more honest to FAST_HOLEY_ELEMENTS and
> > ALLOW_RETURN_HOLE
> > 
> > Done.
> 
> Would that impact the performance?

I don't believe so, but if it does, that's because it's needed for correctness
(this instruction will now return true for RequiresHoleCheck()).

[adamk, 2015-03-31 00:25:46]

I think this is ready to land, but I'm going to wait till after the branch point
as there's some brand new stuff in here.

[adamk, 2015-04-06 23:26:50]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-04-06 23:26:50]

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

[commit-bot: I haz the power, 2015-04-06 23:27:09]

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

[commit-bot: I haz the power, 2015-04-07 00:11:57]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-04-07 00:12:16]

Patchset 12 (id:??) landed as
https://crrev.com/909500aa1db9789b68e101045a6359a7fcb30e83
Cr-Commit-Position: refs/heads/master@{#27605}