Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2438683005:
[regexp] Move RegExp.prototype[@@search] to TF (Closed)
Description[regexp] Move RegExp.prototype[@@search] to TF
Implements upcoming changes to @@search according to
https://github.com/tc39/ecma262/pull/627.
This also adds SameValue to CodeStubAssembler and extracts a part of
CSA::TruncateTaggedToFloat64.
BUG=v8:5339
Committed: https://crrev.com/e29fcbee9cebada7423eee79a77bc91a320588be
Cr-Commit-Position: refs/heads/master@{#41000}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix typo in lastindex restoration #
Total comments: 6
Patch Set 4 : SameValue, separate slow and fast paths #Patch Set 5 : Remove unused variables #Patch Set 6 : Refactor SameValue #Patch Set 7 : [regexp] Move RegExp.prototype[@@search] to TF #
Total comments: 4
Patch Set 8 : Rename to TryTaggedToFloat64 #
Total comments: 35
Patch Set 9 : Address comments #Patch Set 10 : Rebase #
Messages
Total messages: 76 (56 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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27056) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16560)
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...
jgruber@chromium.org changed reviewers: + yangguo@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (left): https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1179: if (is_last_index_unchanged.IsNothing()) return isolate->pending_exception(); This does not seem correct. To throw an exception we do not return the pending exception object, but the isolate->heap()->exception() sentinel. I know this is being removed, but having this suggests that we don't have a test case for this. https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1183: StoreLastIndex(a, context, has_initialmap, receiver, smi_zero); The spec actually suggests to always set the last index to 0, regardless of what the previous value was. This is an observable difference. Also, below we check for IsInitialRegExpMap because it might have changed. This could happen here too, if the regexp does not have the initial map and LoadLastIndex triggered side effects to change the map. I suggest having one fast path with has_initial_map, because then we can be certain that nothing observable can happen. We would not need to recheck IsInitialRegExpMap along this fast path. And we would not need LoadLastIndex, since it's going to be overwritten and not-observable anyways, then always StoreLastIndex to 0. The slow path would not need to do the IsInitialRegExpMap along the way either, since we would just know that it would be false. https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1200: LoadLastIndex(a, context, has_initialmap, receiver); Again, the spec suggests to always overwrite the last index, and not load at all.
https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (left): https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1179: if (is_last_index_unchanged.IsNothing()) return isolate->pending_exception(); On 2016/10/25 07:30:13, Yang wrote: > This does not seem correct. To throw an exception we do not return the pending > exception object, but the isolate->heap()->exception() sentinel. I know this is > being removed, but having this suggests that we don't have a test case for this. Great catch. Looking at this further, we actually need to do more work here - the spec uses SameValue(x, y) so a simple identity comparison is not enough unfortunately. Will also add a test case. https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1183: StoreLastIndex(a, context, has_initialmap, receiver, smi_zero); On 2016/10/25 07:30:12, Yang wrote: > The spec actually suggests to always set the last index to 0, regardless of what > the previous value was. This is an observable difference. > > Also, below we check for IsInitialRegExpMap because it might have changed. This > could happen here too, if the regexp does not have the initial map and > LoadLastIndex triggered side effects to change the map. > > I suggest having one fast path with has_initial_map, because then we can be > certain that nothing observable can happen. We would not need to recheck > IsInitialRegExpMap along this fast path. And we would not need LoadLastIndex, > since it's going to be overwritten and not-observable anyways, then always > StoreLastIndex to 0. > > The slow path would not need to do the IsInitialRegExpMap along the way either, > since we would just know that it would be false. The spec has changed fairly recently to avoid unnecessary writes to lastIndex: https://github.com/tc39/ecma262/pull/627 This CL implements those semantics. Agreed on the separate fast/slow path implementations (also had this as a TODO above). Good catch on the missing map checks. Will go with that solution for the next patch set. https://codereview.chromium.org/2438683005/diff/40001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:1200: LoadLastIndex(a, context, has_initialmap, receiver); On 2016/10/25 07:30:12, Yang wrote: > Again, the spec suggests to always overwrite the last index, and not load at > all. The spec has changed recently (see above).
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...
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...
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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16040) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16082) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28332)
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
Rewrote this to use SameValue (as specced) and to have separate fast/slow paths. +ishell for changes in CSA. +bmeurer for +-0.0 handling. Let me know if you'd prefer CSA changes in a separate CL. PTAL.
Description was changed from ========== [regexp] Move RegExp.prototype[@@search] to TF BUG=v8:5339 ========== to ========== [regexp] Move RegExp.prototype[@@search] to TF This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/2438683005/diff/120001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2438683005/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:2279: Node* CodeStubAssembler::TruncateNumberToFloat64(Node* value, This name is confusing as the function doesn't truncate. It's more like TryTaggedToFloat64 or something. https://codereview.chromium.org/2438683005/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:8458: Node* const lhs_float = TruncateNumberToFloat64(lhs, &strict_equal); Truncate is highly confusing here, since you don't truncate.
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/2438683005/diff/120001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2438683005/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:2279: Node* CodeStubAssembler::TruncateNumberToFloat64(Node* value, On 2016/11/11 10:25:35, Benedikt Meurer wrote: > This name is confusing as the function doesn't truncate. It's more like > TryTaggedToFloat64 or something. Done. https://codereview.chromium.org/2438683005/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:8458: Node* const lhs_float = TruncateNumberToFloat64(lhs, &strict_equal); On 2016/11/11 10:25:35, Benedikt Meurer wrote: > Truncate is highly confusing here, since you don't truncate. Done.
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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16051) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16012)
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.
https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1192: Node* const previous_last_index = FastLoadLastIndex(a, context, receiver); Given that the only difference between fast and slow paths is the way the lastIndex property is accessed I think it makes sense to factor out the code that generates one branch to a helper function. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1197: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); According to my understanding of a spec we must always call Set. It could be observable if we pass non-regexp receiver with getter/setter lastIndex. For the fast case the store is not observable but I'm not sure how much do you save here given the complexity of a SameValue implementation. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1214: a->GotoIf(a->SameValue(current_last_index, previous_last_index, context), Same here. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1227: a->Return(a->SmiConstant(Smi::FromInt(-1))); SmiConstant(-1) https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1270: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); According to my understanding of a spec we must always call Set. It could be observable if we pass non-regexp receiver with getter/setter lastIndex. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1287: a->GotoIf(a->SameValue(current_last_index, previous_last_index, context), Same here. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1304: // Return the index of the match. I guess in this case the match_indices could still be in fast mode. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2279: Node* CodeStubAssembler::TryTaggedToFloat64(Node* value, Please also this TryTaggedToFloat64() in a kDouble case of PrepareValueForWrite(). https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2283: var_result.Bind(Float64Constant(0.0)); Please remove this bind if the CSA does not complain. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2300: Branch(WordEqual(LoadMap(value), HeapNumberMapConstant()), Branch(IsHeapNumberMap(LoadMap(value))), https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:8468: Select(Float64Equal(rhs_float, rhs_float), int_false, int_true); Select requires also the representation of a result (kTagged by default) which is MachineType::PointerRepresentation() in your case. Otherwise GC may be unhappy.
Thanks for the thorough review Igor, some initial replies on your major points. Will fix the rest in an upcoming CL. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1192: Node* const previous_last_index = FastLoadLastIndex(a, context, receiver); On 2016/11/11 16:13:21, Igor Sheludko wrote: > Given that the only difference between fast and slow paths is the way the > lastIndex property is accessed I think it makes sense to factor out the code > that generates one branch to a helper function. That helper function exists and is used elsewhere (also in the first version of this CL). The reason we now use separate fast and slow paths is that merging the two paths would require repeated map checks to ensure we're still on the fast path. Looking at this again, we can also skip the call to RegExpExec on the fast path, calling directly into RegExpPrototypeExecInternal instead. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1197: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); On 2016/11/11 16:13:20, Igor Sheludko wrote: > According to my understanding of a spec we must always call Set. It could be > observable if we pass non-regexp receiver with getter/setter lastIndex. Changes to @@search are about to be merged into the spec, and this CL implements those semantics. See https://github.com/tc39/ecma262/pull/627. That pull request ensures Set is only called when necessary. Sorry about not making this clearer. I discussed it in reply to Yang's comments above, but I'll add it to this CL's commit message. > For the fast case the store is not observable but I'm not sure how much do you > save here given the complexity of a SameValue implementation. True - but the common case, when both args to SameValue are Smis, should skip most of the complication. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1214: a->GotoIf(a->SameValue(current_last_index, previous_last_index, context), On 2016/11/11 16:13:20, Igor Sheludko wrote: > Same here. See above. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1227: a->Return(a->SmiConstant(Smi::FromInt(-1))); On 2016/11/11 16:13:20, Igor Sheludko wrote: > SmiConstant(-1) Nice, is this a recent change? Will fix. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1270: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); On 2016/11/11 16:13:21, Igor Sheludko wrote: > According to my understanding of a spec we must always call Set. It could be > observable if we pass non-regexp receiver with getter/setter lastIndex. See above. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1287: a->GotoIf(a->SameValue(current_last_index, previous_last_index, context), On 2016/11/11 16:13:21, Igor Sheludko wrote: > Same here. See above. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1304: // Return the index of the match. On 2016/11/11 16:13:20, Igor Sheludko wrote: > I guess in this case the match_indices could still be in fast mode. Yes, but I decided against complicating the implementation here for small performance gains on the slow path. I can be convinced otherwise though if anyone has strong opinions on this. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2279: Node* CodeStubAssembler::TryTaggedToFloat64(Node* value, On 2016/11/11 16:13:21, Igor Sheludko wrote: > Please also this TryTaggedToFloat64() in a kDouble case of > PrepareValueForWrite(). Will fix. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2283: var_result.Bind(Float64Constant(0.0)); On 2016/11/11 16:13:21, Igor Sheludko wrote: > Please remove this bind if the CSA does not complain. Will fix. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2300: Branch(WordEqual(LoadMap(value), HeapNumberMapConstant()), On 2016/11/11 16:13:21, Igor Sheludko wrote: > Branch(IsHeapNumberMap(LoadMap(value))), Will fix. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:8468: Select(Float64Equal(rhs_float, rhs_float), int_false, int_true); On 2016/11/11 16:13:21, Igor Sheludko wrote: > Select requires also the representation of a result (kTagged by default) which > is MachineType::PointerRepresentation() in your case. Otherwise GC may be > unhappy. Will fix.
Description was changed from ========== [regexp] Move RegExp.prototype[@@search] to TF This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ========== to ========== [regexp] Move RegExp.prototype[@@search] to TF This implements @@search according to the upcoming changes as in https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ==========
On 2016/11/13 13:00:10, jgruber wrote: > Thanks for the thorough review Igor, some initial replies on your major points. > Will fix the rest in an upcoming CL. s/upcoming CL/upcoming patchset/
https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1192: Node* const previous_last_index = FastLoadLastIndex(a, context, receiver); On 2016/11/13 13:00:09, jgruber wrote: > On 2016/11/11 16:13:21, Igor Sheludko wrote: > > Given that the only difference between fast and slow paths is the way the > > lastIndex property is accessed I think it makes sense to factor out the code > > that generates one branch to a helper function. > > That helper function exists and is used elsewhere (also in the first version of > this CL). The reason we now use separate fast and slow paths is that merging the > two paths would require repeated map checks to ensure we're still on the fast > path. > > Looking at this again, we can also skip the call to RegExpExec on the fast path, > calling directly into RegExpPrototypeExecInternal instead. I meant something like this: BranchIfFastPath(...) Bind(&fast_path); GenerateXYZBody(fast: true) Bind(&slow_path); GenerateXYZBody(fast: false); https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1197: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); On 2016/11/13 13:00:09, jgruber wrote: > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > According to my understanding of a spec we must always call Set. It could be > > observable if we pass non-regexp receiver with getter/setter lastIndex. > > Changes to @@search are about to be merged into the spec, and this CL implements > those semantics. See https://github.com/tc39/ecma262/pull/627. That pull request > ensures Set is only called when necessary. > > Sorry about not making this clearer. I discussed it in reply to Yang's comments > above, but I'll add it to this CL's commit message. > > > For the fast case the store is not observable but I'm not sure how much do you > > save here given the complexity of a SameValue implementation. > > True - but the common case, when both args to SameValue are Smis, should skip > most of the complication. Ah. Sorry, I didn't notice that. But still the idea is that for the fast case we know that the lastIndex is a data property and therefore the access is not observable and it will probably be faster to skip the SaveValue and always write zero. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1227: a->Return(a->SmiConstant(Smi::FromInt(-1))); On 2016/11/13 13:00:09, jgruber wrote: > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > SmiConstant(-1) > > Nice, is this a recent change? Will fix. Yes. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1304: // Return the index of the match. On 2016/11/13 13:00:09, jgruber wrote: > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > I guess in this case the match_indices could still be in fast mode. > > Yes, but I decided against complicating the implementation here for small > performance gains on the slow path. I can be convinced otherwise though if > anyone has strong opinions on this. Maybe if you follow the GenerateXYZBody idea mentioned above it will not be too complicated :)
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/16189) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16231) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
In this patchset: * A helper function to generate fast/slow paths * Unconditionally store on fast path instead of calling into SameValue * Call the internal exec implementation directly on the fast path instead of going through RegExpExec. PTAL. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1192: Node* const previous_last_index = FastLoadLastIndex(a, context, receiver); On 2016/11/13 18:27:59, Igor Sheludko wrote: > On 2016/11/13 13:00:09, jgruber wrote: > > On 2016/11/11 16:13:21, Igor Sheludko wrote: > > > Given that the only difference between fast and slow paths is the way the > > > lastIndex property is accessed I think it makes sense to factor out the code > > > that generates one branch to a helper function. > > > > That helper function exists and is used elsewhere (also in the first version > of > > this CL). The reason we now use separate fast and slow paths is that merging > the > > two paths would require repeated map checks to ensure we're still on the fast > > path. > > > > Looking at this again, we can also skip the call to RegExpExec on the fast > path, > > calling directly into RegExpPrototypeExecInternal instead. > > I meant something like this: > BranchIfFastPath(...) > Bind(&fast_path); > GenerateXYZBody(fast: true) > Bind(&slow_path); > GenerateXYZBody(fast: false); Done. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1197: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); On 2016/11/13 18:27:59, Igor Sheludko wrote: > On 2016/11/13 13:00:09, jgruber wrote: > > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > > According to my understanding of a spec we must always call Set. It could be > > > observable if we pass non-regexp receiver with getter/setter lastIndex. > > > > Changes to @@search are about to be merged into the spec, and this CL > implements > > those semantics. See https://github.com/tc39/ecma262/pull/627. That pull > request > > ensures Set is only called when necessary. > > > > Sorry about not making this clearer. I discussed it in reply to Yang's > comments > > above, but I'll add it to this CL's commit message. > > > > > For the fast case the store is not observable but I'm not sure how much do > you > > > save here given the complexity of a SameValue implementation. > > > > True - but the common case, when both args to SameValue are Smis, should skip > > most of the complication. > > Ah. Sorry, I didn't notice that. > But still the idea is that for the fast case we know that the lastIndex is a > data property and therefore the access is not observable and it will probably be > faster to skip the SaveValue and always write zero. Done. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1227: a->Return(a->SmiConstant(Smi::FromInt(-1))); On 2016/11/13 18:27:59, Igor Sheludko wrote: > On 2016/11/13 13:00:09, jgruber wrote: > > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > > SmiConstant(-1) > > > > Nice, is this a recent change? Will fix. > > Yes. Done. https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1304: // Return the index of the match. On 2016/11/13 18:27:59, Igor Sheludko wrote: > On 2016/11/13 13:00:09, jgruber wrote: > > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > > I guess in this case the match_indices could still be in fast mode. > > > > Yes, but I decided against complicating the implementation here for small > > performance gains on the slow path. I can be convinced otherwise though if > > anyone has strong opinions on this. > > Maybe if you follow the GenerateXYZBody idea mentioned above it will not be too > complicated :) Done. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2279: Node* CodeStubAssembler::TryTaggedToFloat64(Node* value, On 2016/11/11 16:13:21, Igor Sheludko wrote: > Please also this TryTaggedToFloat64() in a kDouble case of > PrepareValueForWrite(). Done. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2283: var_result.Bind(Float64Constant(0.0)); On 2016/11/11 16:13:21, Igor Sheludko wrote: > Please remove this bind if the CSA does not complain. Done. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:2300: Branch(WordEqual(LoadMap(value), HeapNumberMapConstant()), On 2016/11/11 16:13:21, Igor Sheludko wrote: > Branch(IsHeapNumberMap(LoadMap(value))), Done. https://codereview.chromium.org/2438683005/diff/140001/src/code-stub-assemble... src/code-stub-assembler.cc:8468: Select(Float64Equal(rhs_float, rhs_float), int_false, int_true); On 2016/11/11 16:13:21, Igor Sheludko wrote: > Select requires also the representation of a result (kTagged by default) which > is MachineType::PointerRepresentation() in your case. Otherwise GC may be > unhappy. Done.
Not rebasing for patch failures until comments are through (to preserve readability).
Description was changed from ========== [regexp] Move RegExp.prototype[@@search] to TF This implements @@search according to the upcoming changes as in https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ========== to ========== [regexp] Move RegExp.prototype[@@search] to TF Implements upcoming changes to @@search according to https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ==========
https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:1197: a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); On 2016/11/13 18:27:59, Igor Sheludko wrote: > On 2016/11/13 13:00:09, jgruber wrote: > > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > > According to my understanding of a spec we must always call Set. It could be > > > observable if we pass non-regexp receiver with getter/setter lastIndex. > > > > Changes to @@search are about to be merged into the spec, and this CL > implements > > those semantics. See https://github.com/tc39/ecma262/pull/627. That pull > request > > ensures Set is only called when necessary. > > > > Sorry about not making this clearer. I discussed it in reply to Yang's > comments > > above, but I'll add it to this CL's commit message. > > > > > For the fast case the store is not observable but I'm not sure how much do > you > > > save here given the complexity of a SameValue implementation. > > > > True - but the common case, when both args to SameValue are Smis, should skip > > most of the complication. > > Ah. Sorry, I didn't notice that. > But still the idea is that for the fast case we know that the lastIndex is a > data property and therefore the access is not observable and it will probably be > faster to skip the SaveValue and always write zero. Yeah. I agree. Always writing zero avoids the complexity introduced by SameValue and the branch. Note that this would also make code size a bit smaller.
On 2016/11/15 13:03:43, Yang wrote: > https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... > File src/builtins/builtins-regexp.cc (right): > > https://codereview.chromium.org/2438683005/diff/140001/src/builtins/builtins-... > src/builtins/builtins-regexp.cc:1197: > a->GotoIf(a->SameValue(previous_last_index, smi_zero, context), &next); > On 2016/11/13 18:27:59, Igor Sheludko wrote: > > On 2016/11/13 13:00:09, jgruber wrote: > > > On 2016/11/11 16:13:20, Igor Sheludko wrote: > > > > According to my understanding of a spec we must always call Set. It could > be > > > > observable if we pass non-regexp receiver with getter/setter lastIndex. > > > > > > Changes to @@search are about to be merged into the spec, and this CL > > implements > > > those semantics. See https://github.com/tc39/ecma262/pull/627. That pull > > request > > > ensures Set is only called when necessary. > > > > > > Sorry about not making this clearer. I discussed it in reply to Yang's > > comments > > > above, but I'll add it to this CL's commit message. > > > > > > > For the fast case the store is not observable but I'm not sure how much do > > you > > > > save here given the complexity of a SameValue implementation. > > > > > > True - but the common case, when both args to SameValue are Smis, should > skip > > > most of the complication. > > > > Ah. Sorry, I didn't notice that. > > But still the idea is that for the fast case we know that the lastIndex is a > > data property and therefore the access is not observable and it will probably > be > > faster to skip the SaveValue and always write zero. > > Yeah. I agree. Always writing zero avoids the complexity introduced by SameValue > and the branch. Note that this would also make code size a bit smaller. lgtm.
lgtm
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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12545)
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, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2438683005/#ps180001 (title: "Rebase")
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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12557)
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...
Message was sent while issue was closed.
Description was changed from ========== [regexp] Move RegExp.prototype[@@search] to TF Implements upcoming changes to @@search according to https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ========== to ========== [regexp] Move RegExp.prototype[@@search] to TF Implements upcoming changes to @@search according to https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Move RegExp.prototype[@@search] to TF Implements upcoming changes to @@search according to https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 ========== to ========== [regexp] Move RegExp.prototype[@@search] to TF Implements upcoming changes to @@search according to https://github.com/tc39/ecma262/pull/627. This also adds SameValue to CodeStubAssembler and extracts a part of CSA::TruncateTaggedToFloat64. BUG=v8:5339 Committed: https://crrev.com/e29fcbee9cebada7423eee79a77bc91a320588be Cr-Commit-Position: refs/heads/master@{#41000} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e29fcbee9cebada7423eee79a77bc91a320588be Cr-Commit-Position: refs/heads/master@{#41000} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
