Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527963002:
[regexp] Migrate @@match to TurboFan (Closed)
|
Created:
4 years ago by jgruber Modified:
4 years ago Reviewers:
Igor Sheludko CC:
v8-reviews_googlegroups.com, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[regexp] Migrate @@match to TurboFan
Microbenchmarks show a 4x improvement on the fast path and 2.5x improvement on
the slow path when compared to the CPP builtin implementation.
Compared to the old JS implementation, the fast path is 20% faster and the slow
path 35% slower.
BUG=v8:5339, v8:5562
Committed: https://crrev.com/4e7571a5a9bb8563ae93e0aff48c08bead765b14
Cr-Commit-Position: refs/heads/master@{#41338}
Patch Set 1 #Patch Set 2 : Initialize initial fixed array elements #Patch Set 3 : Update call to StoreFixedArrayElement for new signature oO #Patch Set 4 : Fix match fetching on slow path #Patch Set 5 : Adapt arguments #
Total comments: 13
Patch Set 6 : Address comments #Patch Set 7 : Remove unused variable #
Messages
Total messages: 39 (31 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: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16832) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
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: 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...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
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: 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 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: 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 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...
jgruber@chromium.org changed reviewers: + ishell@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits: https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1176: CodeStubAssembler::ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; |mode| should be passed from outside for consistency. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1197: is_fastpath ? FastFlagGetter(a, regexp, JSRegExp::kGlobal) Maybe it's worth adding FlagGetter(..., bool is_fastpath). https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1276: BranchIfFastRegExpResult(a, context, a->LoadMap(result), &fast_result, Is it possible that the RegExpPrototypeExecInternal returns a slow result object? If not then we can deal only with fast results on the fast path. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1292: Node* const name = a->HeapConstant( You can use SmiConstant(0) here. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1294: Callable getproperty_callable = CodeFactory::GetProperty(isolate); // TODO(ishell): Use GetElement stub once it's available. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1304: // The match is guaranteed to be a string on the fast path. CSA_ASSERT that? https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1469: BranchIfFastRegExpResult(a, context, a->LoadMap(match_indices), Same here.
lgtm with nits:
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...
Thanks for the review. Addressed all comments, and additionally refactored fixed array handling into a GrowableFixedArray class which wraps growth, access and JSArray creation. PTAL. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1176: CodeStubAssembler::ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; On 2016/11/25 17:31:27, Igor Sheludko wrote: > |mode| should be passed from outside for consistency. Done. Refactored growable array handling into a GrowableFixedArray class since the same code will also be used in an upcoming CL for @@split. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1197: is_fastpath ? FastFlagGetter(a, regexp, JSRegExp::kGlobal) On 2016/11/25 17:31:27, Igor Sheludko wrote: > Maybe it's worth adding FlagGetter(..., bool is_fastpath). Done. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1276: BranchIfFastRegExpResult(a, context, a->LoadMap(result), &fast_result, On 2016/11/25 17:31:27, Igor Sheludko wrote: > Is it possible that the RegExpPrototypeExecInternal returns a slow result > object? If not then we can deal only with fast results on the fast path. Done. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1294: Callable getproperty_callable = CodeFactory::GetProperty(isolate); On 2016/11/25 17:31:27, Igor Sheludko wrote: > // TODO(ishell): Use GetElement stub once it's available. Done. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1304: // The match is guaranteed to be a string on the fast path. On 2016/11/25 17:31:27, Igor Sheludko wrote: > CSA_ASSERT that? Done. https://codereview.chromium.org/2527963002/diff/80001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1469: BranchIfFastRegExpResult(a, context, a->LoadMap(match_indices), On 2016/11/25 17:31:27, Igor Sheludko wrote: > Same here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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...
lgtm
The CQ bit was unchecked by jgruber@chromium.org
The CQ bit was checked by jgruber@chromium.org
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": 120001, "attempt_start_ts": 1480409505618630,
"parent_rev": "77e56f6075f5cb29f9908a9df062d3f939724377", "commit_rev":
"45baa0364606f9d6b96f9ef52d9931a7bc56afca"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Migrate @@match to TurboFan Microbenchmarks show a 4x improvement on the fast path and 2.5x improvement on the slow path when compared to the CPP builtin implementation. Compared to the old JS implementation, the fast path is 20% faster and the slow path 35% slower. BUG=v8:5339,v8:5562 ========== to ========== [regexp] Migrate @@match to TurboFan Microbenchmarks show a 4x improvement on the fast path and 2.5x improvement on the slow path when compared to the CPP builtin implementation. Compared to the old JS implementation, the fast path is 20% faster and the slow path 35% slower. BUG=v8:5339,v8:5562 Committed: https://crrev.com/4e7571a5a9bb8563ae93e0aff48c08bead765b14 Cr-Commit-Position: refs/heads/master@{#41338} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4e7571a5a9bb8563ae93e0aff48c08bead765b14 Cr-Commit-Position: refs/heads/master@{#41338} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
