Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by jgruber Modified:
4 years, 2 months ago Reviewers:
Igor Sheludko, Benedikt Meurer CC:
v8-reviews_googlegroups.com, Benedikt Meurer, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[regexp] Port RegExp.prototype.exec to TurboFan
This ports RegExp.prototype.exec to a TurboFan builtin.
LastMatchInfo is now stored on the context in order to be able to access
it from the stub.
Unmodified RegExp instances go through a fast path of accessing the
lastIndex property as an in-object field, while modified instances call
into runtime for lastIndex loads and stores.
Octane/regexp shows slight improvements (between 0 and 5%) with this CL.
BUG=v8:5339
Committed: https://crrev.com/db99bdff767177cfc6467400955fc156a7459658
Cr-Commit-Position: refs/heads/master@{#39899}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Handle modified regexp instances #Patch Set 4 : Fix whitespace #Patch Set 5 : Format #Patch Set 6 : Handle smi this values #
Total comments: 43
Patch Set 7 : Address comments #
Total comments: 4
Patch Set 8 : Address comments #
Created: 4 years, 2 months ago
Messages
Total messages: 38 (28 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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25290)
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] Port RegExp.prototype.exec to TurboFan BUG= ========== to ========== [regexp] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements with this CL. Drive-by-fix: swapped label names in StringCharCodeAt. BUG=v8:5339 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/25292)
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/9877) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
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
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
Looks good to me overall. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:93: Callable substring_callable = CodeFactory::SubString(isolate); Nit: inline SubString here. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:94: Callable constructresult_callable = Nit: Follow-up CL, please port construct result to TurboFan as well and inline here.
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_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
First round of comments. https://codereview.chromium.org/2375953002/diff/100001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1674: Accessors::FunctionSetPrototype(regexp_fun, proto).Assert(); I think you can pass the right prototype object to InstallFunction that it creates regexp_fun. See Date initialization for example. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:32: JSRegExp::kSize + JSRegExp::kLastIndexFieldIndex; kLastIndexFieldIndex is an index, but not an offset. Please add "*kPointerSize" to be correct. Unfortunately we can't use Map::GetInObjectPropertyOffset() here because we don't have a Map instance. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:43: a->CallRuntime(Runtime::kGetProperty, context, regexp, name)); You can call GetPropertyStub here instead. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:63: JSRegExp::kSize + JSRegExp::kLastIndexFieldIndex; Same here. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:74: a->CallRuntime(Runtime::kSetProperty, context, regexp, name, value, // TODO(ishell): Use SetPropertyStub here once available. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:97: Node* const num_indices = a->SmiUntag(a->LoadFixedArrayElement( Once you avoid mixing int32/intptr indices, please pass INTPTR_PARAMETERS to LoadFixedArrayElement() here and below to avoid generation of Int32ToIntPtr convertions (sign extension instruction). https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:99: Node* const num_results = a->SmiTag(a->Word32Shr(num_indices, 1)); WordShr https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:123: Variable var_from_cursor(a, MachineRepresentation::kWord32); MachineType::PointerRepesentation() (which is intptr essentially). https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:124: Variable var_to_cursor(a, MachineRepresentation::kWord32); Same here. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:135: Node* from_cursor = var_from_cursor.value(); const? https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:142: Node* from_cursor_plus1 = a->IntPtrAdd(from_cursor, a->IntPtrConstant(1)); const? https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:154: a->Branch(a->IntPtrLessThan(var_from_cursor.value(), limit), &loop, &out); Since we don't expect negative indices here, I would suggest to use UintPtrLessThan() instead. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:202: a->Word32And(a->SmiUntag(flags), You are mixing int32/intptr computations here. I think it's better to stick with intptr. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:248: Node* const native_context = a->LoadNativeContext(context); |native_context| loaded above should be still available. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2233: int instance_type, Same here. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2236: Variable var_value_map(this, MachineType::PointerRepresentation()); MachineRepresentation::kTagged https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2367: Label if_stringisnotshort(this), I guess this renaming should be part of substr CL. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.h:472: compiler::Node* ThrowIfNotType(compiler::Node* context, compiler::Node* value, Suggestion: ThrowIfNotInstanceType https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.h:473: int instance_type, char const* method_name); InstanceType instance_type
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] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements with this CL. Drive-by-fix: swapped label names in StringCharCodeAt. BUG=v8:5339 ========== to ========== [regexp] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements (between 0 and 5%) with this CL. BUG=v8:5339 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal https://codereview.chromium.org/2375953002/diff/100001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1674: Accessors::FunctionSetPrototype(regexp_fun, proto).Assert(); On 2016/09/29 12:55:28, Igor Sheludko wrote: > I think you can pass the right prototype object to InstallFunction that it > creates regexp_fun. See Date initialization for example. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:32: JSRegExp::kSize + JSRegExp::kLastIndexFieldIndex; On 2016/09/29 12:55:29, Igor Sheludko wrote: > kLastIndexFieldIndex is an index, but not an offset. Please add "*kPointerSize" > to be correct. > > Unfortunately we can't use Map::GetInObjectPropertyOffset() here because we > don't have a Map instance. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:43: a->CallRuntime(Runtime::kGetProperty, context, regexp, name)); On 2016/09/29 12:55:28, Igor Sheludko wrote: > You can call GetPropertyStub here instead. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:63: JSRegExp::kSize + JSRegExp::kLastIndexFieldIndex; On 2016/09/29 12:55:28, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:74: a->CallRuntime(Runtime::kSetProperty, context, regexp, name, value, On 2016/09/29 12:55:29, Igor Sheludko wrote: > // TODO(ishell): Use SetPropertyStub here once available. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:93: Callable substring_callable = CodeFactory::SubString(isolate); On 2016/09/29 11:10:08, Benedikt Meurer wrote: > Nit: inline SubString here. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:94: Callable constructresult_callable = On 2016/09/29 11:10:08, Benedikt Meurer wrote: > Nit: Follow-up CL, please port construct result to TurboFan as well and inline > here. Acknowledged. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:97: Node* const num_indices = a->SmiUntag(a->LoadFixedArrayElement( On 2016/09/29 12:55:28, Igor Sheludko wrote: > Once you avoid mixing int32/intptr indices, please pass INTPTR_PARAMETERS to > LoadFixedArrayElement() here and below to avoid generation of Int32ToIntPtr > convertions (sign extension instruction). Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:99: Node* const num_results = a->SmiTag(a->Word32Shr(num_indices, 1)); On 2016/09/29 12:55:28, Igor Sheludko wrote: > WordShr Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:123: Variable var_from_cursor(a, MachineRepresentation::kWord32); On 2016/09/29 12:55:28, Igor Sheludko wrote: > MachineType::PointerRepesentation() (which is intptr essentially). Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:124: Variable var_to_cursor(a, MachineRepresentation::kWord32); On 2016/09/29 12:55:28, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:135: Node* from_cursor = var_from_cursor.value(); On 2016/09/29 12:55:28, Igor Sheludko wrote: > const? Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:142: Node* from_cursor_plus1 = a->IntPtrAdd(from_cursor, a->IntPtrConstant(1)); On 2016/09/29 12:55:28, Igor Sheludko wrote: > const? Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:154: a->Branch(a->IntPtrLessThan(var_from_cursor.value(), limit), &loop, &out); On 2016/09/29 12:55:28, Igor Sheludko wrote: > Since we don't expect negative indices here, I would suggest to use > UintPtrLessThan() instead. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:202: a->Word32And(a->SmiUntag(flags), On 2016/09/29 12:55:29, Igor Sheludko wrote: > You are mixing int32/intptr computations here. I think it's better to stick with > intptr. Done. https://codereview.chromium.org/2375953002/diff/100001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:248: Node* const native_context = a->LoadNativeContext(context); On 2016/09/29 12:55:28, Igor Sheludko wrote: > |native_context| loaded above should be still available. Done. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2233: int instance_type, On 2016/09/29 12:55:29, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2236: Variable var_value_map(this, MachineType::PointerRepresentation()); On 2016/09/29 12:55:29, Igor Sheludko wrote: > MachineRepresentation::kTagged Done. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2367: Label if_stringisnotshort(this), On 2016/09/29 12:55:29, Igor Sheludko wrote: > I guess this renaming should be part of substr CL. How come? It's unrelated to both - I'll just split it out into a separate CL. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.h:472: compiler::Node* ThrowIfNotType(compiler::Node* context, compiler::Node* value, On 2016/09/29 12:55:29, Igor Sheludko wrote: > Suggestion: ThrowIfNotInstanceType Done. https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.h:473: int instance_type, char const* method_name); On 2016/09/29 12:55:29, Igor Sheludko wrote: > InstanceType instance_type Done.
lgtm with nits: https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2375953002/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:2367: Label if_stringisnotshort(this), On 2016/09/29 14:40:27, jgruber wrote: > On 2016/09/29 12:55:29, Igor Sheludko wrote: > > I guess this renaming should be part of substr CL. > > How come? It's unrelated to both - I'll just split it out into a separate CL. Ok. https://codereview.chromium.org/2375953002/diff/120001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2375953002/diff/120001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1669: { Please remove this pair of braces. https://codereview.chromium.org/2375953002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2375953002/diff/120001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:98: CodeStubAssembler::ParameterMode intptr_mode = How about just mode? or parameter_mode?
LGTM from my side.
https://codereview.chromium.org/2375953002/diff/120001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2375953002/diff/120001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1669: { On 2016/09/29 15:01:34, Igor Sheludko wrote: > Please remove this pair of braces. Done. https://codereview.chromium.org/2375953002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2375953002/diff/120001/src/builtins/builtins-... src/builtins/builtins-regexp.cc:98: CodeStubAssembler::ParameterMode intptr_mode = On 2016/09/29 15:01:34, Igor Sheludko wrote: > How about just mode? or parameter_mode? 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, bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2375953002/#ps140001 (title: "Address comments")
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] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements (between 0 and 5%) with this CL. BUG=v8:5339 ========== to ========== [regexp] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements (between 0 and 5%) with this CL. BUG=v8:5339 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements (between 0 and 5%) with this CL. BUG=v8:5339 ========== to ========== [regexp] Port RegExp.prototype.exec to TurboFan This ports RegExp.prototype.exec to a TurboFan builtin. LastMatchInfo is now stored on the context in order to be able to access it from the stub. Unmodified RegExp instances go through a fast path of accessing the lastIndex property as an in-object field, while modified instances call into runtime for lastIndex loads and stores. Octane/regexp shows slight improvements (between 0 and 5%) with this CL. BUG=v8:5339 Committed: https://crrev.com/db99bdff767177cfc6467400955fc156a7459658 Cr-Commit-Position: refs/heads/master@{#39899} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/db99bdff767177cfc6467400955fc156a7459658 Cr-Commit-Position: refs/heads/master@{#39899} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
