Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767893002:
Optimize new TypedArray(typedArray) constructor (Closed)
|
Created:
4 years, 9 months ago by Dan Ehrenberg Modified:
4 years, 9 months ago Reviewers:
adamk CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionOptimize new TypedArray(typedArray) constructor
A previous spec compliance fix for TypedArrays caused a ~4x performance
regression. This patch removes the regression by calling out
to a path within the runtime which implements array copying more
efficiently.
BUG=chromium:592007
R=adamk
LOG=Y
Committed: https://crrev.com/ab6e48de4815a03cb6523fbce209fd6fb6df9696
Cr-Commit-Position: refs/heads/master@{#34601}
Patch Set 1 #Patch Set 2 : Fix broken build #Patch Set 3 : Fix mis-named variable #Patch Set 4 : Fix test262 failures and remove regression #
Total comments: 7
Patch Set 5 : Fix asan issue #Messages
Total messages: 32 (16 generated)
The CQ bit was checked by littledan@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/1767893002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2415) v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14799) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/2411) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16534)
The CQ bit was checked by littledan@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/1767893002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2417) v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14801) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16536)
The CQ bit was checked by littledan@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/1767893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Description was changed from ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch brings the regression down to 50% by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y ========== to ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch removes the regression by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/60001
Actually the regression was due to redundant zeroing in my previous patch, and it's gone now. This new patch has some restructuring; PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js File src/js/typedarray.js (left): https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ol... src/js/typedarray.js:212: if (byteLength <= %_TypedArrayMaxSizeInHeap()) { What's up with this if statement? Why is it OK to remove it, that is? https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ne... src/js/typedarray.js:214: // It is crucial that we let any exceptions from arrayLike[i] Not sure this comment is worth keeping around. https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ne... src/js/typedarray.js:247: var initialized = %TypedArrayInitializeFromArrayLike(obj, ARRAY_ID, typedArray, length); Will this ever return false? If not, and this is the only remaining caller, should it be changed to %TypedArrayInitializeFromTypedArray? Also, 80 col nit.
The CQ bit was checked by littledan@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/1767893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/80001
https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js File src/js/typedarray.js (left): https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ol... src/js/typedarray.js:212: if (byteLength <= %_TypedArrayMaxSizeInHeap()) { On 2016/03/08 at 19:32:10, adamk wrote: > What's up with this if statement? Why is it OK to remove it, that is? Apparently it wasn't, and this was found by the tests! The new version leaves this function as-is, except for getting length passed in from the outside. I thought somehow it only mattered for TypedArrays, but it matters for arraylikes also. https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ne... src/js/typedarray.js:214: // It is crucial that we let any exceptions from arrayLike[i] On 2016/03/08 at 19:32:10, adamk wrote: > Not sure this comment is worth keeping around. How about I leave all this as is for now, since I'm not changing this part anymore, and come back later when I do the backlog of TypedArray cleanups I've been thinking about? https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ne... src/js/typedarray.js:247: var initialized = %TypedArrayInitializeFromArrayLike(obj, ARRAY_ID, typedArray, length); On 2016/03/08 at 19:32:10, adamk wrote: > Will this ever return false? If not, and this is the only remaining caller, should it be changed to %TypedArrayInitializeFromTypedArray? > > Also, 80 col nit. I played around with a bunch of alternatives here, including changing around what the runtime function does. I ended up reverting to a more conservative path once I realized how the previous code was taking advantage of the runtime function in the arraylike case (not redundantly zeroing and initializing, which is OK because an uninitialized ArrayBuffer never escapes, even if an exception is thrown when setting properties).
lgtm https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/1767893002/diff/60001/src/js/typedarray.js#ne... src/js/typedarray.js:214: // It is crucial that we let any exceptions from arrayLike[i] On 2016/03/08 19:55:54, Dan Ehrenberg wrote: > On 2016/03/08 at 19:32:10, adamk wrote: > > Not sure this comment is worth keeping around. > > How about I leave all this as is for now, since I'm not changing this part > anymore, and come back later when I do the backlog of TypedArray cleanups I've > been thinking about? Fine to leave for now since you're not touching it anymore.
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
The CQ bit was unchecked by littledan@chromium.org
Description was changed from ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch brings the regression down to 50% by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y ========== to ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch removes the regression by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y ==========
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767893002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch removes the regression by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y ========== to ========== Optimize new TypedArray(typedArray) constructor A previous spec compliance fix for TypedArrays caused a ~4x performance regression. This patch removes the regression by calling out to a path within the runtime which implements array copying more efficiently. BUG=chromium:592007 R=adamk LOG=Y Committed: https://crrev.com/ab6e48de4815a03cb6523fbce209fd6fb6df9696 Cr-Commit-Position: refs/heads/master@{#34601} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ab6e48de4815a03cb6523fbce209fd6fb6df9696 Cr-Commit-Position: refs/heads/master@{#34601} |
