[es6] implement Object.assign

[es6] implement Object.assign

BUG=v8:4007
LOG=N
R=arv@chromium.org, rossberg@chromium.org

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

[caitp (gmail), 2014-09-06 18:44:27]

caitpotter88@gmail.com changed reviewers:
+ jsbell@chromium.org, rossberg@chromium.org

[caitp (gmail), 2014-09-06 18:50:17]

So, the implementation which landed in Gecko is reportedly very slow, much
slower than implementations like lodash's _.assign. My goal here is to try and
do a bit of a better job with this.

The only real strategy I've tried so far is wrapping the try/catch block in a
function call, so that maybe this will help ensure ObjectAssign can be
optimized. This is at the expense of readability, so maybe it's possible to do
better here --- the winnings from this technique are pretty marginal based on
the perf test I wrote

V1 (TryCatch3)

ObjectAssign-Object(Score): 12663
ObjectAssign-Object(Score): 13717
ObjectAssign-Object(Score): 12650
ObjectAssign-Object(Score): 13204
ObjectAssign-Object(Score): 13228

V1.2

ObjectAssign-Object(Score): 13451
ObjectAssign-Object(Score): 13706
ObjectAssign-Object(Score): 13559
ObjectAssign-Object(Score): 13322
ObjectAssign-Object(Score): 13348

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

V2 (No TryCatch3)

ObjectAssign-Object(Score): 13004
ObjectAssign-Object(Score): 12502
ObjectAssign-Object(Score): 11859
ObjectAssign-Object(Score): 12566
ObjectAssign-Object(Score): 12414

[caitp (gmail), 2014-09-07 14:39:55]

I guess the other thing is, this is currently not behind a flag, which it
probably should be (move to harmony-object.js or something and conditionally
install?)

[jsbell, 2014-09-08 17:22:42]

Thanks for including me, but I'm a v8 n00b myself so of limited use. Just a
couple of nits.

Since you're worried about perf, do you have absolute performance numbers, e.g.
for your impl with inline try/catch vs. your impl with TryCatch3 (current CL)
vs. lodash vs. (e.g.) your impl with exception handling totally removed? I
glanced at the lodash code[1] and it looks like it does no exception handling.

[1] https://github.com/lodash/lodash/blob/master/dist/lodash.js#L1143

https://codereview.chromium.org/548833002/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/548833002/diff/1/src/v8natives.js#newcode1399
src/v8natives.js:1399: kError.error = e;
Wouldn't this "leak" the exception?

(I'm not intimately familiar with the special environment for v8natives so maybe
this is taken care of elsewhere.)

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
File test/mjsunit/harmony/object-assign.js (right):

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:33: // TODO: `b.aSymbol is not copied into
a
Extraneous (or missing) `

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:122: // String and Symbol valued
properties are copied
Add another TODO here for Symbol property?

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:130: // Intermediate exceptions do not
stop property traversal, first exception is
Add a property that *is* copied successfully, even if the rest throw?

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:131: // reported (3)
What's the '(3)' here?

[caitp (gmail), 2014-09-08 17:48:28]

On 2014/09/08 17:22:42, jsbell wrote:
> Thanks for including me, but I'm a v8 n00b myself so of limited use. Just a
> couple of nits.
> 
> Since you're worried about perf, do you have absolute performance numbers,
e.g.
> for your impl with inline try/catch vs. your impl with TryCatch3 (current CL)
> vs. lodash vs. (e.g.) your impl with exception handling totally removed? I
> glanced at the lodash code[1] and it looks like it does no exception handling.
> 
> [1] https://github.com/lodash/lodash/blob/master/dist/lodash.js#L1143


Yeah, I'll get some numbers for lodash with the same test suite, but I'm not
confident that it's really possible for Object.assign to beat _.assign.

https://codereview.chromium.org/548833002/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/548833002/diff/1/src/v8natives.js#newcode1399
src/v8natives.js:1399: kError.error = e;
Yes, within the sandbox it would be kept alive --- a way around this would be to
explicitly null it after finishing, but it's unlikely to make a big difference

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
File test/mjsunit/harmony/object-assign.js (right):

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:33: // TODO: `b.aSymbol is not copied into
a
On 2014/09/08 17:22:42, jsbell wrote:
> Extraneous (or missing) `

Acknowledged.

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:122: // String and Symbol valued
properties are copied
On 2014/09/08 17:22:42, jsbell wrote:
> Add another TODO here for Symbol property?

All of the tests here were basically copy/pasted from the spidermonkey
Object.assign tests and adapted for mjsunit --- with the exception of tests
which make use of direct proxies (which is unfortunate, because they look very
useful here).

I believe this comment (which was pulled out of spidermonkey) is sort of a
misnomer, it should be "keyed" rather than "valued". There is a test (which is
commented out due to being broken) on line 33 which asserts that symbol keys are
copied.

It would probably make sense to move that test down here, or move this one up
there. I'll do that after investigating why it's not copying symbol keys.

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:130: // Intermediate exceptions do not
stop property traversal, first exception is
On 2014/09/08 17:22:42, jsbell wrote:
> Add a property that *is* copied successfully, even if the rest throw?

asserting that log equals "ba" is asserting that both copies are copied (the
setter throws), even though the first setter threw an exception

https://codereview.chromium.org/548833002/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-assign.js:131: // reported (3)
On 2014/09/08 17:22:42, jsbell wrote:
> What's the '(3)' here?

These tests are pulled out of the spidermonkey Object.assign test suite, there
are a series of other tests here, which are numbered this way, but unfortunately
these all depend on direct proxies (we don't have a way of faking
[[OwnPropertyKeys]] in v8 currently).

This one sort of works around this using the assumption that the order of
properties in a literal will be the order that they appear in
[[OwnPropertyKeys]], which is spec-correct, but still not as robust as mocking
[[OwnPropertyKeys]] with a proxy.

Other tests using proxies were a bit harder to hack around the lack of direct
proxies and lack of support for 'ownKeys', but I'll look at adding more when I
have some time.

[jsbell, 2014-11-25 21:10:16]

Reading the Nov TC39 notes, looks like the performance issue was discussed and
addressed by removing the pendingExeption bit from the spec, just throwing the
first exception when it occurs.

https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-19.md

(Presumably that's the reason lodash etc trump the Gecko impl, but I haven't
actually looked myself.)

[arv (Not doing code reviews), 2014-12-01 23:32:17]

On 2014/11/25 21:10:16, jsbell wrote:
> Reading the Nov TC39 notes, looks like the performance issue was discussed and
> addressed by removing the pendingExeption bit from the spec, just throwing the
> first exception when it occurs.
> 
> https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-19.md
> 
> (Presumably that's the reason lodash etc trump the Gecko impl, but I haven't
> actually looked myself.)

Correct. No need for a try/catch any more.

[caitp (gmail), 2014-12-02 00:27:01]

On 2014/12/01 23:32:17, arv wrote:
> On 2014/11/25 21:10:16, jsbell wrote:
> > Reading the Nov TC39 notes, looks like the performance issue was discussed
and
> > addressed by removing the pendingExeption bit from the spec, just throwing
the
> > first exception when it occurs.
> > 
> > https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-19.md
> > 
> > (Presumably that's the reason lodash etc trump the Gecko impl, but I haven't
> > actually looked myself.)
> 
> Correct. No need for a try/catch any more.

Added a new version without pendingException, and with a few bugs fixed.
Perf-tests are gone for now, but they were probably wrong anyways, I'll re-add
in a subsequent CL if we want to launch this feature

[arv (Not doing code reviews), 2014-12-02 14:41:18]

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

[arv (Not doing code reviews), 2014-12-02 14:41:19]

https://codereview.chromium.org/548833002/diff/20001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/20001/src/harmony-object.js#ne...
src/harmony-object.js:24: var desc = ObjectGetOwnPropertyDescriptor(from, key);
I believe we an use ObjectPropertyIsEnumerable here instead.

[caitp (gmail), 2014-12-02 18:32:08]

https://codereview.chromium.org/548833002/diff/20001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/20001/src/harmony-object.js#ne...
src/harmony-object.js:24: var desc = ObjectGetOwnPropertyDescriptor(from, key);
On 2014/12/02 14:41:19, arv wrote:
> I believe we an use ObjectPropertyIsEnumerable here instead.

did these earlier --- I slightly modified the way ObjectPropertyIsEnumerable
works, so that there's no need to use [[Call]] --- I dunno if that matters or
not, but it seemed useful to me.

[arv (Not doing code reviews), 2014-12-02 19:16:06]

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js#ne...
src/harmony-object.js:24: if (ObjectPropertyIsEnumerableJS(from, key)) {
You can do %_CallFunction here so you do not need to change v8natives.js

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js#ne...
src/harmony-object.js:26: %SetProperty(to, key, propValue, kStrictMode);
I don't think this needs a runtime.

to[key] = propValue;

[caitp (gmail), 2014-12-02 19:39:01]

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js#ne...
src/harmony-object.js:24: if (ObjectPropertyIsEnumerableJS(from, key)) {
On 2014/12/02 19:16:06, arv wrote:
> You can do %_CallFunction here so you do not need to change v8natives.js

Done --- It's much harder to read like this though :>

https://codereview.chromium.org/548833002/diff/60001/src/harmony-object.js#ne...
src/harmony-object.js:26: %SetProperty(to, key, propValue, kStrictMode);
On 2014/12/02 19:16:06, arv wrote:
> I don't think this needs a runtime.
> 
> to[key] = propValue;

Done.

[caitp (gmail), 2015-04-03 20:58:34]

PTAL, this one has almost had its first birthday =)

I had a look through v8-users and didn't find any intent thread for Object.is,
which already shipped, so I think that can be foregone.

Since the changes a few months ago regarding throwing behaviour, the performance
difference as compared with lodash/underscore's versions is probably much
smaller now.

[arv (Not doing code reviews), 2015-04-03 21:15:09]

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
ObjectGetOwnPropertyKeys takes 2 params

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
This will not include proxies. Maybe call a higher level function? and add test

https://codereview.chromium.org/548833002/diff/100001/test/mjsunit/harmony/ob...
File test/mjsunit/harmony/object-assign.js (right):

https://codereview.chromium.org/548833002/diff/100001/test/mjsunit/harmony/ob...
test/mjsunit/harmony/object-assign.js:7: // Based on Mozilla Array.of() tests at
http://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_6/Object/assi...
Is this comment really correct?

[caitp (gmail), 2015-04-03 21:28:41]

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
On 2015/04/03 21:15:09, arv wrote:
> ObjectGetOwnPropertyKeys takes 2 params

The filter is optional-ish, but will do

On 2015/04/03 21:15:09, arv wrote:
> This will not include proxies. Maybe call a higher level function? and add
test

We don't actually have direct proxies in v8, and the implemented proxies aren't
shipping --- is it really needed right now?

I can add a version based on the proxy [[OwnPropertyKeys]] handler, but it's
going to be dead code until direct proxies are implemented

https://codereview.chromium.org/548833002/diff/100001/test/mjsunit/harmony/ob...
File test/mjsunit/harmony/object-assign.js (right):

https://codereview.chromium.org/548833002/diff/100001/test/mjsunit/harmony/ob...
test/mjsunit/harmony/object-assign.js:7: // Based on Mozilla Array.of() tests at
http://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_6/Object/assi...
On 2015/04/03 21:15:09, arv wrote:
> Is this comment really correct?

The Array.of() part certainly isn't! fixing that

[caitp (gmail), 2015-04-03 21:37:26]

On 2015/04/03 21:28:41, caitp wrote:
> https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js
> File src/harmony-object.js (right):
> 
>
https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
> src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
> On 2015/04/03 21:15:09, arv wrote:
> > ObjectGetOwnPropertyKeys takes 2 params
> 
> The filter is optional-ish, but will do
> 
> On 2015/04/03 21:15:09, arv wrote:
> > This will not include proxies. Maybe call a higher level function? and add
> test
> 
> We don't actually have direct proxies in v8, and the implemented proxies
aren't
> shipping --- is it really needed right now?
> 
> I can add a version based on the proxy [[OwnPropertyKeys]] handler, but it's
> going to be dead code until direct proxies are implemented

I might have a go at implementing direct proxies --- it doesn't seem possible to
really follow the spec here currently,
since the current proxy implementation doesn't save the proxy target in an
internal slot. Adding that is out of scope
for this, but that seems like a fun project to work on later, if nobody else is
up for it

[arv (Not doing code reviews), 2015-04-03 21:43:02]

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
On 2015/04/03 21:28:41, caitp wrote:
> On 2015/04/03 21:15:09, arv wrote:
> > ObjectGetOwnPropertyKeys takes 2 params
> 
> The filter is optional-ish, but will do

Yes, but it adds an adapter frame for no good reason.

> On 2015/04/03 21:15:09, arv wrote:
> > This will not include proxies. Maybe call a higher level function? and add
> test
> 
> We don't actually have direct proxies in v8, and the implemented proxies
aren't
> shipping --- is it really needed right now?
> 
> I can add a version based on the proxy [[OwnPropertyKeys]] handler, but it's
> going to be dead code until direct proxies are implemented

We have a proxy implementation that we do call for ObjectGetOwnPropertyNames so
it seems likely that we would forget to make Object.assign work when we update
our proxy implementation unless ObjectGetOwnPropertyKeys already works with
proxies.

Maybe just add an "assert" for now?

[caitp (gmail), 2015-04-03 21:44:38]

On 2015/04/03 21:43:02, arv wrote:
> https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js
> File src/harmony-object.js (right):
> 
>
https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#n...
> src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from);
> On 2015/04/03 21:28:41, caitp wrote:
> > On 2015/04/03 21:15:09, arv wrote:
> > > ObjectGetOwnPropertyKeys takes 2 params
> > 
> > The filter is optional-ish, but will do
> 
> Yes, but it adds an adapter frame for no good reason.
> 
> > On 2015/04/03 21:15:09, arv wrote:
> > > This will not include proxies. Maybe call a higher level function? and add
> > test
> > 
> > We don't actually have direct proxies in v8, and the implemented proxies
> aren't
> > shipping --- is it really needed right now?
> > 
> > I can add a version based on the proxy [[OwnPropertyKeys]] handler, but it's
> > going to be dead code until direct proxies are implemented
> 
> We have a proxy implementation that we do call for ObjectGetOwnPropertyNames
so
> it seems likely that we would forget to make Object.assign work when we update
> our proxy implementation unless ObjectGetOwnPropertyKeys already works with
> proxies.
> 
> Maybe just add an "assert" for now?

I added a version which calls the ownKeys handler, and throws if it's not there
--- hows that?

[arv (Not doing code reviews), 2015-04-03 22:03:44]

Thanks.

[caitp (gmail), 2015-04-03 22:17:23]

On 2015/04/03 22:03:44, arv wrote:
> Thanks.

The perf comparison with lodash is still pretty bad...

(lodash tests are using v3.6.0, the test code is almost identical except
Object.assign is replaced with _.assign)

So that's not great :(

```
Echo:v8 caitp$ tools/run_perf.py test/js-perf-test/ObjectAssign.json 
>>> Running suite: ObjectAssign/Object
>>> Stdout (#1):
Assign-Object(Score): 30174
LodashAssign-Object(Score): 79510

>>> Stdout (#2):
Assign-Object(Score): 29988
LodashAssign-Object(Score): 81014

>>> Stdout (#3):
Assign-Object(Score): 30211
LodashAssign-Object(Score): 83624

>>> Stdout (#4):
Assign-Object(Score): 29753
LodashAssign-Object(Score): 84225

>>> Stdout (#5):
Assign-Object(Score): 30701
LodashAssign-Object(Score): 84638
```

[impinball, 2015-04-06 14:57:59]

On 2015/04/03 22:17:23, caitp wrote:
> On 2015/04/03 22:03:44, arv wrote:
> > Thanks.
> 
> The perf comparison with lodash is still pretty bad...
> 
> (lodash tests are using v3.6.0, the test code is almost identical except
> Object.assign is replaced with _.assign)
> 
> So that's not great :(
> 
> ```
> Echo:v8 caitp$ tools/run_perf.py test/js-perf-test/ObjectAssign.json 
> >>> Running suite: ObjectAssign/Object
> >>> Stdout (#1):
> Assign-Object(Score): 30174
> LodashAssign-Object(Score): 79510
> 
> >>> Stdout (#2):
> Assign-Object(Score): 29988
> LodashAssign-Object(Score): 81014
> 
> >>> Stdout (#3):
> Assign-Object(Score): 30211
> LodashAssign-Object(Score): 83624
> 
> >>> Stdout (#4):
> Assign-Object(Score): 29753
> LodashAssign-Object(Score): 84225
> 
> >>> Stdout (#5):
> Assign-Object(Score): 30701
> LodashAssign-Object(Score): 84638
> ```

Or, in other words, this may need some heavy profiling.

Although, I wonder if you could get away with ObjectKeys (specifically the JS
function, not the runtime one) instead of OwnPropertyKeys + a check in the loop,
since I see a *lot* of parallels between EnumerableOwnNames and Object.assign's
algorithm within the spec (and AFAICT there wouldn't be any visible changes,
even with a proxy). Although I won't be able to test this until tonight to know
for sure, I wouldn't be surprised if that helps cut down a lot of that perf
problem.

[caitp (gmail), 2015-04-06 15:21:45]

On 2015/04/06 14:57:59, impinball wrote:
> On 2015/04/03 22:17:23, caitp wrote:
> > On 2015/04/03 22:03:44, arv wrote:
> > > Thanks.
> > 
> > The perf comparison with lodash is still pretty bad...
> > 
> > (lodash tests are using v3.6.0, the test code is almost identical except
> > Object.assign is replaced with _.assign)
> > 
> > So that's not great :(
> > 
> > ```
> > Echo:v8 caitp$ tools/run_perf.py test/js-perf-test/ObjectAssign.json 
> > >>> Running suite: ObjectAssign/Object
> > >>> Stdout (#1):
> > Assign-Object(Score): 30174
> > LodashAssign-Object(Score): 79510
> > 
> > >>> Stdout (#2):
> > Assign-Object(Score): 29988
> > LodashAssign-Object(Score): 81014
> > 
> > >>> Stdout (#3):
> > Assign-Object(Score): 30211
> > LodashAssign-Object(Score): 83624
> > 
> > >>> Stdout (#4):
> > Assign-Object(Score): 29753
> > LodashAssign-Object(Score): 84225
> > 
> > >>> Stdout (#5):
> > Assign-Object(Score): 30701
> > LodashAssign-Object(Score): 84638
> > ```
> 
> Or, in other words, this may need some heavy profiling.
> 
> Although, I wonder if you could get away with ObjectKeys (specifically the JS
> function, not the runtime one) instead of OwnPropertyKeys + a check in the
loop,
> since I see a *lot* of parallels between EnumerableOwnNames and
Object.assign's
> algorithm within the spec (and AFAICT there wouldn't be any visible changes,
> even with a proxy). Although I won't be able to test this until tonight to
know
> for sure, I wouldn't be surprised if that helps cut down a lot of that perf
> problem.

The difference is that EnumerableOwnNames doesn't include Symbol properties,
which Object.assign is intended to deal with.

[caitp (gmail), 2015-04-09 21:27:34]

anyways, as best as I could figure, lodash gets a bit of a speed bump, because
it's able to take advantage of Object.keys() (which can cache enumerable string
keys for an object). I'm not sure if this perf enhancement is noticeable outside
of microbenchmarks or not, depending on how the caching works.

It should definitely be possible to speed up the code for O.assign in the same
way, but I think that can wait until the feature lands. Just needs a signoff

[caitp (gmail), 2015-04-10 00:54:25]

On 2015/04/09 21:27:34, caitp wrote:
> anyways, as best as I could figure, lodash gets a bit of a speed bump, because
> it's able to take advantage of Object.keys() (which can cache enumerable
string
> keys for an object). I'm not sure if this perf enhancement is noticeable
outside
> of microbenchmarks or not, depending on how the caching works.
> 
> It should definitely be possible to speed up the code for O.assign in the same
> way, but I think that can wait until the feature lands. Just needs a signoff

Ahhh, did I say "speed bump", I meant "speed boost" :<

[rossberg, 2015-05-06 11:43:06]

lgtm

[arv (Not doing code reviews), 2015-05-06 15:35:48]

LGTM with nits

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:6: var $objectAssign;
This is never used. I think the idea is to only "export" what is needed.

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable))
{
I wonder if this does anything extra over [[GetOwnProperty]] that might be
observable?

https://codereview.chromium.org/548833002/diff/160001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/548833002/diff/160001/src/v8natives.js#newcode29
src/v8natives.js:29: var $ownPropertyKeys;
$ownPropertyKeys does not need to be exported

https://codereview.chromium.org/548833002/diff/160001/test/mjsunit/harmony/ob...
File test/mjsunit/harmony/object-assign.js (right):

https://codereview.chromium.org/548833002/diff/160001/test/mjsunit/harmony/ob...
test/mjsunit/harmony/object-assign.js:9: function checkDataProperty(object,
propertyKey, value, writable, enumerable, configurable) {
long line

[caitp (gmail), 2015-05-06 15:40:26]

thx

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:6: var $objectAssign;
On 2015/05/06 15:35:48, arv wrote:
> This is never used. I think the idea is to only "export" what is needed.

Acknowledged.

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable))
{
On 2015/05/06 15:35:48, arv wrote:
> I wonder if this does anything extra over [[GetOwnProperty]] that might be
> observable?

I can't remember why went with this, maybe [[GetOwnProperty]] was just slow or
something. But this way isn't really following the spec, so I guess that's worth
fixing

[caitp (gmail), 2015-05-06 15:43:00]

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable))
{
On 2015/05/06 15:40:26, caitp wrote:
> On 2015/05/06 15:35:48, arv wrote:
> > I wonder if this does anything extra over [[GetOwnProperty]] that might be
> > observable?
> 
> I can't remember why went with this, maybe [[GetOwnProperty]] was just slow or
> something. But this way isn't really following the spec, so I guess that's
worth
> fixing

Wait no, $propertyIsEnumerable looks like it's a bit faster for non-proxies, but
still should basically do the right thing for proxies, so it's the right way to
go

[arv (Not doing code reviews), 2015-05-06 15:44:34]

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js
File src/harmony-object.js (right):

https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable))
{
On 2015/05/06 15:40:26, caitp wrote:
> On 2015/05/06 15:35:48, arv wrote:
> > I wonder if this does anything extra over [[GetOwnProperty]] that might be
> > observable?
> 
> I can't remember why went with this, maybe [[GetOwnProperty]] was just slow or
> something. But this way isn't really following the spec, so I guess that's
worth
> fixing

Maybe we could have something even lower level here? It looks like you can just
call %IsPropertyEnumerable here. It will call into proxies as needed.

[caitp (gmail), 2015-05-06 15:48:42]

On 2015/05/06 15:44:34, arv wrote:
> https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js
> File src/harmony-object.js (right):
> 
>
https://codereview.chromium.org/548833002/diff/160001/src/harmony-object.js#n...
> src/harmony-object.js:34: if (%_CallFunction(from, key,
$propertyIsEnumerable))
> {
> On 2015/05/06 15:40:26, caitp wrote:
> > On 2015/05/06 15:35:48, arv wrote:
> > > I wonder if this does anything extra over [[GetOwnProperty]] that might be
> > > observable?
> > 
> > I can't remember why went with this, maybe [[GetOwnProperty]] was just slow
or
> > something. But this way isn't really following the spec, so I guess that's
> worth
> > fixing
> 
> Maybe we could have something even lower level here? It looks like you can
just
> call %IsPropertyEnumerable here. It will call into proxies as needed.

The v8natives method will do that anyways, so I guess that's fine

[caitp (gmail), 2015-05-06 15:51:33]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-05-06 15:51:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, arv@chromium.org
Link to the patchset: https://codereview.chromium.org/548833002/#ps200001
(title: "Use runtime %IsPropertyEnumerable()")

[commit-bot: I haz the power, 2015-05-06 15:51:49]

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

[arv (Not doing code reviews), 2015-05-06 16:05:24]

Great. Still LGTM

[commit-bot: I haz the power, 2015-05-06 16:17:46]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-06 16:18:05]

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