Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(311)

Issue 548833002: [es6] implement Object.assign (Closed)

Created:
6 years, 3 months ago by caitp (gmail)
Modified:
5 years, 7 months ago
CC:
v8-dev
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[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}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Remove pendingException, move behind harmony-object flag, fix Symbol properties #

Total comments: 2

Patch Set 3 : Use propertyIsEnumerable #

Patch Set 4 : Rebased #

Total comments: 4

Patch Set 5 : Don't change v8natives.js, just use assignment rather than %SetProperty #

Patch Set 6 : Rebased again #

Total comments: 6

Patch Set 7 : Nits #

Patch Set 8 : Add some basic perf tests #

Patch Set 9 : Rebase + IIFE #

Total comments: 8

Patch Set 10 : Don't export it #

Patch Set 11 : Use runtime %IsPropertyEnumerable() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+306 lines, -5 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A src/harmony-object.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +46 lines, -0 lines 0 comments Download
M src/v8natives.js View 1 2 3 4 5 6 7 8 9 10 3 chunks +15 lines, -0 lines 0 comments Download
M test/js-perf-test/JSTests.json View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments Download
A test/js-perf-test/Object/assign.js View 1 2 3 4 5 6 7 1 chunk +81 lines, -0 lines 0 comments Download
A + test/js-perf-test/Object/run.js View 1 2 3 4 5 6 7 1 chunk +2 lines, -3 lines 0 comments Download
A test/mjsunit/harmony/object-assign.js View 1 2 3 4 5 6 1 chunk +142 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 37 (4 generated)
caitp (gmail)
So, the implementation which landed in Gecko is reportedly very slow, much slower than implementations ...
6 years, 3 months ago (2014-09-06 18:50:17 UTC) #2
caitp (gmail)
I guess the other thing is, this is currently not behind a flag, which it ...
6 years, 3 months ago (2014-09-07 14:39:55 UTC) #3
jsbell
Thanks for including me, but I'm a v8 n00b myself so of limited use. Just ...
6 years, 3 months ago (2014-09-08 17:22:42 UTC) #4
caitp (gmail)
On 2014/09/08 17:22:42, jsbell wrote: > Thanks for including me, but I'm a v8 n00b ...
6 years, 3 months ago (2014-09-08 17:48:28 UTC) #5
jsbell
Reading the Nov TC39 notes, looks like the performance issue was discussed and addressed by ...
6 years ago (2014-11-25 21:10:16 UTC) #6
arv (Not doing code reviews)
On 2014/11/25 21:10:16, jsbell wrote: > Reading the Nov TC39 notes, looks like the performance ...
6 years ago (2014-12-01 23:32:17 UTC) #7
caitp (gmail)
On 2014/12/01 23:32:17, arv wrote: > On 2014/11/25 21:10:16, jsbell wrote: > > Reading the ...
6 years ago (2014-12-02 00:27:01 UTC) #8
arv (Not doing code reviews)
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#newcode24 src/harmony-object.js:24: var desc = ObjectGetOwnPropertyDescriptor(from, key); I believe we an ...
6 years ago (2014-12-02 14:41:19 UTC) #10
caitp (gmail)
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#newcode24 src/harmony-object.js:24: var desc = ObjectGetOwnPropertyDescriptor(from, key); On 2014/12/02 14:41:19, arv ...
6 years ago (2014-12-02 18:32:08 UTC) #11
arv (Not doing code reviews)
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#newcode24 src/harmony-object.js:24: if (ObjectPropertyIsEnumerableJS(from, key)) { You can do %_CallFunction here ...
6 years ago (2014-12-02 19:16:06 UTC) #12
caitp (gmail)
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#newcode24 src/harmony-object.js:24: if (ObjectPropertyIsEnumerableJS(from, key)) { On 2014/12/02 19:16:06, arv wrote: ...
6 years ago (2014-12-02 19:39:01 UTC) #13
caitp (gmail)
PTAL, this one has almost had its first birthday =) I had a look through ...
5 years, 8 months ago (2015-04-03 20:58:34 UTC) #14
arv (Not doing code reviews)
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#newcode21 src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from); ObjectGetOwnPropertyKeys takes 2 params https://codereview.chromium.org/548833002/diff/100001/src/harmony-object.js#newcode21 ...
5 years, 8 months ago (2015-04-03 21:15:09 UTC) #15
caitp (gmail)
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#newcode21 src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from); On 2015/04/03 21:15:09, arv wrote: ...
5 years, 8 months ago (2015-04-03 21:28:41 UTC) #16
caitp (gmail)
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#newcode21 > ...
5 years, 8 months ago (2015-04-03 21:37:26 UTC) #17
arv (Not doing code reviews)
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#newcode21 src/harmony-object.js:21: var keys = ObjectGetOwnPropertyKeys(from); On 2015/04/03 21:28:41, caitp wrote: ...
5 years, 8 months ago (2015-04-03 21:43:02 UTC) #18
caitp (gmail)
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#newcode21 > ...
5 years, 8 months ago (2015-04-03 21:44:38 UTC) #19
arv (Not doing code reviews)
Thanks.
5 years, 8 months ago (2015-04-03 22:03:44 UTC) #20
caitp (gmail)
On 2015/04/03 22:03:44, arv wrote: > Thanks. The perf comparison with lodash is still pretty ...
5 years, 8 months ago (2015-04-03 22:17:23 UTC) #21
impinball
On 2015/04/03 22:17:23, caitp wrote: > On 2015/04/03 22:03:44, arv wrote: > > Thanks. > ...
5 years, 8 months ago (2015-04-06 14:57:59 UTC) #22
caitp (gmail)
On 2015/04/06 14:57:59, impinball wrote: > On 2015/04/03 22:17:23, caitp wrote: > > On 2015/04/03 ...
5 years, 8 months ago (2015-04-06 15:21:45 UTC) #23
caitp (gmail)
anyways, as best as I could figure, lodash gets a bit of a speed bump, ...
5 years, 8 months ago (2015-04-09 21:27:34 UTC) #24
caitp (gmail)
On 2015/04/09 21:27:34, caitp wrote: > anyways, as best as I could figure, lodash gets ...
5 years, 8 months ago (2015-04-10 00:54:25 UTC) #25
rossberg
lgtm
5 years, 7 months ago (2015-05-06 11:43:06 UTC) #26
arv (Not doing code reviews)
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#newcode6 src/harmony-object.js:6: var $objectAssign; This is never used. ...
5 years, 7 months ago (2015-05-06 15:35:48 UTC) #27
caitp (gmail)
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#newcode6 src/harmony-object.js:6: var $objectAssign; On 2015/05/06 15:35:48, arv wrote: > ...
5 years, 7 months ago (2015-05-06 15:40:26 UTC) #28
caitp (gmail)
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#newcode34 src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable)) { On 2015/05/06 15:40:26, caitp ...
5 years, 7 months ago (2015-05-06 15:43:00 UTC) #29
arv (Not doing code reviews)
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#newcode34 src/harmony-object.js:34: if (%_CallFunction(from, key, $propertyIsEnumerable)) { On 2015/05/06 15:40:26, caitp ...
5 years, 7 months ago (2015-05-06 15:44:34 UTC) #30
caitp (gmail)
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#newcode34 > ...
5 years, 7 months ago (2015-05-06 15:48:42 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/548833002/200001
5 years, 7 months ago (2015-05-06 15:51:49 UTC) #34
arv (Not doing code reviews)
Great. Still LGTM
5 years, 7 months ago (2015-05-06 16:05:24 UTC) #35
commit-bot: I haz the power
Committed patchset #11 (id:200001)
5 years, 7 months ago (2015-05-06 16:17:46 UTC) #36
commit-bot: I haz the power
5 years, 7 months ago (2015-05-06 16:18:05 UTC) #37
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/fda20efb2f75a11b711501c3d8bb19cef4873200
Cr-Commit-Position: refs/heads/master@{#28270}

Powered by Google App Engine
This is Rietveld 408576698