Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 3 months ago by caitp (gmail) Modified:
5 years, 7 months ago Reviewers:
arv (Not doing code reviews), jsbell, rossberg 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() #
Created: 5 years, 7 months ago
Messages
Total messages: 37 (4 generated)
caitpotter88@gmail.com changed reviewers: + jsbell@chromium.org, rossberg@chromium.org
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
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?)
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?
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.
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.)
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.
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@chromium.org changed reviewers: + arv@chromium.org
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.
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.
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;
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.
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.
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?
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
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
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?
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?
Thanks.
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 ```
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.
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.
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
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" :<
lgtm
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
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
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
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.
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
The CQ bit was checked by caitpotter88@gmail.com
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()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/548833002/200001
Great. Still LGTM
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/fda20efb2f75a11b711501c3d8bb19cef4873200 Cr-Commit-Position: refs/heads/master@{#28270} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
