Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543483003:
[regexp] Skip result construction in test, @@match, @@search (Closed)
|
Created:
4 years ago by jgruber Modified:
4 years ago Reviewers:
Igor Sheludko CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[regexp] Skip result construction in test, @@match, @@search
We can skip RegExpResult construction on the fast path for several functions to
be more efficient.
BUG=v8:5330, v8:5674
Committed: https://crrev.com/360b7668eae78dbe761ccf28c6b7585af0aea548
Cr-Commit-Position: refs/heads/master@{#41418}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address comments #Patch Set 3 : Remove unused variables #
Depends on Patchset: Messages
Total messages: 20 (12 generated)
The CQ bit was checked by jgruber@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.
jgruber@chromium.org changed reviewers: + ishell@chromium.org
https://codereview.chromium.org/2543483003/diff/1/test/js-perf-test/RegExp/ba... File test/js-perf-test/RegExp/base_ctor.js (right): https://codereview.chromium.org/2543483003/diff/1/test/js-perf-test/RegExp/ba... test/js-perf-test/RegExp/base_ctor.js:29: class SubRegExp extends RegExp { Ignore this, it's part of https://codereview.chromium.org/2537253003/ which has already landed.
lgtm with nits: https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:229: Node* LoadMatchInfoField(CodeStubAssembler* a, Node* const match_info, I think you should add CSA::LoadFieldArrayElement(Node* array, int index, int additional_offset = 0) instead. Just like we added CSA::StoreFieldArrayElement(Node* array, int index, ...) recently. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:243: Node* const num_indices = a->SmiUntag(a->LoadFixedArrayElement( LoadMatchInfoField (or new LoadFixedArrayElement) here and below. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:421: Node* const new_lastindex = a->LoadFixedArrayElement( Same here. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1524: void Generate_RegExpPrototypeSearchBodyFast(CodeStubAssembler* a, I think you can drop Generate_ prefixes from helper functions. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1613: exec_result, JSRegExpResult::kIndexOffset, MachineType::AnyTagged()); AnyTagged() is default, you can drop it for readability.
https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:229: Node* LoadMatchInfoField(CodeStubAssembler* a, Node* const match_info, On 2016/11/30 23:17:22, Igor Sheludko wrote: > I think you should add CSA::LoadFieldArrayElement(Node* array, int index, int > additional_offset = 0) instead. Just like we added > CSA::StoreFieldArrayElement(Node* array, int index, ...) recently. Will do this in a follow-up CL since it also affects code all over the place. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:243: Node* const num_indices = a->SmiUntag(a->LoadFixedArrayElement( On 2016/11/30 23:17:22, Igor Sheludko wrote: > LoadMatchInfoField (or new LoadFixedArrayElement) here and below. Done. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:421: Node* const new_lastindex = a->LoadFixedArrayElement( On 2016/11/30 23:17:22, Igor Sheludko wrote: > Same here. Done and at all other places as well. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1524: void Generate_RegExpPrototypeSearchBodyFast(CodeStubAssembler* a, On 2016/11/30 23:17:22, Igor Sheludko wrote: > I think you can drop Generate_ prefixes from helper functions. Done. https://codereview.chromium.org/2543483003/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1613: exec_result, JSRegExpResult::kIndexOffset, MachineType::AnyTagged()); On 2016/11/30 23:17:22, Igor Sheludko wrote: > AnyTagged() is default, you can drop it for readability. Done.
The CQ bit was checked by jgruber@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/2543483003/#ps20001 (title: "Address comments")
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_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by jgruber@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/2543483003/#ps40001 (title: "Remove unused variables")
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": 40001, "attempt_start_ts": 1480586618507540,
"parent_rev": "dcc387d2ec19d34794338780c9e463debc6d03f4", "commit_rev":
"ea0eb75ef835ba20974eb1a69c5000c5eeca968e"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Skip result construction in test, @@match, @@search We can skip RegExpResult construction on the fast path for several functions to be more efficient. BUG=v8:5330,v8:5674 ========== to ========== [regexp] Skip result construction in test, @@match, @@search We can skip RegExpResult construction on the fast path for several functions to be more efficient. BUG=v8:5330,v8:5674 Committed: https://crrev.com/360b7668eae78dbe761ccf28c6b7585af0aea548 Cr-Commit-Position: refs/heads/master@{#41418} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/360b7668eae78dbe761ccf28c6b7585af0aea548 Cr-Commit-Position: refs/heads/master@{#41418} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
