Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551443002:
[regexp] Migrate constructor and compile to CSA (Closed)
Description[regexp] Migrate constructor and compile to CSA
Microbenchmarks show 25% improvement over C++, 11% improvement over JS
for the constructor. We don't have a microbenchmark covering the compile
method.
Locally, octane/regexp improved by 2%.
BUG=v8:5339
Committed: https://crrev.com/28cc20eeadcfd59f0c050b5dec18c2781be1eb33
Cr-Commit-Position: refs/heads/master@{#41490}
Patch Set 1 #Patch Set 2 : Remove unused variables #
Total comments: 28
Patch Set 3 : Address comments #
Total comments: 8
Patch Set 4 : Address comments #
Messages
Total messages: 34 (24 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_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...
Description was changed from ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ========== to ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the (deprecated) compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ==========
Description was changed from ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the (deprecated) compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ========== to ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ==========
jgruber@chromium.org changed reviewers: + cbruni@chromium.org
First part of a series. 1. RegExp ctor and compile 2. New TF_BUILTIN style Pt. 1 (simple 1:1 conversion) 3. New TF_BUILTIN style Pt. 2 (move helpers to REAssembler)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ishell@chromium.org changed reviewers: + ishell@chromium.org
First round of comments. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:457: #define CASE_FOR_FLAG(FLAG, LABEL, NEXT_LABEL) \ While you are here, I think you can simplify the macro by removing the labels: { Label next; // ... GotoIf(, &next); // ... Goto(&next); Bind(&next); } https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:461: a->GotoIf(a->WordEqual(a->WordAnd(flags_intptr, mask), int_zero), \ You need something like IsWordSet(flags_intptr, FLAG) here. You can either add IsWordSet(Node* word, intptr_t mask) similar to existing IsWordSet32() or use the latter if you switch untag flags field to word32. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:484: #define CASE_FOR_FLAG(NAME, FLAG, LABEL, NEXT_LABEL) \ Same here. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:488: a->HeapConstant(isolate->factory()->NewStringFromAsciiChecked(NAME)); \ You can speed up the slow lookups a bit if you create an internalized strings. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:526: #define CASE_FOR_FLAG(FLAG, CHAR, LABEL, NEXT_LABEL) \ Same here. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:530: a->GotoIf(a->WordEqual(a->WordAnd(flags_intptr, mask), int_zero), \ IsSetWord() https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:602: a->Branch(a->IsUndefined(maybe_pattern), &if_isundefined, &if_notundefined); Add a comment // TODO(ishell): Use Select() once it supports lambdas. I'm adding such a Select. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:623: a->Branch(a->IsUndefined(maybe_flags), &if_isundefined, &if_notundefined); Same here.
https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:712: Callable getproperty_callable = CodeFactory::GetProperty(isolate); I guess you are going to optimize it further in a follow up CL? https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:728: &if_patternisfastregexp); The instance type check only tells you that the pattern is a JSRegExp but it could be a slow-mode object. Please fix the fastpath block or the check. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:734: Node* const source = a.LoadObjectField(pattern, JSRegExp::kSourceOffset); Here. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:741: Node* const value = FlagsGetter(&a, pattern, context, true); And here. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:856: Node* const new_flags = FlagsGetter(&a, pattern, context, true); Same here.
https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:602: a->Branch(a->IsUndefined(maybe_pattern), &if_isundefined, &if_notundefined); On 2016/12/05 09:35:51, Igor Sheludko wrote: > Add a comment > // TODO(ishell): Use Select() once it supports lambdas. > I'm adding such a Select. The new Select() is landed and you can use it!
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. Comments addressed, PTAL. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:457: #define CASE_FOR_FLAG(FLAG, LABEL, NEXT_LABEL) \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > While you are here, I think you can simplify the macro by removing the labels: > > { > Label next; > // ... > GotoIf(, &next); > // ... > Goto(&next); > Bind(&next); > } Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:461: a->GotoIf(a->WordEqual(a->WordAnd(flags_intptr, mask), int_zero), \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > You need something like IsWordSet(flags_intptr, FLAG) here. > > You can either add IsWordSet(Node* word, intptr_t mask) similar to existing > IsWordSet32() or use the latter if you switch untag flags field to word32. Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:484: #define CASE_FOR_FLAG(NAME, FLAG, LABEL, NEXT_LABEL) \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:488: a->HeapConstant(isolate->factory()->NewStringFromAsciiChecked(NAME)); \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > You can speed up the slow lookups a bit if you create an internalized strings. Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:526: #define CASE_FOR_FLAG(FLAG, CHAR, LABEL, NEXT_LABEL) \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:530: a->GotoIf(a->WordEqual(a->WordAnd(flags_intptr, mask), int_zero), \ On 2016/12/05 09:35:51, Igor Sheludko wrote: > IsSetWord() Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:602: a->Branch(a->IsUndefined(maybe_pattern), &if_isundefined, &if_notundefined); On 2016/12/05 09:35:51, Igor Sheludko wrote: > Add a comment > // TODO(ishell): Use Select() once it supports lambdas. > I'm adding such a Select. Nice, I liike! https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:623: a->Branch(a->IsUndefined(maybe_flags), &if_isundefined, &if_notundefined); On 2016/12/05 09:35:51, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:712: Callable getproperty_callable = CodeFactory::GetProperty(isolate); On 2016/12/05 11:28:14, Igor Sheludko wrote: > I guess you are going to optimize it further in a follow up CL? Only if it seems necessary. The RegExp constructor is a fairly low priority target since most regexps are constructed through literals, which take another code path. Plus, we already have a fairly significant speedup over the old JS version with this CL. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:728: &if_patternisfastregexp); On 2016/12/05 11:28:14, Igor Sheludko wrote: > The instance type check only tells you that the pattern is a JSRegExp but it > could be a slow-mode object. > Please fix the fastpath block or the check. Yup, I'm aware of that, and this implements semantics as specced (see https://tc39.github.io/ecma262/#sec-regexp-pattern-flags, step 4). The instance type check is equivalent to 'has a [[RegExpMatcher]] internal slot', and in that case we always access the internal [[OriginalSource]] and [[OriginalFlags]] fields, which correspond to the fast flags and source accesses below. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:734: Node* const source = a.LoadObjectField(pattern, JSRegExp::kSourceOffset); On 2016/12/05 11:28:14, Igor Sheludko wrote: > Here. See above. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:741: Node* const value = FlagsGetter(&a, pattern, context, true); On 2016/12/05 11:28:14, Igor Sheludko wrote: > And here. See above. https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:856: Node* const new_flags = FlagsGetter(&a, pattern, context, true); On 2016/12/05 11:28:14, Igor Sheludko wrote: > Same here. See above.
lgtm with nits: https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:728: &if_patternisfastregexp); On 2016/12/05 12:28:48, jgruber wrote: > On 2016/12/05 11:28:14, Igor Sheludko wrote: > > The instance type check only tells you that the pattern is a JSRegExp but it > > could be a slow-mode object. > > Please fix the fastpath block or the check. > > Yup, I'm aware of that, and this implements semantics as specced (see > https://tc39.github.io/ecma262/#sec-regexp-pattern-flags, step 4). The instance > type check is equivalent to 'has a [[RegExpMatcher]] internal slot', and in that > case we always access the internal [[OriginalSource]] and [[OriginalFlags]] > fields, which correspond to the fast flags and source accesses below. Ah. Sorry. I missed the fact that they are already stored in the header. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:580: a->IsUndefined(maybe_pattern), [&] { return a->EmptyStringConstant(); }, You don't need reference access to the captured variables. Use [=] here. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:581: [&] { return a->ToString(context, maybe_pattern); }, Same here. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:586: a->IsUndefined(maybe_flags), [&] { return a->EmptyStringConstant(); }, Same here. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:587: [&] { return a->ToString(context, maybe_flags); }, And here.
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 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...
https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:580: a->IsUndefined(maybe_pattern), [&] { return a->EmptyStringConstant(); }, On 2016/12/05 12:43:41, Igor Sheludko wrote: > You don't need reference access to the captured variables. Use [=] here. Done. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:581: [&] { return a->ToString(context, maybe_pattern); }, On 2016/12/05 12:43:40, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:586: a->IsUndefined(maybe_flags), [&] { return a->EmptyStringConstant(); }, On 2016/12/05 12:43:40, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2551443002/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:587: [&] { return a->ToString(context, maybe_flags); }, On 2016/12/05 12:43:40, Igor Sheludko wrote: > And here. Done.
The CQ bit was unchecked by jgruber@chromium.org
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/2551443002/#ps60001 (title: "Address comments")
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": 60001, "attempt_start_ts": 1480945526349080,
"parent_rev": "d0d315164ef0c0d5a29403432ed57f7976a2ba00", "commit_rev":
"71cb1b37d627915436c32e221b3a0dcdff52feda"}
Message was sent while issue was closed.
Description was changed from ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ========== to ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 ========== to ========== [regexp] Migrate constructor and compile to CSA Microbenchmarks show 25% improvement over C++, 11% improvement over JS for the constructor. We don't have a microbenchmark covering the compile method. Locally, octane/regexp improved by 2%. BUG=v8:5339 Committed: https://crrev.com/28cc20eeadcfd59f0c050b5dec18c2781be1eb33 Cr-Commit-Position: refs/heads/master@{#41490} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/28cc20eeadcfd59f0c050b5dec18c2781be1eb33 Cr-Commit-Position: refs/heads/master@{#41490} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
