Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709773002:
[builtins] (Re-)implement Array.prototype.every/some with the CSA (Closed)
Description[builtins] (Re-)implement Array.prototype.every/some with the CSA
In the process, re-factor the implementation of Array.prototype.forEach so that
the bulk of the implementation can be re-used, since much of the spec is
identical. The refactor should also make it more straight-forward to implement
map and filter. The re-factored version only have a single slow path for processing
elements which is used for both the overall slow path and for the bailout from the
FAST_ELEMENTS case.
Review-Url: https://codereview.chromium.org/2709773002
Cr-Commit-Position: refs/heads/master@{#43745}
Committed: https://chromium.googlesource.com/v8/v8/+/6e0496b256b7beff5132fcb5d6aa220db63258b3
Patch Set 1 #Patch Set 2 : Latest version #Patch Set 3 : Remove stray change #Patch Set 4 : Fix error string #Patch Set 5 : Add some #
Total comments: 23
Patch Set 6 : Review feedback #Patch Set 7 : Review feedback #Patch Set 8 : Simplify name generation #Patch Set 9 : Rebase #
Messages
Total messages: 47 (37 generated)
The CQ bit was checked by danno@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_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by danno@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.
Description was changed from ========== [builtins] Refactor forEach CSA stuf to prepare for Array.prototype.every ========== to ========== [builtins] Refactor forEach CSA machinery to prepare for Array.prototype.every ==========
Description was changed from ========== [builtins] Refactor forEach CSA machinery to prepare for Array.prototype.every ========== to ========== [builtins] (Re-)implement Array.prototype.every with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. ==========
Description was changed from ========== [builtins] (Re-)implement Array.prototype.every with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. ========== to ========== [builtins] (Re-)implement Array.prototype.every/some with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. ==========
danno@chromium.org changed reviewers: + bmeurer@chromium.org, ishell@google.com
Description was changed from ========== [builtins] (Re-)implement Array.prototype.every/some with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. ========== to ========== [builtins] (Re-)implement Array.prototype.every/some with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. The re-factored version only have a single slow path for processing elements which is used for both the overall slow path and for the bailout from the FAST_ELEMENTS case. ==========
ptal
friendly ping
ishell@chromium.org changed reviewers: + ishell@chromium.org
https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:442: BuiltinResultGenerator generator, const X& https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:443: CallResultProcessor processor) { Same here. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:487: s += name; name could just be a "Array.prototype.smth" for simplicity. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:564: CallResultProcessor processor, Node* a, Same here. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:573: [context, kind, this, o, &original_map, callbackfn, this_arg, processor, I think these long capture lists do not contribute to the readablity. How about just [=, &original_map, &last_index] ? https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:694: [this](Node*, Node*) { return UndefinedConstant(); }, [=] https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:706: [this](Node*, Node*) { return TrueConstant(); }, [=] https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:707: [this](Node* a, Node* p_k, Node* value) { Same here. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:724: [this](Node*, Node*) { return FalseConstant(); }, [=] https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:725: [this](Node* a, Node* p_k, Node* value) { Same here. https://codereview.chromium.org/2709773002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (left): https://codereview.chromium.org/2709773002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8384: #ifdef DEBUG Spurious change here and below?
The CQ bit was checked by danno@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.
please take a look https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:442: BuiltinResultGenerator generator, On 2017/03/10 14:14:44, Igor Sheludko wrote: > const X& Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:443: CallResultProcessor processor) { On 2017/03/10 14:14:45, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:487: s += name; On 2017/03/10 14:14:45, Igor Sheludko wrote: > name could just be a "Array.prototype.smth" for simplicity. Unfortunately, these must match the method name exactly due to various tests that look at the exact Excaption that is generated at this failure. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:564: CallResultProcessor processor, Node* a, On 2017/03/10 14:14:45, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:573: [context, kind, this, o, &original_map, callbackfn, this_arg, processor, On 2017/03/10 14:14:44, Igor Sheludko wrote: > I think these long capture lists do not contribute to the readablity. > How about just [=, &original_map, &last_index] ? Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:694: [this](Node*, Node*) { return UndefinedConstant(); }, On 2017/03/10 14:14:44, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:706: [this](Node*, Node*) { return TrueConstant(); }, On 2017/03/10 14:14:44, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:707: [this](Node* a, Node* p_k, Node* value) { On 2017/03/10 14:14:44, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:724: [this](Node*, Node*) { return FalseConstant(); }, On 2017/03/10 14:14:44, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:725: [this](Node* a, Node* p_k, Node* value) { On 2017/03/10 14:14:44, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2709773002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (left): https://codereview.chromium.org/2709773002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8384: #ifdef DEBUG On 2017/03/10 14:14:45, Igor Sheludko wrote: > Spurious change here and below? Done.
The CQ bit was checked by danno@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.
lgtm https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2709773002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:487: s += name; On 2017/03/13 07:22:06, danno wrote: > On 2017/03/10 14:14:45, Igor Sheludko wrote: > > name could just be a "Array.prototype.smth" for simplicity. > > Unfortunately, these must match the method name exactly due to various tests > that look at the exact Excaption that is generated at this failure. I meant to pass full method name as a parameter instead of computing it here.
The CQ bit was checked by danno@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 danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2709773002/#ps140001 (title: "Simplify name generation")
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36440)
The CQ bit was checked by danno@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...
On 2017/03/13 12:07:37, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on master.tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36440) lgtm.
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 danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2709773002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489409522122660,
"parent_rev": "09de9969ccb9bc3bbd667788afad665b309d02f5", "commit_rev":
"6e0496b256b7beff5132fcb5d6aa220db63258b3"}
Message was sent while issue was closed.
Description was changed from ========== [builtins] (Re-)implement Array.prototype.every/some with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. The re-factored version only have a single slow path for processing elements which is used for both the overall slow path and for the bailout from the FAST_ELEMENTS case. ========== to ========== [builtins] (Re-)implement Array.prototype.every/some with the CSA In the process, re-factor the implementation of Array.prototype.forEach so that the bulk of the implementation can be re-used, since much of the spec is identical. The refactor should also make it more straight-forward to implement map and filter. The re-factored version only have a single slow path for processing elements which is used for both the overall slow path and for the bailout from the FAST_ELEMENTS case. Review-Url: https://codereview.chromium.org/2709773002 Cr-Commit-Position: refs/heads/master@{#43745} Committed: https://chromium.googlesource.com/v8/v8/+/6e0496b256b7beff5132fcb5d6aa220db63... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/v8/v8/+/6e0496b256b7beff5132fcb5d6aa220db63... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
