Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663803002:
[string] Migrate String.prototype.{split,replace} to TF (Closed)
Description[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Original-Commit-Position: refs/heads/master@{#42881}
Committed: https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd224259
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42883}
Committed: https://chromium.googlesource.com/v8/v8/+/cb19ecd610ee0e7f8bc661616cdb9ca4e9ea804b
Patch Set 1 #Patch Set 2 : Fix test failures #
Total comments: 2
Patch Set 3 : Whitelist new builtins and fix fast-regexp check #
Total comments: 2
Patch Set 4 : Revert whitelists #Patch Set 5 : Add fast-path for strings #
Total comments: 2
Patch Set 6 : Fix fast-path check for strings #Patch Set 7 : Rebase #
Total comments: 26
Patch Set 8 : Address comments #
Total comments: 2
Patch Set 9 : Properly call ToString on early exit #
Total comments: 9
Patch Set 10 : Address comments #Patch Set 11 : Shortcut after index check #Patch Set 12 : Remove debug-evaluate whitelisting #
Messages
Total messages: 74 (51 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jgruber@chromium.org changed reviewers: + bmeurer@chromium.org, ishell@chromium.org
CC Yang, FYI in debug-evaluate-no-side-effect-builtins.
We could probably get some nice addtl improvements with some more work in this
area:
* RegExp fast paths - either inline RegExp{Split,Replace} or call a stub
directly when String{Split,Replace} is called with a RegExp argument.
* Same for String arguments - but we'd need to store the initial String map on
the context after setup. This will get easier once string.js is completely
migrated.
* More flexible StringIndexOf in CSA - we could handle up to 4/8 chars without
too much effort, which means we could avoid runtime calls in these cases.
* GetSubstitution currently means a runtime call, but it could also be moved to
CSA.
yangguo@chromium.org changed reviewers: + yangguo@chromium.org
https://codereview.chromium.org/2663803002/diff/20001/test/debugger/debug/deb... File test/debugger/debug/debug-evaluate-no-side-effect-builtins.js (right): https://codereview.chromium.org/2663803002/diff/20001/test/debugger/debug/deb... test/debugger/debug/debug-evaluate-no-side-effect-builtins.js:57: if (f == "replace") continue; Does String.prototype.{split,replace} with non-regexp arguments actually cause any side effects? If not, can we add these new builtins to the whitelist?
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/2663803002/diff/20001/test/debugger/debug/deb... File test/debugger/debug/debug-evaluate-no-side-effect-builtins.js (right): https://codereview.chromium.org/2663803002/diff/20001/test/debugger/debug/deb... test/debugger/debug/debug-evaluate-no-side-effect-builtins.js:57: if (f == "replace") continue; On 2017/01/31 11:52:44, Yang wrote: > Does String.prototype.{split,replace} with non-regexp arguments actually cause > any side effects? If not, can we add these new builtins to the whitelist? Added to whitelist in new patchset.
src/debug lgtm. https://codereview.chromium.org/2663803002/diff/40001/test/debugger/debug/deb... File test/debugger/debug/debug-evaluate-no-side-effect-builtins.js (right): https://codereview.chromium.org/2663803002/diff/40001/test/debugger/debug/deb... test/debugger/debug/debug-evaluate-no-side-effect-builtins.js:56: if (f == "split") continue; we can then remove these two lines here?
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...
Still adding a fast path for strings. https://codereview.chromium.org/2663803002/diff/40001/test/debugger/debug/deb... File test/debugger/debug/debug-evaluate-no-side-effect-builtins.js (right): https://codereview.chromium.org/2663803002/diff/40001/test/debugger/debug/deb... test/debugger/debug/debug-evaluate-no-side-effect-builtins.js:56: if (f == "split") continue; On 2017/01/31 12:52:51, Yang wrote: > we can then remove these two lines here? Reverted whitelisting again after offline discussion.
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: This issue passed the CQ dry run.
And added a string fast-path. Should be ready for review, PTAL.
You confused String.prototype and String.__proto__ https://codereview.chromium.org/2663803002/diff/80001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:1109: Node* const proto_map = LoadMap(LoadMapPrototype(LoadMap(string_function))); This needs to check the String.prototype map rather than the String.__proto__
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/2663803002/diff/80001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:1109: Node* const proto_map = LoadMap(LoadMapPrototype(LoadMap(string_function))); On 2017/01/31 15:02:36, Benedikt Meurer wrote: > This needs to check the String.prototype map rather than the String.__proto__ You're right. I was confused because the stored String.prototype map stored on the context was wrong. That's fixed in https://codereview.chromium.org/2663303003/, and the fast path check is fixed here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) 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_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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) 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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/33791) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21964)
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/2663803002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1069: return Word32Or(IsUndefined(value), IsNull(value)); CSA will generate a code that always compute both values. Consider using Select() here or maybe the form GotoIfNullOrUndefined(value, label) will be even better. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1075: Label out(this); Definitely a slow path, can probably be a deferred label. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1168: [this, context, search, receiver, replace](Node* fn) { I think it's fine to let it capture all that's necessary here: [=] https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1176: Node* const subject_string = CallStub(tostring_callable, context, receiver); Suggestion for another CL: how about having CallToStringStub(context, receiver) somewhere in CSA? We call it a lot. And same for other usages of raw CallStub() around for popular stubs. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1212: CallRuntime(Runtime::kStringIndexOfUnchecked, context, subject_string, Why not just StringBuiltinsAssembler::StringIndexOf()? https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1221: Return(subject_string); 1) Maybe we should introduce ReturnIf(condition, value) at some point... 2) We didn't follow the spec in in the old code, but while you are here... It looks like we should call ToString(replace) if the replace is not callable before returning from here. See step 6 of String.prototype.replace. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1311: [this, context, separator, receiver, limit](Node* fn) { [=] https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1324: IsUndefined(limit), [this]() { return SmiConstant(Smi::kMaxValue); }, [=] https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1325: [this, context, limit]() { return ToUint32(context, limit); }, [=] https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1336: const ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; INTPTR_PARAMETERS is default, you can just skip passing mode. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1338: Node* const allocation_site = nullptr; This parameter is also nullptr by default, you can skip passing it as well. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1359: const ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; Same here. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1361: Node* const allocation_site = nullptr; Same here.
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, all comments addressed. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1069: return Word32Or(IsUndefined(value), IsNull(value)); On 2017/02/01 12:55:30, Igor Sheludko wrote: > CSA will generate a code that always compute both values. Consider using > Select() here or maybe the form GotoIfNullOrUndefined(value, label) will be even > better. The reason I went with this form is that I'm expecting a non-null or undefined value in the common case, and I'd expect an OR + JMP to be faster than two JMPs. But I'm fine with switching to short-circuit evaluation as well. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1075: Label out(this); On 2017/02/01 12:55:30, Igor Sheludko wrote: > Definitely a slow path, can probably be a deferred label. Added a deferred label for the slow path. I suppose this is what you meant since 'out' is the fast path. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1168: [this, context, search, receiver, replace](Node* fn) { On 2017/02/01 12:55:30, Igor Sheludko wrote: > I think it's fine to let it capture all that's necessary here: [=] Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1176: Node* const subject_string = CallStub(tostring_callable, context, receiver); On 2017/02/01 12:55:30, Igor Sheludko wrote: > Suggestion for another CL: how about having CallToStringStub(context, receiver) > somewhere in CSA? We call it a lot. > > And same for other usages of raw CallStub() around for popular stubs. I like it. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1212: CallRuntime(Runtime::kStringIndexOfUnchecked, context, subject_string, On 2017/02/01 12:55:30, Igor Sheludko wrote: > Why not just StringBuiltinsAssembler::StringIndexOf()? Replaced with CodeFactory::StringIndexOf. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1221: Return(subject_string); On 2017/02/01 12:55:30, Igor Sheludko wrote: > 1) Maybe we should introduce ReturnIf(condition, value) at some point... > 2) We didn't follow the spec in in the old code, but while you are here... It > looks like we should call ToString(replace) if the replace is not callable > before returning from here. See step 6 of String.prototype.replace. ReturnIf sounds good! Ugh - yes that looks correct. A bit unfortunate that the spec is forcing us to do useless work. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1311: [this, context, separator, receiver, limit](Node* fn) { On 2017/02/01 12:55:30, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1324: IsUndefined(limit), [this]() { return SmiConstant(Smi::kMaxValue); }, On 2017/02/01 12:55:30, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1325: [this, context, limit]() { return ToUint32(context, limit); }, On 2017/02/01 12:55:30, Igor Sheludko wrote: > [=] Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1336: const ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; On 2017/02/01 12:55:30, Igor Sheludko wrote: > INTPTR_PARAMETERS is default, you can just skip passing mode. Done. I was thinking of refactoring common array allocations into a helper assembler. RegExp builtins also allocate empty or singleton arrays a couple of times. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1338: Node* const allocation_site = nullptr; On 2017/02/01 12:55:30, Igor Sheludko wrote: > This parameter is also nullptr by default, you can skip passing it as well. Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1359: const ParameterMode mode = CodeStubAssembler::INTPTR_PARAMETERS; On 2017/02/01 12:55:30, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2663803002/diff/120001/src/builtins/builtins-... src/builtins/builtins-string.cc:1361: Node* const allocation_site = nullptr; On 2017/02/01 12:55:30, Igor Sheludko wrote: > Same here. Done.
https://codereview.chromium.org/2663803002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:1215: Node* const replace_string = CallStub(tostring_callable, context, replace); We should not call it in case of callable "replace". I'd suggest to just move early exit check to the proper places of if_iscallablereplace and if_notcallablereplace blocks.
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/2663803002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:1215: Node* const replace_string = CallStub(tostring_callable, context, replace); On 2017/02/01 14:12:14, Igor Sheludko wrote: > We should not call it in case of callable "replace". > I'd suggest to just move early exit check to the proper places of > if_iscallablereplace and if_notcallablereplace blocks. Good catch. I moved the ToString call into the early exit branch with a prior map check.
lgtm once last comments are addressed: https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1219: GotoUnless(SmiIsNegative(match_start_index), &next); // The spec requires to perform ToString(replace) if the {replace} is not // callable even if we are going to exit here. // Since ToString() being applied to Smi does not have side effects for // numbers we can skip it. GotoIf(TaggedIsSmi(replace), &return_subject); https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1220: GotoIf(IsCallableMap(LoadMap(replace)), &return_subject); GotoIf(TaggedIsSmi(replace), &call_tostring); https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1222: // Call ToString(replace) and throw away the result. The call is required I suggested another comment above. https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1224: CallStub(tostring_callable, context, replace); // TODO(jgruber): consider introducing ToStringSideffectsStub() which does only observable part of ToString().
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/2663803002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1219: GotoUnless(SmiIsNegative(match_start_index), &next); On 2017/02/01 15:31:22, Igor Sheludko wrote: > // The spec requires to perform ToString(replace) if the {replace} is not > // callable even if we are going to exit here. > // Since ToString() being applied to Smi does not have side effects for > // numbers we can skip it. > GotoIf(TaggedIsSmi(replace), &return_subject); Done. https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1220: GotoIf(IsCallableMap(LoadMap(replace)), &return_subject); On 2017/02/01 15:31:23, Igor Sheludko wrote: > GotoIf(TaggedIsSmi(replace), &call_tostring); Leftover comment? https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1222: // Call ToString(replace) and throw away the result. The call is required On 2017/02/01 15:31:22, Igor Sheludko wrote: > I suggested another comment above. Done. https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1224: CallStub(tostring_callable, context, replace); On 2017/02/01 15:31:22, Igor Sheludko wrote: > // TODO(jgruber): consider introducing ToStringSideffectsStub() which does only > observable part of ToString(). Done.
lgtm https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2663803002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1220: GotoIf(IsCallableMap(LoadMap(replace)), &return_subject); On 2017/02/02 07:49:40, jgruber wrote: > On 2017/02/01 15:31:23, Igor Sheludko wrote: > > GotoIf(TaggedIsSmi(replace), &call_tostring); > > Leftover comment? Yep. Decided to add a comment above, but forgot to remove this one.
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
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2663803002/#ps200001 (title: "Shortcut after index check")
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 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, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2663803002/#ps220001 (title: "Remove debug-evaluate whitelisting")
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": 220001, "attempt_start_ts": 1486028595887800,
"parent_rev": "864799d3ebbaeb8d1d54192fe97c6dc30ecd3e66", "commit_rev":
"65ad1e35d9a97c1126a55cc9d3014598fd224259"}
Message was sent while issue was closed.
Description was changed from
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
==========
to
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42881}
Committed:
https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2671673003/ by machenbach@chromium.org. The reason for reverting is: Breaks win64 debug: https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20debug/builds....
Message was sent while issue was closed.
Note that happened earlier too: https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20debug/builds... Seems to be a build-dependent flake that flushes an existing problem?
Message was sent while issue was closed.
On 2017/02/02 11:15:43, Michael Achenbach wrote: > Note that happened earlier too: > https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20debug/builds... > > Seems to be a build-dependent flake that flushes an existing problem? Yes I saw that, I don't think it's related.
Message was sent while issue was closed.
Description was changed from
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42881}
Committed:
https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
==========
to
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42881}
Committed:
https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
==========
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": 220001, "attempt_start_ts": 1486034917709810,
"parent_rev": "2517b79cd6a482450cde8928022b08ba36266e26", "commit_rev":
"cb19ecd610ee0e7f8bc661616cdb9ca4e9ea804b"}
Message was sent while issue was closed.
Description was changed from
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42881}
Committed:
https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
==========
to
==========
[string] Migrate String.prototype.{split,replace} to TF
BUG=
Review-Url: https://codereview.chromium.org/2663803002
Cr-Original-Commit-Position: refs/heads/master@{#42881}
Committed:
https://chromium.googlesource.com/v8/v8/+/65ad1e35d9a97c1126a55cc9d3014598fd2...
Review-Url: https://codereview.chromium.org/2663803002
Cr-Commit-Position: refs/heads/master@{#42883}
Committed:
https://chromium.googlesource.com/v8/v8/+/cb19ecd610ee0e7f8bc661616cdb9ca4e9e...
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/v8/v8/+/cb19ecd610ee0e7f8bc661616cdb9ca4e9e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
