Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309243003:
ES6: Array.prototype.slice and friends should use ToLength instead of ToUint32 (Closed)
DescriptionES6: Array.prototype.slice and friends should use ToLength instead of ToUint32
Defines a new --harmony-tolength flag, and a ToLengthFlagged() runtime function,
that is used where ES6 requires ToLength(), but a pre-ES6 conversion existed
before. When the flag is disabled, the function uses TO_UINT32(), which is
the pre-ES6 behaviour. When the flag enabled, the ES6-compliant ToLength()
conversion is used.
Based on a patch initially from Diego Pino <dpino@igalia.com>;
BUG=v8:3087
LOG=Y
Committed: https://crrev.com/d4e1299f1686ff55f9425d6b322c194a1968fe3f
Cr-Commit-Position: refs/heads/master@{#30772}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebased with current Git master #Patch Set 3 : Use a TO_LENGTH macro instead of the ToLengthFlagged function #
Total comments: 7
Patch Set 4 : Renamed TO_LENGTH → TO_LENGTH_OR_UINT32 #
Total comments: 2
Patch Set 5 : Fixed nit: use factory()->ToBoolean() #
Messages
Total messages: 45 (17 generated)
aperez@igalia.com changed reviewers: + arv@chromium.org, dslomov@chromium.org, rossberg@chromium.org, wingo@igalia.com
On 2015/08/31 20:00:14, aperez wrote: > mailto:aperez@igalia.com changed reviewers: > + mailto:arv@chromium.org, mailto:dslomov@chromium.org, mailto:rossberg@chromium.org, > mailto:wingo@igalia.com PTAL. Also, if you see my latest comment in the bug, there does not seem to be any performance regression when applying this change :-)
On 2015/08/31 20:00:14, aperez wrote: > mailto:aperez@igalia.com changed reviewers: > + mailto:arv@chromium.org, mailto:dslomov@chromium.org, mailto:rossberg@chromium.org, > mailto:wingo@igalia.com PTAL. Also, if you see my latest comment in the bug, there does not seem to be any performance regression when applying this change :-)
aperez@igalia.com changed reviewers: - arv@chromium.org, dslomov@chromium.org
The CQ bit was checked by cbruni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/8466) v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9533)
Doesn't merge right now, I think due to some merge artifacts (I added two comments). https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js File src/debug/liveedit.js (right): https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js#newco... src/debug/liveedit.js:219: for (var j = 0; j < update_positions_list[i].live_shared_function_infos.length; j++) { Did you change this on purpose? Seems unrelated to your fix. https://codereview.chromium.org/1309243003/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/1309243003/diff/1/src/object-observe.js#newco... src/object-observe.js:696: for (var i = 0; i < $observeObjectMethods.length; i++) I presume these are accidental changes?
https://codereview.chromium.org/1309243003/diff/1/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/1309243003/diff/1/src/runtime.js#newcode717 src/runtime.js:717: function ToLengthFlagged(arg) { I would create a TO_LENGTH macro out of this (see src/macros.py).
On 2015/09/08 07:10:34, cbruni wrote: > Doesn't merge right now, I think due to some merge artifacts (I added two > comments). Right, I am going to rebase the patch, upload a CL with the rebase. Then I'll go through the suggested changes and upload a new CL just with that, to make it easier to review. > https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js > File src/debug/liveedit.js (right): > > https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js#newco... > src/debug/liveedit.js:219: for (var j = 0; j < > update_positions_list[i].live_shared_function_infos.length; j++) { > Did you change this on purpose? Seems unrelated to your fix. This was on purpose. Not doing it would make the bootstrapper raise an exception and mksnapshot would not finish correctly. I'see if we can avoid the change once the code is uses a macro like you suggest.
On 2015/09/08 at 20:43:45, aperez wrote: > On 2015/09/08 07:10:34, cbruni wrote: > > Doesn't merge right now, I think due to some merge artifacts (I added two > > comments). > > Right, I am going to rebase the patch, upload a CL with the rebase. > Then I'll go through the suggested changes and upload a new CL just > with that, to make it easier to review. > > > https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js > > File src/debug/liveedit.js (right): > > > > https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js#newco... > > src/debug/liveedit.js:219: for (var j = 0; j < > > update_positions_list[i].live_shared_function_infos.length; j++) { > > Did you change this on purpose? Seems unrelated to your fix. > > This was on purpose. Not doing it would make the bootstrapper raise > an exception and mksnapshot would not finish correctly. I'see if we > can avoid the change once the code is uses a macro like you suggest. oh ok didn't get so far ;) Nice test coverage BTW. I'll be happy to have a look at it again.
PTAL. I have uploaded an updated CL which uses a macro as suggested. That made it possible to revert the changes to the parts of runtime code which were using .forEach(). https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js File src/debug/liveedit.js (right): https://codereview.chromium.org/1309243003/diff/1/src/debug/liveedit.js#newco... src/debug/liveedit.js:219: for (var j = 0; j < update_positions_list[i].live_shared_function_infos.length; j++) { On 2015/09/08 07:10:34, cbruni wrote: > Did you change this on purpose? Seems unrelated to your fix. Now that I am changing the code to use a macro I can revert this change and leave the code as it was. https://codereview.chromium.org/1309243003/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/1309243003/diff/1/src/object-observe.js#newco... src/object-observe.js:696: for (var i = 0; i < $observeObjectMethods.length; i++) On 2015/09/08 07:10:34, cbruni wrote: > I presume these are accidental changes? Same here, now with the macro is possible to avoid this change. https://codereview.chromium.org/1309243003/diff/1/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/1309243003/diff/1/src/runtime.js#newcode717 src/runtime.js:717: function ToLengthFlagged(arg) { On 2015/09/08 07:18:07, cbruni wrote: > I would create a TO_LENGTH macro out of this (see src/macros.py). Acknowledged.
Probably fine like this... but I was just reminded that an earlier introduction of toLength caused a serious performance regression on Regexp, so we do have to be careful. Only that the Array functions are not that prone to this as the fast paths are implemented in C++. https://codereview.chromium.org/1309243003/diff/40001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1309243003/diff/40001/src/heap/heap.h#newcode264 src/heap/heap.h:264: V(harmony_unicode_regexps_string, "harmony_unicode_regexps") \ Are these here on purpose? https://codereview.chromium.org/1309243003/diff/40001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1309243003/diff/40001/src/macros.py#newcode150 src/macros.py:150: macro TO_LENGTH(arg) = (harmony_tolength ? $toLength(arg) : TO_UINT32(arg)); With the possibility of being annoying... maybe we could change the macro name to reflect that it might change behavior depending on a flag. For instance TO_LENGHT_OR_UINT32? There has been some controversy among my colleagues on how to nicely implement this change :) https://codereview.chromium.org/1309243003/diff/40001/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/1309243003/diff/40001/src/object-observe.js#n... src/object-observe.js:696: var removePrototypeFn = function (f, i) { DAT space :D
Changed macro name, also removed the bogus patch hunks from the rebase. PTAL. https://codereview.chromium.org/1309243003/diff/40001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1309243003/diff/40001/src/heap/heap.h#newcode264 src/heap/heap.h:264: V(harmony_unicode_regexps_string, "harmony_unicode_regexps") \ On 2015/09/09 13:51:16, cbruni wrote: > Are these here on purpose? Ouch, leftovers from rebasing, I'll remove them. https://codereview.chromium.org/1309243003/diff/40001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1309243003/diff/40001/src/macros.py#newcode150 src/macros.py:150: macro TO_LENGTH(arg) = (harmony_tolength ? $toLength(arg) : TO_UINT32(arg)); On 2015/09/09 13:51:16, cbruni wrote: > With the possibility of being annoying... maybe we could change the macro name > to reflect that it might change behavior depending on a flag. For instance > TO_LENGHT_OR_UINT32? > > There has been some controversy among my colleagues on how to nicely implement > this change :) Ack. I don't mind changing the name. Reviews with nits are fine as long as there are progress and everybody is happy with the final version of the patch :) https://codereview.chromium.org/1309243003/diff/40001/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/1309243003/diff/40001/src/object-observe.js#n... src/object-observe.js:696: var removePrototypeFn = function (f, i) { On 2015/09/09 13:51:16, cbruni wrote: > DAT space :D Acknowledged. https://codereview.chromium.org/1309243003/diff/40001/src/runtime.js File src/runtime.js (left): https://codereview.chromium.org/1309243003/diff/40001/src/runtime.js#oldcode656 src/runtime.js:656: Also, I noticed this bogus line removal here. I'm nuking it.
lgtm lgmt Looks good :)
The CQ bit was checked by aperez@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by aperez@igalia.com
The CQ bit was checked by aperez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/60001
The CQ bit was unchecked by aperez@igalia.com
The CQ bit was checked by aperez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5672)
littledan@chromium.org changed reviewers: + littledan@chromium.org
lgtm Code looks good to me. I am still worried about performance though. Could you run some benchmarks which hit the path before committing? Either that or be prepared to back it out later when the regression becomes apparent.
On 2015/09/10 17:46:05, Dan Ehrenberg wrote: > lgtm > > Code looks good to me. I am still worried about performance though. Could you > run some benchmarks which hit the path before committing? Either that or be > prepared to back it out later when the regression becomes apparent. I have run some benchmarks and commented here: https://code.google.com/p/v8/issues/detail?id=3087#c30 As long as --harmony-tolength is disabled by default, there will be no observable performance regression. There are some tweaks we can done to improve performance of ToLength, and once those are in we could remove the flag and always use ToLength. If nobody against before tomorrow morning (CEST morning :D), I will tell the CQ to apply this patch by then, and I will be around the whole day in case it might need to be reverted.
The CQ bit was checked by aperez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5780)
dehrenberg@google.com changed reviewers: + mstarzinger@chromium.org
On 2015/09/15 09:32:28, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5780) The presubmit has been failing due to the lack of an owner LGTM for src/heap. Mstarzinger, could you do a review?
LGTM (rubber-stamp for heap). https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:1857: Handle<HeapObject> flag(FLAG_harmony_tolength ? heap()->true_value() nit: Use factory()->ToBoolean(FLAG_harmony_tolength) here.
LGTM (rubber-stamp for heap). https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:1857: Handle<HeapObject> flag(FLAG_harmony_tolength ? heap()->true_value() nit: Use factory()->ToBoolean(FLAG_harmony_tolength) here.
Fixed nit. Let's land this for good :) https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1309243003/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:1857: Handle<HeapObject> flag(FLAG_harmony_tolength ? heap()->true_value() On 2015/09/16 16:21:33, Michael Starzinger wrote: > nit: Use factory()->ToBoolean(FLAG_harmony_tolength) here. Acknowledged.
The CQ bit was checked by aperez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org, dehrenberg@google.com, cbruni@chromium.org, littledan@chromium.org Link to the patchset: https://codereview.chromium.org/1309243003/#ps80001 (title: "Fixed nit: use factory()->ToBoolean()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309243003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309243003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d4e1299f1686ff55f9425d6b322c194a1968fe3f Cr-Commit-Position: refs/heads/master@{#30772} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
