[es7] refactor and fix Object.values() / Object.entries()

[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org

Committed: https://crrev.com/5c5ccd9d7f8693990d1a9eb26ba3a94f376dcf0b
Cr-Commit-Position: refs/heads/master@{#33782}

[caitp (gmail), 2016-01-26 16:45:39]

I was taking a look at the ChakraCore impl, and saw that the original impl was
getting a few things wrong (verified by re-reading the spec).

Unfortunately, the pre-filtering of non-enumerable properties is not adequate
per-spec, due to the possible proxy traps or accessors that might mutate the
object.

[adamk, 2016-01-26 18:49:54]

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

[adamk, 2016-01-26 18:49:54]

+cbruni for key-accumulator expertise

[Dan Ehrenberg, 2016-01-26 18:56:32]

On 2016/01/26 at 18:49:54, adamk wrote:
> +cbruni for key-accumulator expertise

There are a bunch of new test262 tests for Object.values/Object.entries. Could
you run them and see what it says? (Sorry, the roll is still in progress; you'd
have to change DEPS to get the newer version, or wait a little bit.)

[caitp (gmail), 2016-01-26 19:57:00]

On 2016/01/26 18:56:32, Dan Ehrenberg wrote:
> On 2016/01/26 at 18:49:54, adamk wrote:
> > +cbruni for key-accumulator expertise
> 
> There are a bunch of new test262 tests for Object.values/Object.entries. Could
> you run them and see what it says? (Sorry, the roll is still in progress;
you'd
> have to change DEPS to get the newer version, or wait a little bit.)

I've just verified that it passes built-ins/Object/values/* and
built-ins/Object/entries/* with flying colours

[caitp (gmail), 2016-02-01 15:00:28]

It looks like the GetKeys_Internal with the trivial lambda gets inlined (at
least in Xcode 7 in an optdebug build), so that is hopefully a good indicator
that the lambda won't slow down Object.keys() with the llvm toolchain.

[Dan Ehrenberg, 2016-02-01 18:24:46]

On 2016/02/01 at 15:00:28, caitpotter88 wrote:
> It looks like the GetKeys_Internal with the trivial lambda gets inlined (at
least in Xcode 7 in an optdebug build), so that is hopefully a good indicator
that the lambda won't slow down Object.keys() with the llvm toolchain.

If you're thinking about performance, did you try a microbenchmark?

[caitp (gmail), 2016-02-02 02:29:16]

On 2016/02/01 18:24:46, Dan Ehrenberg wrote:
> On 2016/02/01 at 15:00:28, caitpotter88 wrote:
> > It looks like the GetKeys_Internal with the trivial lambda gets inlined (at
> least in Xcode 7 in an optdebug build), so that is hopefully a good indicator
> that the lambda won't slow down Object.keys() with the llvm toolchain.
> 
> If you're thinking about performance, did you try a microbenchmark?

I've set one up (1000 properties, 50% enumerable, 50% elements, 50% string keys)

The results are pretty similar, but with-patch (which is also rebased, and built
with a different toolchain because I seem to be having some issues with clang
ever since rebasing) seems to do a little better for each run.

>>> Running suite: JSTests/Object (Without patch)
>>> Stdout (#1):
Keys-Object(Score): 1896

>>> Stdout (#2):
Keys-Object(Score): 1803

>>> Stdout (#3):
Keys-Object(Score): 1658

>>> Stdout (#4):
Keys-Object(Score): 1663

>>> Stdout (#5):
Keys-Object(Score): 1756

---------------------------------------------

>>> Running suite: JSTests/Object (With patch)
>>> Stdout (#1):
Keys-Object(Score): 2098

>>> Stdout (#2):
Keys-Object(Score): 2126

>>> Stdout (#3):
Keys-Object(Score): 2108

>>> Stdout (#4):
Keys-Object(Score): 2104

>>> Stdout (#5):
Keys-Object(Score): 2138

---------------------------------------------

Not a super realistic benchmark, but it doesn't look like it performs too
differently in the (admittedly rare case) of big objects

[Camillo Bruni, 2016-02-03 12:03:49]

Added some feedback (mostly about "auto"). I'm fine with the templatized GetKeys
implementation.
If performance degrades we should see it immediately in sunspider's
string-fasta, which seems to be particularly affected by keys / for-in changes.

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

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.cc#...
src/key-accumulator.cc:63: auto did_accumulate = accumulate(result,
insertion_index, *key);
auto => Maybe<bool>

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.cc#...
src/key-accumulator.cc:105: auto add_value = [receiver,
isolate](Handle<FixedArray> result, int i,
I'm fine with the "auto" type for the accumulate lambda (or whatever these
things are called in C++) but I would refrain from using them in all the other
places.

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.h
File src/key-accumulator.h (right):

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:57: GetKeysConversion convert = KEEP_NUMBERS);
Do you think we will ever need the convert argument here? If not you can remove
it. For GetKeys we needed it at some point.

https://codereview.chromium.org/1637753004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1637753004/diff/40001/src/objects.cc#newcode8927
src/objects.cc:8927: USE(ContainsOnlyValidKeys);
Probably you wanted to add a DCHECK(ContainsOnlyValidKeys(keys)) at the end of
the method?

https://codereview.chromium.org/1637753004/diff/40001/src/objects.cc#newcode8939
src/objects.cc:8939: USE(ContainsOnlyValidKeys);
same here :)

[caitp (gmail), 2016-02-03 12:57:31]

thanks for the look

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

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.cc#...
src/key-accumulator.cc:105: auto add_value = [receiver,
isolate](Handle<FixedArray> result, int i,
On 2016/02/03 12:03:48, cbruni wrote:
> I'm fine with the "auto" type for the accumulate lambda (or whatever these
> things are called in C++) but I would refrain from using them in all the other
> places.

replaced all non-lambda's with proper types

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.h
File src/key-accumulator.h (right):

https://codereview.chromium.org/1637753004/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:57: GetKeysConversion convert = KEEP_NUMBERS);
On 2016/02/03 12:03:48, cbruni wrote:
> Do you think we will ever need the convert argument here? If not you can
remove
> it. For GetKeys we needed it at some point.

it has been removed

https://codereview.chromium.org/1637753004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1637753004/diff/40001/src/objects.cc#newcode8927
src/objects.cc:8927: USE(ContainsOnlyValidKeys);
On 2016/02/03 12:03:48, cbruni wrote:
> Probably you wanted to add a DCHECK(ContainsOnlyValidKeys(keys)) at the end of
> the method?

Not sure why I added this, copy/paste code? There is no FixedArray of keys to
validate in either method

[Camillo Bruni, 2016-02-04 17:21:47]

lgtm

[caitp (gmail), 2016-02-04 19:44:31]

On 2016/02/04 17:21:47, cbruni wrote:
> lgtm

the templatized key-accumulator stuff no longer works because of the proxy
shortcut, so it's a bit slower than it would have been --- refactored to remove
any touches to key-accumulator.

should get one last check before cq

[Camillo Bruni, 2016-02-05 12:53:56]

ah right, shortcuts everywhere :) 
patch 6 lgtm

[caitp (gmail), 2016-02-05 12:54:49]

Description was changed from

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

In addition, the methods were potentially inefficient, causing many more Map
transitions for the Array stores than were really needed. This CL corrects
this behaviour.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org
==========

to

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org
==========

[caitp (gmail), 2016-02-05 12:55:45]

On 2016/02/05 12:53:56, cbruni wrote:
> ah right, shortcuts everywhere :) 
> patch 6 lgtm

Dan, do you think you could sign off on the rest?

[Dan Ehrenberg, 2016-02-05 13:56:41]

lgtm

[caitp (gmail), 2016-02-05 14:05:26]

The CQ bit was checked by caitpotter88@gmail.com

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

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

[commit-bot: I haz the power, 2016-02-05 14:38:30]

Description was changed from

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org
==========

to

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org
==========

[commit-bot: I haz the power, 2016-02-05 14:38:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-05 14:38:41]

Description was changed from

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org
==========

to

==========
[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org

Committed: https://crrev.com/5c5ccd9d7f8693990d1a9eb26ba3a94f376dcf0b
Cr-Commit-Position: refs/heads/master@{#33782}
==========

[commit-bot: I haz the power, 2016-02-05 14:38:41]

Patchset 6 (id:??) landed as
https://crrev.com/5c5ccd9d7f8693990d1a9eb26ba3a94f376dcf0b
Cr-Commit-Position: refs/heads/master@{#33782}

[Michael Achenbach, 2016-02-05 15:35:05]

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

The reason for reverting is: [Sheriff] Breaks gc stress:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/....