Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1132723008:
Implement %TypedArray%.reverse (Closed)
|
Created:
5 years, 7 months ago by dehrenberg Modified:
5 years, 7 months ago Reviewers:
arv (Not doing code reviews) CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement %TypedArray%.reverse
This patch adds the reverse method to TypedArrays, together with a
test. The test also runs for normal Arrays, since I didn't see a
test for reversing dense arrays.
BUG=v8:3578
LOG=Y
R=arv@chromium.org
Committed: https://crrev.com/cc74268d30074934d7a40dd8256862b45dcbf0d8
Patch from Daniel Ehrenberg <dehrenberg@chromium.org>;.
Cr-Commit-Position: refs/heads/master@{#28493}
Patch Set 1 #
Total comments: 6
Patch Set 2 : cleanups #
Total comments: 3
Patch Set 3 : rebase #
Messages
Total messages: 21 (7 generated)
https://codereview.chromium.org/1132723008/diff/1/src/array.js File src/array.js (right): https://codereview.chromium.org/1132723008/diff/1/src/array.js#newcode569 src/array.js:569: if (UseSparseVariant(array, len, IS_ARRAY(array), len)) { This is always false for typed arrays. This can therefore be moved to ArrayReverse. https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... File test/mjsunit/harmony/typedarray-reverse.js (right): https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... test/mjsunit/harmony/typedarray-reverse.js:12: var typedArrayConstructors = [ rename since it contains non typed array constructor now https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... test/mjsunit/harmony/typedarray-reverse.js:22: id // Also test arrays Rename id to ArrayMaker or something...
https://codereview.chromium.org/1132723008/diff/1/src/array.js File src/array.js (right): https://codereview.chromium.org/1132723008/diff/1/src/array.js#newcode569 src/array.js:569: if (UseSparseVariant(array, len, IS_ARRAY(array), len)) { On 2015/05/19 15:18:02, arv wrote: > This is always false for typed arrays. This can therefore be moved to > ArrayReverse. Done. https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... File test/mjsunit/harmony/typedarray-reverse.js (right): https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... test/mjsunit/harmony/typedarray-reverse.js:12: var typedArrayConstructors = [ On 2015/05/19 15:18:03, arv wrote: > rename since it contains non typed array constructor now Done. https://codereview.chromium.org/1132723008/diff/1/test/mjsunit/harmony/typeda... test/mjsunit/harmony/typedarray-reverse.js:22: id // Also test arrays On 2015/05/19 15:18:03, arv wrote: > Rename id to ArrayMaker or something... Done.
https://codereview.chromium.org/1132723008/diff/20001/src/array.js File src/array.js (right): https://codereview.chromium.org/1132723008/diff/20001/src/array.js#newcode572 src/array.js:572: if (!IS_UNDEFINED(current_i) || i in array) { Add todos to remove the in checks for typed arrays. ...and also typed arrays cannot contain undefined or holes At this point reverse is probably better reimplemented.
https://codereview.chromium.org/1132723008/diff/20001/src/array.js File src/array.js (right): https://codereview.chromium.org/1132723008/diff/20001/src/array.js#newcode572 src/array.js:572: if (!IS_UNDEFINED(current_i) || i in array) { On 2015/05/19 16:37:25, arv wrote: > Add todos to remove the in checks for typed arrays. > > ...and also typed arrays cannot contain undefined or holes > > At this point reverse is probably better reimplemented. All the array methods have tons of code to deal with holes. I don't think it'd be useful to sprinkle todos everywhere. I'm not even positive if all of these are performance-sensitive--for example, you could optimize join to not handle holes, but I don't think any program would get faster as a result. What I've been doing in this patch series is just getting methods to work at a basic level on TypedArrays, maybe the most useful part being that there are tests. We will optimize afterwards. I am not sure if reverse() comes up as something that's important to optimize, but we'll have to see by examining real programs or thinking about it more. In this particular case, I think the lack of a cloned method body would a much bigger performance effect than these little conditionals on holes, but that's just a guess.
LGTM Lets get the right behavior first and then see what becomes hot. https://codereview.chromium.org/1132723008/diff/20001/src/array.js File src/array.js (right): https://codereview.chromium.org/1132723008/diff/20001/src/array.js#newcode572 src/array.js:572: if (!IS_UNDEFINED(current_i) || i in array) { On 2015/05/19 16:56:20, dehrenberg wrote: > On 2015/05/19 16:37:25, arv wrote: > > Add todos to remove the in checks for typed arrays. > > > > ...and also typed arrays cannot contain undefined or holes > > > > At this point reverse is probably better reimplemented. > > All the array methods have tons of code to deal with holes. I don't think it'd > be useful to sprinkle todos everywhere. I'm not even positive if all of these > are performance-sensitive--for example, you could optimize join to not handle > holes, but I don't think any program would get faster as a result. What I've > been doing in this patch series is just getting methods to work at a basic level > on TypedArrays, maybe the most useful part being that there are tests. We will > optimize afterwards. I am not sure if reverse() comes up as something that's > important to optimize, but we'll have to see by examining real programs or > thinking about it more. In this particular case, I think the lack of a cloned > method body would a much bigger performance effect than these little > conditionals on holes, but that's just a guess. Very good point.
The CQ bit was checked by dehrenberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132723008/20001
The CQ bit was unchecked by commit-bot@chromium.org
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/5827) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/5924)
The CQ bit was checked by dehrenberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132723008/20001
The CQ bit was unchecked by commit-bot@chromium.org
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/5829)
The CQ bit was checked by dehrenberg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1132723008/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132723008/40001
The CQ bit was unchecked by commit-bot@chromium.org
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/513)
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as cc74268d30074934d7a40dd8256862b45dcbf0d8 (presubmit successful).
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc74268d30074934d7a40dd8256862b45dcbf0d8 Cr-Commit-Position: refs/heads/master@{#28493} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
