Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775503006:
[builtins] Improve performance of array.prototype.filter and map (Closed)
Description[builtins] Improve performance of array.prototype.filter and map.
BUG=
Review-Url: https://codereview.chromium.org/2775503006
Cr-Commit-Position: refs/heads/master@{#44793}
Committed: https://chromium.googlesource.com/v8/v8/+/1eb0ef316103caf526f9ab80290b5ba313e232af
Patch Set 1 #Patch Set 2 : Small fix. #Patch Set 3 : Some refactoring. #Patch Set 4 : Rebase to include map. #Patch Set 5 : Incorrect mode used. #Patch Set 6 : REBASE. #Patch Set 7 : compile fix. #Patch Set 8 : Confusion with Parameter mode fix. #Patch Set 9 : Remove old impl. #
Total comments: 10
Patch Set 10 : fixes #
Total comments: 6
Patch Set 11 : Fixes and rebase. #Patch Set 12 : Comment response. #
Total comments: 2
Patch Set 13 : Code comments #
Messages
Total messages: 61 (45 generated)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mvstanton@chromium.org changed reviewers: + danno@chromium.org
Hi Danno, Here is the basic CL with filter and map performance improvements. I'll pull the js-perf-test code out into another CL which will also include map performance tests. Thanks for the look! --Michael
Description was changed from ========== [builtins] Make array.prototype.filter have nice performance. BUG= ========== to ========== [builtins] Improve performance of array.prototype.filter and map. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Danno, Okay *now* it's actually ready, sorry for the bug..!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23672) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/25348) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mvstanton@chromium.org changed reviewers: + yangguo@chromium.org
Hi Danno, hi Yang: Yang, could you have a look at array.js and bootstrapper.cc? Danno, have a look at the rest. I went ahead and brought this out from behind the --experimental-fast-array-builtins flag, as there are issues with the typed array some/each test262 tests (Tobias said a fix is coming soon). Plus, this implementation is an improvement over the array.js version of map and filter, so we may as well go with it. Thank you! --Michael
On 2017/04/03 13:39:26, mvstanton wrote: > Hi Danno, hi Yang: > > Yang, could you have a look at array.js and bootstrapper.cc? > Danno, have a look at the rest. > > I went ahead and brought this out from behind the > --experimental-fast-array-builtins flag, as there are issues with the typed > array some/each test262 tests (Tobias said a fix is coming soon). Plus, this > implementation is an improvement over the array.js version of map and filter, so > we may as well go with it. > > Thank you! > --Michael lgtm
Cool! Getting there. First round of feedback. https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:141: if (o_kind < DICTIONARY_ELEMENTS) { Please use the predicates in elements-kind.h, e.g. IsFastElementKind(). https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:147: Label object_push_pre(this), object_push(this), double_push(this); I think you can make this slightly better. Since you have the elements kind, you only have to generate the code for either FAST_ELEMENTS or FAST_DOUBLE_ELEMENTS at compile time. https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:227: GotoIf(Int32GreaterThan(kind, Int32Constant(FAST_HOLEY_SMI_ELEMENTS)), Consider adding/using existing predicates in code-stub-assembler.h like "IsFastElementsKind" and "IsHoleyFastElementsKind". https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:521: a_.Bind(processor(this, DICTIONARY_ELEMENTS, k_value, k())); Although I know what you mean here, the elements may not be dictionary at this point. In fact, they can be anything. I am leaning towards adding a new ElementsKind kind of like "NO_ELEMENTS", but called "UNKNOWN_ELEMENTS" and to pass that here. https://codereview.chromium.org/2775503006/diff/160001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2775503006/diff/160001/src/code-stub-assemble... src/code-stub-assembler.cc:1588: Node* growth = IntPtrOrSmiConstant(1, mode); This prologue and grow code looks quite similar between the two versions of BuildAppendJSArray. Could you perhaps factor it out too? (parameterized of course by the amount to grow)? https://codereview.chromium.org/2775503006/diff/160001/src/js/array.js File src/js/array.js (left): https://codereview.chromium.org/2775503006/diff/160001/src/js/array.js#oldcod... src/js/array.js:1054: Nice. I like it.
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Danno, here, comments addressed. https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:141: if (o_kind < DICTIONARY_ELEMENTS) { On 2017/04/03 15:35:27, danno wrote: > Please use the predicates in elements-kind.h, e.g. IsFastElementKind(). Per our discussion, I'll just remove the o_kind parameter (still, good suggestion!). https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:147: Label object_push_pre(this), object_push(this), double_push(this); On 2017/04/03 15:35:27, danno wrote: > I think you can make this slightly better. Since you have the elements kind, you > only have to generate the code for either FAST_ELEMENTS or FAST_DOUBLE_ELEMENTS > at compile time. Right, because of the vagaries of what can happen with the species constructor, I'm compelled to handle all cases. There might be some improvements available at the margin for later. https://codereview.chromium.org/2775503006/diff/160001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:227: GotoIf(Int32GreaterThan(kind, Int32Constant(FAST_HOLEY_SMI_ELEMENTS)), On 2017/04/03 15:35:27, danno wrote: > Consider adding/using existing predicates in code-stub-assembler.h like > "IsFastElementsKind" and "IsHoleyFastElementsKind". I use a "greater than" metaphor to avoid multiple comparisons. https://codereview.chromium.org/2775503006/diff/160001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2775503006/diff/160001/src/code-stub-assemble... src/code-stub-assembler.cc:1588: Node* growth = IntPtrOrSmiConstant(1, mode); On 2017/04/03 15:35:27, danno wrote: > This prologue and grow code looks quite similar between the two versions of > BuildAppendJSArray. Could you perhaps factor it out too? (parameterized of > course by the amount to grow)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
Got a couple of drive-by nits inline (but looks good to me otherwise): https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1602: Variable var_tagged_length(this, MachineRepresentation::kTagged); Nit: VARIABLE(var_x, ...) instead of Variable var_x(this, ...) and BIND(&label) instead of Bind(&label) everywhere. https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1616: Node* new_length = IntPtrOrSmiAdd(growth, var_length.value(), mode); Nit: Leftover code? https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1635: var_tagged_length.Bind(length); Nit: var_tagged_length and the success label aren't needed.
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Jakob, those were great comments! I finally dealt with confusion among SMI/INT_PTR representation under BuildAppendJSArray(). I think I need another round of comments from Danno, PTAL, thanks! --Michael https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1602: Variable var_tagged_length(this, MachineRepresentation::kTagged); On 2017/04/19 10:25:28, jgruber wrote: > Nit: VARIABLE(var_x, ...) instead of Variable var_x(this, ...) and BIND(&label) > instead of Bind(&label) everywhere. Done. https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1616: Node* new_length = IntPtrOrSmiAdd(growth, var_length.value(), mode); On 2017/04/19 10:25:28, jgruber wrote: > Nit: Leftover code? Done. https://codereview.chromium.org/2775503006/diff/180001/src/code-stub-assemble... src/code-stub-assembler.cc:1635: var_tagged_length.Bind(length); On 2017/04/19 10:25:28, jgruber wrote: > Nit: var_tagged_length and the success label aren't needed. Good idea, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comment https://codereview.chromium.org/2775503006/diff/220001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2775503006/diff/220001/src/code-stub-assemble... src/code-stub-assembler.h:682: // followed for allocation failure. Label |fits| is followed if current nit: There is no label |fits|?
The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run
The CQ bit was checked by mvstanton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org, danno@chromium.org Link to the patchset: https://codereview.chromium.org/2775503006/#ps240001 (title: "Code comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed outdated comment...landin'.. https://codereview.chromium.org/2775503006/diff/220001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2775503006/diff/220001/src/code-stub-assemble... src/code-stub-assembler.h:682: // followed for allocation failure. Label |fits| is followed if current On 2017/04/24 12:12:11, danno wrote: > nit: There is no label |fits|? Ah! Outdated comment - I fixed the wording. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493036462000780,
"parent_rev": "86ba46613354a7cc23a5958340f9982bdb98f352", "commit_rev":
"1eb0ef316103caf526f9ab80290b5ba313e232af"}
Message was sent while issue was closed.
Description was changed from ========== [builtins] Improve performance of array.prototype.filter and map. BUG= ========== to ========== [builtins] Improve performance of array.prototype.filter and map. BUG= Review-Url: https://codereview.chromium.org/2775503006 Cr-Commit-Position: refs/heads/master@{#44793} Committed: https://chromium.googlesource.com/v8/v8/+/1eb0ef316103caf526f9ab80290b5ba313e... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/v8/v8/+/1eb0ef316103caf526f9ab80290b5ba313e...
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
This CL seems to be causing failures on the arm64 nosnap builder: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... I can reproduce failures in both cctest/test-spaces/InlineAllocationObserverCadence and cctest/test-unboxed-doubles/Regress436816 It looks like these may be tickling something unrelated to this CL, though, as there's clearly nothing arm64-specific here.
Message was sent while issue was closed.
As a side-note, this CL could have used a more full-fledged description, and a link to a tracking bug if appropriate (https://bugs.chromium.org/p/v8/issues/detail?id=1956 for example).
Message was sent while issue was closed.
On 2017/04/24 22:48:01, adamk wrote: > This CL seems to be causing failures on the arm64 nosnap builder: > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... > > I can reproduce failures in both > cctest/test-spaces/InlineAllocationObserverCadence and > cctest/test-unboxed-doubles/Regress436816 > > It looks like these may be tickling something unrelated to this CL, though, as > there's clearly nothing arm64-specific here. I can reproduce both of these failures on x64 debug nosnap (no arm64 required). Still think it's unrelated to this CL, though.
Message was sent while issue was closed.
Sorry for the poor CL description! Normally I try to do better here... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
