Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358263002:
[builtins] migrate C++ String Iterator builtins to baseline TurboFan (Closed)
Description[builtins] migrate C++ String Iterator builtins to baseline TurboFan
Migrate newly added C++ String Iterator builtins to TFJ builtins, per
step 4. of the String Iterator Baseline Implementation section of the design doc
BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
Committed: https://crrev.com/f9a2c8b1112c4e915df8bc5f7ea1fccdf7a33ff8
Cr-Commit-Position: refs/heads/master@{#39765}
Patch Set 1 #Patch Set 2 : V2 (fixups?) #
Total comments: 1
Patch Set 3 : v3 (try again, but probably not ._.) #
Total comments: 7
Patch Set 4 : v4 (refactor and try again) #Patch Set 5 : v5 (CF instead of SF where appropriate) #Patch Set 6 : v5 + minor cleanup #Patch Set 7 : v6 (fix toString() behaviour in S.p[@@iterator]) #
Total comments: 11
Patch Set 8 : v7 #
Total comments: 5
Patch Set 9 : some review #Patch Set 10 : Adapt arguments for TFJ builtins #
Total comments: 14
Patch Set 11 : final nits #Patch Set 12 : Fix the comment #Patch Set 13 : add "break;" statement to switch case #
Total comments: 1
Messages
Total messages: 81 (49 generated)
The CQ bit was checked by caitp@igalia.com 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...
PTAL I haven't measured if this is really beneficial for the baseline implementation (yet), but measurements will be forthcoming
PTAL I haven't measured if this is really beneficial for the baseline implementation (yet), but measurements will be forthcoming
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...)
On 2016/09/21 21:26:03, caitp wrote: > PTAL > > I haven't measured if this is really beneficial for the baseline implementation > (yet), but measurements will be forthcoming Eh, the performance measurements can wait until it's not broken. Hopefully ready for a look by morning in MUC
The CQ bit was checked by caitp@igalia.com 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/24674)
https://codereview.chromium.org/2358263002/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:2498: Node* ch = Word32Or(WordShl(lead, Int32Constant(16)), trail); I'm not sure how to build on a big endian arch (it looks like BUILD.gn has todos for big endian mips, and the Makefile doesn't support big endian mips builds... build_config.h doesn't seem to take PPC into consideration). So this may not be the right behaviour for big endian arches at all. I'm guessing the CodeStubAssembler won't generate code for sequential calls to StoreNoWriteBarrier() that codegen can turn into a single 32bit store, but if I'm wrong, seems like a better way to go.
The CQ bit was checked by caitp@igalia.com 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_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_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
Nice. First round of comments. Also please update JSStringIterator::JSStringIteratorVerify() to assert the invariant that the string is always either a sequential or external string. https://codereview.chromium.org/2358263002/diff/40001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/builtins/builtins-s... src/builtins/builtins-string.cc:739: Node* js_iter = assembler->Parameter(0); Nit: rename js_iter to iterator. https://codereview.chromium.org/2358263002/diff/40001/src/builtins/builtins-s... src/builtins/builtins-string.cc:776: assembler->Int32Constant(Context::ITERATOR_RESULT_MAP_INDEX), 0, This must be IntPtrConstant then. https://codereview.chromium.org/2358263002/diff/40001/src/builtins/builtins-s... src/builtins/builtins-string.cc:778: Node* iterator = assembler->Allocate(JSIteratorResult::kSize); Nit: Rename iterator to result. https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, I'd prefer to have a helper method StringFromCodePoint, similar to StringFromCharCode, which just takes the 32-bit code point and returns a proper string, utilizing StringFromCharCode for 16-bit strings, otherwise just allocating a surrogate pair string. And since the decision tree to load the actual code point is highly specific to the string iterator (as it can only deal with sequential/external strings), I'd prefer to have that either inlined into the iterator next function or as a helper function in builtins-string.cc. So basically this way: code, size = LoadCodePointAt(string, string.length, iterator.index); iterator.index += size; return StringFromCodePoint(code);
https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, On 2016/09/22 03:48:23, Benedikt Meurer wrote: > I'd prefer to have a helper method StringFromCodePoint, similar to > StringFromCharCode, which just takes the 32-bit code point and returns a proper > string, utilizing StringFromCharCode for 16-bit strings, otherwise just > allocating a surrogate pair string. > > And since the decision tree to load the actual code point is highly specific to > the string iterator (as it can only deal with sequential/external strings), I'd > prefer to have that either inlined into the iterator next function or as a > helper function in builtins-string.cc. So basically this way: > > code, size = LoadCodePointAt(string, string.length, iterator.index); > iterator.index += size; > return StringFromCodePoint(code); The main reason I've put this in CodeStubAssembler is for readability --- I find it difficult to read what's going on with all of the `assembler->` taking up a lot of extra space and not really serving much purpose. Something like the MacroAssembler convention of `#define __ ACCESS_MASM(masm_)` would help with that a bit, but I didn't want to do that because I figured it would get shot down :) I'm in the process of moving this to builtins-string.cc, but it takes up a lot more lines, and is a lot harder to read this way.
The CQ bit was checked by caitp@igalia.com 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_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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13281) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9450)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by caitp@igalia.com 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 checked by caitp@igalia.com 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_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13293) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by caitp@igalia.com 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.
Now that everything is passing tests, some microbenchmark results: ``` >>> Running suite: JSTests/Strings >>> Stdout (#1): StringIterators-Spread_OneByteShort-Strings(Score): 131440 StringIterators-Spread_TwoByteShort-Strings(Score): 58650 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 31940 >>> Stdout (#1) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 67850 StringIterators-Spread_TwoByteShort-Strings(Score): 31460 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 18240 ``` (subsequent runs are pretty similar) The benchmarks are pretty straightforward, available at https://gist.github.com/caitp/db6eec57821fc0982709544b04b950bd
On 2016/09/22 23:42:27, caitp wrote: > Now that everything is passing tests, some microbenchmark results: > > ``` > >>> Running suite: JSTests/Strings > >>> Stdout (#1): > StringIterators-Spread_OneByteShort-Strings(Score): 131440 > StringIterators-Spread_TwoByteShort-Strings(Score): 58650 > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 31940 > > >>> Stdout (#1) - without patch: > StringIterators-Spread_OneByteShort-Strings(Score): 67850 > StringIterators-Spread_TwoByteShort-Strings(Score): 31460 > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 18240 > ``` > > (subsequent runs are pretty similar) > > The benchmarks are pretty straightforward, available at > https://gist.github.com/caitp/db6eec57821fc0982709544b04b950bd Further testing: with 5784773feb92d06e76d4eb0fb93ed9496b9df30b ("[builtins] move String.prototype[@@iterator] to C++ builtin") reverted, this patch performs slightly better on those benchmarks than the old JS implementation, even when cons-strings are the target string. Values like ``` >>> Stdout (#1): StringIterators-Spread_OneByteShort-Strings(Score): 125060 StringIterators-Spread_TwoByteShort-Strings(Score): 54610 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 30320 >>> Stdout (#1) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 122670 StringIterators-Spread_TwoByteShort-Strings(Score): 53300 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 29860 ``` I think this is acceptable for a baseline, but am not super happy about the amount of code this amounts to.
On 2016/09/23 01:26:36, caitp wrote: > On 2016/09/22 23:42:27, caitp wrote: > > Now that everything is passing tests, some microbenchmark results: > > > > ``` > > >>> Running suite: JSTests/Strings > > >>> Stdout (#1): > > StringIterators-Spread_OneByteShort-Strings(Score): 131440 > > StringIterators-Spread_TwoByteShort-Strings(Score): 58650 > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 31940 > > > > >>> Stdout (#1) - without patch: > > StringIterators-Spread_OneByteShort-Strings(Score): 67850 > > StringIterators-Spread_TwoByteShort-Strings(Score): 31460 > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 18240 > > ``` > > > > (subsequent runs are pretty similar) > > > > The benchmarks are pretty straightforward, available at > > https://gist.github.com/caitp/db6eec57821fc0982709544b04b950bd > > Further testing: with 5784773feb92d06e76d4eb0fb93ed9496b9df30b ("[builtins] move > String.prototype[@@iterator] to C++ builtin") reverted, this patch performs > slightly better on those benchmarks than the old JS implementation, even when > cons-strings are the target string. Values like > > ``` > >>> Stdout (#1): > StringIterators-Spread_OneByteShort-Strings(Score): 125060 > StringIterators-Spread_TwoByteShort-Strings(Score): 54610 > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 30320 > > >>> Stdout (#1) - without patch: > StringIterators-Spread_OneByteShort-Strings(Score): 122670 > StringIterators-Spread_TwoByteShort-Strings(Score): 53300 > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 29860 > ``` > > I think this is acceptable for a baseline, but am not super happy about the > amount of code this amounts to. Well, those are fairly short strings, so you mostly measure the setup cost, which is only slightly better; also I might be mistaken, but it seems like these benchmarks measure a lot of other things as well, not only the performance of the string iterator. Can you do a benchmark with large/mid-size strings and only iterating a string via for-of?
https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, I'd be fine with __ if it improves readability (and in fact I think it often does, as can be seen in the interpreter). An alternative approach could be a BuiltinAssembler that inherits CodeStubAssembler, which would also make this easier to read w/o polluting the CodeStubAssembler. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:734: static compiler::Node* CreateStringFromCodePointInternal( You don't need static inside an anonymous namespace. I'm also not sure about the encoding, why do you need that? Because w/o the encoding I think this should be on the CodeStubAssembler as StringFromCodePoint, which we can also utilize for String.fromCodePoint in the future.
On 2016/09/23 03:44:49, Benedikt Meurer wrote: > On 2016/09/23 01:26:36, caitp wrote: > > On 2016/09/22 23:42:27, caitp wrote: > > > Now that everything is passing tests, some microbenchmark results: > > > > > > ``` > > > >>> Running suite: JSTests/Strings > > > >>> Stdout (#1): > > > StringIterators-Spread_OneByteShort-Strings(Score): 131440 > > > StringIterators-Spread_TwoByteShort-Strings(Score): 58650 > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 31940 > > > > > > >>> Stdout (#1) - without patch: > > > StringIterators-Spread_OneByteShort-Strings(Score): 67850 > > > StringIterators-Spread_TwoByteShort-Strings(Score): 31460 > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 18240 > > > ``` > > > > > > (subsequent runs are pretty similar) > > > > > > The benchmarks are pretty straightforward, available at > > > https://gist.github.com/caitp/db6eec57821fc0982709544b04b950bd > > > > Further testing: with 5784773feb92d06e76d4eb0fb93ed9496b9df30b ("[builtins] > move > > String.prototype[@@iterator] to C++ builtin") reverted, this patch performs > > slightly better on those benchmarks than the old JS implementation, even when > > cons-strings are the target string. Values like > > > > ``` > > >>> Stdout (#1): > > StringIterators-Spread_OneByteShort-Strings(Score): 125060 > > StringIterators-Spread_TwoByteShort-Strings(Score): 54610 > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 30320 > > > > >>> Stdout (#1) - without patch: > > StringIterators-Spread_OneByteShort-Strings(Score): 122670 > > StringIterators-Spread_TwoByteShort-Strings(Score): 53300 > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 29860 > > ``` > > > > I think this is acceptable for a baseline, but am not super happy about the > > amount of code this amounts to. > > Well, those are fairly short strings, so you mostly measure the setup cost, > which is only slightly better; also I might be mistaken, but it seems like these > benchmarks measure a lot of other things as well, not only the performance of > the string iterator. > > Can you do a benchmark with large/mid-size strings and only iterating a string > via for-of? I'll write up some more of these in the next few hours https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, On 2016/09/23 03:46:24, Benedikt Meurer wrote: > I'd be fine with __ if it improves readability (and in fact I think it often > does, as can be seen in the interpreter). > > An alternative approach could be a BuiltinAssembler that inherits > CodeStubAssembler, which would also make this easier to read w/o polluting the > CodeStubAssembler. Acknowledged, but maybe that will be a cleanup followup cl https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:734: static compiler::Node* CreateStringFromCodePointInternal( On 2016/09/23 03:46:24, Benedikt Meurer wrote: > You don't need static inside an anonymous namespace. > I'm also not sure about the encoding, why do you need that? > Because w/o the encoding I think this should be on the CodeStubAssembler as > StringFromCodePoint, which we can also utilize for String.fromCodePoint in the > future. The UnicodeEncoding thing is there to control how the character is represented, which would allow it to be used for String.prototype.codePointAt / String.prototype.fromCodePoint. But that's the only reason
On 2016/09/23 10:14:28, caitp wrote: > On 2016/09/23 03:44:49, Benedikt Meurer wrote: > > On 2016/09/23 01:26:36, caitp wrote: > > > On 2016/09/22 23:42:27, caitp wrote: > > > > Now that everything is passing tests, some microbenchmark results: > > > > > > > > ``` > > > > >>> Running suite: JSTests/Strings > > > > >>> Stdout (#1): > > > > StringIterators-Spread_OneByteShort-Strings(Score): 131440 > > > > StringIterators-Spread_TwoByteShort-Strings(Score): 58650 > > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 31940 > > > > > > > > >>> Stdout (#1) - without patch: > > > > StringIterators-Spread_OneByteShort-Strings(Score): 67850 > > > > StringIterators-Spread_TwoByteShort-Strings(Score): 31460 > > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 18240 > > > > ``` > > > > > > > > (subsequent runs are pretty similar) > > > > > > > > The benchmarks are pretty straightforward, available at > > > > https://gist.github.com/caitp/db6eec57821fc0982709544b04b950bd > > > > > > Further testing: with 5784773feb92d06e76d4eb0fb93ed9496b9df30b ("[builtins] > > move > > > String.prototype[@@iterator] to C++ builtin") reverted, this patch performs > > > slightly better on those benchmarks than the old JS implementation, even > when > > > cons-strings are the target string. Values like > > > > > > ``` > > > >>> Stdout (#1): > > > StringIterators-Spread_OneByteShort-Strings(Score): 125060 > > > StringIterators-Spread_TwoByteShort-Strings(Score): 54610 > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 30320 > > > > > > >>> Stdout (#1) - without patch: > > > StringIterators-Spread_OneByteShort-Strings(Score): 122670 > > > StringIterators-Spread_TwoByteShort-Strings(Score): 53300 > > > StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 29860 > > > ``` > > > > > > I think this is acceptable for a baseline, but am not super happy about the > > > amount of code this amounts to. > > > > Well, those are fairly short strings, so you mostly measure the setup cost, > > which is only slightly better; also I might be mistaken, but it seems like > these > > benchmarks measure a lot of other things as well, not only the performance of > > the string iterator. > > > > Can you do a benchmark with large/mid-size strings and only iterating a string > > via for-of? > > I'll write up some more of these in the next few hours > > https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc > File src/code-stub-assembler.cc (right): > > https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler... > src/code-stub-assembler.cc:2432: Node* > CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, > On 2016/09/23 03:46:24, Benedikt Meurer wrote: > > I'd be fine with __ if it improves readability (and in fact I think it often > > does, as can be seen in the interpreter). > > > > An alternative approach could be a BuiltinAssembler that inherits > > CodeStubAssembler, which would also make this easier to read w/o polluting the > > CodeStubAssembler. > > Acknowledged, but maybe that will be a cleanup followup cl > > https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... > File src/builtins/builtins-string.cc (right): > > https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... > src/builtins/builtins-string.cc:734: static compiler::Node* > CreateStringFromCodePointInternal( > On 2016/09/23 03:46:24, Benedikt Meurer wrote: > > You don't need static inside an anonymous namespace. > > I'm also not sure about the encoding, why do you need that? > > Because w/o the encoding I think this should be on the CodeStubAssembler as > > StringFromCodePoint, which we can also utilize for String.fromCodePoint in the > > future. > > The UnicodeEncoding thing is there to control how the character is represented, > which would allow it to be used for String.prototype.codePointAt / > String.prototype.fromCodePoint. But that's the only reason The old benchmarks were actually pretty wrong in general, so I've updated them with correctness checks. ``` tools/run_perf.py --outdir-no-patch=out --outdir=patched --arch=x64 test/js-perf-test/StringTests.json >>> Running suite: JSTests/Strings >>> Stdout (#1): StringIterators-Spread_OneByteShort-Strings(Score): 132160 StringIterators-Spread_TwoByteShort-Strings(Score): 292130 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 220840 StringIterators-ForOf_OneByteLong-Strings(Score): 2243 StringIterators-ForOf_TwoByteLong-Strings(Score): 2180 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 3743 >>> Stdout (#1) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 133170 StringIterators-Spread_TwoByteShort-Strings(Score): 185890 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 128250 StringIterators-ForOf_OneByteLong-Strings(Score): 2000 StringIterators-ForOf_TwoByteLong-Strings(Score): 2028 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 1409 >>> Stdout (#2): StringIterators-Spread_OneByteShort-Strings(Score): 132170 StringIterators-Spread_TwoByteShort-Strings(Score): 285180 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 217390 StringIterators-ForOf_OneByteLong-Strings(Score): 2210 StringIterators-ForOf_TwoByteLong-Strings(Score): 2243 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 3686 >>> Stdout (#2) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 134060 StringIterators-Spread_TwoByteShort-Strings(Score): 190790 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 128820 StringIterators-ForOf_OneByteLong-Strings(Score): 2098 StringIterators-ForOf_TwoByteLong-Strings(Score): 2200 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 1459 >>> Stdout (#3): StringIterators-Spread_OneByteShort-Strings(Score): 130020 StringIterators-Spread_TwoByteShort-Strings(Score): 282820 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 221420 StringIterators-ForOf_OneByteLong-Strings(Score): 2258 StringIterators-ForOf_TwoByteLong-Strings(Score): 2233 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 3640 >>> Stdout (#3) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 129300 StringIterators-Spread_TwoByteShort-Strings(Score): 189900 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 124480 StringIterators-ForOf_OneByteLong-Strings(Score): 2090 StringIterators-ForOf_TwoByteLong-Strings(Score): 2106 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 1460 >>> Stdout (#4): StringIterators-Spread_OneByteShort-Strings(Score): 134210 StringIterators-Spread_TwoByteShort-Strings(Score): 286150 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 224380 StringIterators-ForOf_OneByteLong-Strings(Score): 2183 StringIterators-ForOf_TwoByteLong-Strings(Score): 2248 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 3703 >>> Stdout (#4) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 129490 StringIterators-Spread_TwoByteShort-Strings(Score): 192230 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 124490 StringIterators-ForOf_OneByteLong-Strings(Score): 2118 StringIterators-ForOf_TwoByteLong-Strings(Score): 2134 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 1375 >>> Stdout (#5): StringIterators-Spread_OneByteShort-Strings(Score): 131860 StringIterators-Spread_TwoByteShort-Strings(Score): 277700 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 210450 StringIterators-ForOf_OneByteLong-Strings(Score): 2064 StringIterators-ForOf_TwoByteLong-Strings(Score): 2118 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 3610 >>> Stdout (#5) - without patch: StringIterators-Spread_OneByteShort-Strings(Score): 130530 StringIterators-Spread_TwoByteShort-Strings(Score): 190060 StringIterators-Spread_WithSurrogatePairsShort-Strings(Score): 128400 StringIterators-ForOf_OneByteLong-Strings(Score): 2102 StringIterators-ForOf_TwoByteLong-Strings(Score): 2112 StringIterators-ForOf_WithSurrogatePairsLong-Strings(Score): 1480 ``` In general, it looks like the TF version generally handles surrogate pairs significantly better than the JS version, but is for the most part pretty close / perf neutral for other cases
On 2016/09/23 12:21:48, caitp wrote: > ... > In general, it looks like the TF version generally handles surrogate pairs > significantly better than the JS version, but is for the most part pretty close > / perf neutral for other cases well, in fairness, those long string benchmarks are operating on cons strings, so they incur the cost of flattening during setup, and seem to do a pretty good job of catching up. For even longer strings, they probably do win.
On 2016/09/23 12:26:57, caitp wrote: > On 2016/09/23 12:21:48, caitp wrote: > > ... > > In general, it looks like the TF version generally handles surrogate pairs > > significantly better than the JS version, but is for the most part pretty > close > > / perf neutral for other cases > > well, in fairness, those long string benchmarks are operating on cons strings, > so they incur the cost of flattening during setup, and seem to do a pretty good > job of catching up. For even longer strings, they probably do win. and again, further tests, with more fixed benchmarks, seem to indicate this is basically perf-neutral, and the surrogate pairs case doesn't bring as much of an improvement as I thought (was a bug in the test :(). Still, generally seems to do better than the old code, very slightly.: ``` >>> Running suite: JSTests/StringIterators >>> Stdout (#1): Spread_OneByteShort-StringIterators(Score): 125130 Spread_TwoByteShort-StringIterators(Score): 180920 Spread_WithSurrogatePairsShort-StringIterators(Score): 115120 ForOf_OneByteShort-StringIterators(Score): 212490 ForOf_TwoByteShort-StringIterators(Score): 251680 ForOf_WithSurrogatePairsShort-StringIterators(Score): 150680 ForOf_OneByteLong-StringIterators(Score): 1870 ForOf_TwoByteLong-StringIterators(Score): 1891 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1284 >>> Stdout (#1) - without patch: Spread_OneByteShort-StringIterators(Score): 123250 Spread_TwoByteShort-StringIterators(Score): 188910 Spread_WithSurrogatePairsShort-StringIterators(Score): 118670 ForOf_OneByteShort-StringIterators(Score): 194720 ForOf_TwoByteShort-StringIterators(Score): 259080 ForOf_WithSurrogatePairsShort-StringIterators(Score): 168770 ForOf_OneByteLong-StringIterators(Score): 1888 ForOf_TwoByteLong-StringIterators(Score): 2030 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1327 >>> Stdout (#2): Spread_OneByteShort-StringIterators(Score): 125650 Spread_TwoByteShort-StringIterators(Score): 181690 Spread_WithSurrogatePairsShort-StringIterators(Score): 113940 ForOf_OneByteShort-StringIterators(Score): 199130 ForOf_TwoByteShort-StringIterators(Score): 258030 ForOf_WithSurrogatePairsShort-StringIterators(Score): 151190 ForOf_OneByteLong-StringIterators(Score): 1798 ForOf_TwoByteLong-StringIterators(Score): 1888 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1294 >>> Stdout (#2) - without patch: Spread_OneByteShort-StringIterators(Score): 114420 Spread_TwoByteShort-StringIterators(Score): 173210 Spread_WithSurrogatePairsShort-StringIterators(Score): 111650 ForOf_OneByteShort-StringIterators(Score): 208700 ForOf_TwoByteShort-StringIterators(Score): 260400 ForOf_WithSurrogatePairsShort-StringIterators(Score): 152900 ForOf_OneByteLong-StringIterators(Score): 1788 ForOf_TwoByteLong-StringIterators(Score): 1863 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1265 >>> Stdout (#3): Spread_OneByteShort-StringIterators(Score): 116710 Spread_TwoByteShort-StringIterators(Score): 170910 Spread_WithSurrogatePairsShort-StringIterators(Score): 111590 ForOf_OneByteShort-StringIterators(Score): 200530 ForOf_TwoByteShort-StringIterators(Score): 248370 ForOf_WithSurrogatePairsShort-StringIterators(Score): 154050 ForOf_OneByteLong-StringIterators(Score): 1808 ForOf_TwoByteLong-StringIterators(Score): 1854 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1320 >>> Stdout (#3) - without patch: Spread_OneByteShort-StringIterators(Score): 115410 Spread_TwoByteShort-StringIterators(Score): 169980 Spread_WithSurrogatePairsShort-StringIterators(Score): 108370 ForOf_OneByteShort-StringIterators(Score): 204700 ForOf_TwoByteShort-StringIterators(Score): 254230 ForOf_WithSurrogatePairsShort-StringIterators(Score): 155400 ForOf_OneByteLong-StringIterators(Score): 1871 ForOf_TwoByteLong-StringIterators(Score): 1880 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1291 >>> Stdout (#4): Spread_OneByteShort-StringIterators(Score): 117620 Spread_TwoByteShort-StringIterators(Score): 168530 Spread_WithSurrogatePairsShort-StringIterators(Score): 112380 ForOf_OneByteShort-StringIterators(Score): 209170 ForOf_TwoByteShort-StringIterators(Score): 257580 ForOf_WithSurrogatePairsShort-StringIterators(Score): 143650 ForOf_OneByteLong-StringIterators(Score): 1858 ForOf_TwoByteLong-StringIterators(Score): 1950 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1384 >>> Stdout (#4) - without patch: Spread_OneByteShort-StringIterators(Score): 115510 Spread_TwoByteShort-StringIterators(Score): 172010 Spread_WithSurrogatePairsShort-StringIterators(Score): 108840 ForOf_OneByteShort-StringIterators(Score): 189800 ForOf_TwoByteShort-StringIterators(Score): 243660 ForOf_WithSurrogatePairsShort-StringIterators(Score): 152530 ForOf_OneByteLong-StringIterators(Score): 1831 ForOf_TwoByteLong-StringIterators(Score): 1884 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1359 >>> Stdout (#5): Spread_OneByteShort-StringIterators(Score): 121470 Spread_TwoByteShort-StringIterators(Score): 183830 Spread_WithSurrogatePairsShort-StringIterators(Score): 109530 ForOf_OneByteShort-StringIterators(Score): 207980 ForOf_TwoByteShort-StringIterators(Score): 275850 ForOf_WithSurrogatePairsShort-StringIterators(Score): 162310 ForOf_OneByteLong-StringIterators(Score): 1912 ForOf_TwoByteLong-StringIterators(Score): 1934 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1339 >>> Stdout (#5) - without patch: Spread_OneByteShort-StringIterators(Score): 124170 Spread_TwoByteShort-StringIterators(Score): 173700 Spread_WithSurrogatePairsShort-StringIterators(Score): 117940 ForOf_OneByteShort-StringIterators(Score): 213630 ForOf_TwoByteShort-StringIterators(Score): 257290 ForOf_WithSurrogatePairsShort-StringIterators(Score): 162880 ForOf_OneByteLong-StringIterators(Score): 1820 ForOf_TwoByteLong-StringIterators(Score): 1890 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1333 ``` So, it's not the win I was expecting, but it's a little better. I'm about to submit the new benchmarks in a separate CL.
So for me the most important part is the ability to inline into TurboFan. If the C++ implementation is reasonably fast, I'm fine to stick with that; that gives us all the benefits we need to safely inline the string iterator next function in the JSBuiltinReducer. I expect that to give us the most benefit (and should be closest to the naive for loop implementation). Since we fixed the keyed access with the private symbols, the CS generated code was fairly close the ideal code, with just a few extra checks that you can avoid with the TurboFan version (plus the TF builtin is always the same performance, while the JS implementation could decrease if the receiver gets polymorphic, which is highly unlikely in the String iterator case). As far as I understand those numbers compare the old JS implementation? Can you also add numbers for ToT then?
On 2016/09/23 17:40:03, Benedikt Meurer wrote: > So for me the most important part is the ability to inline into TurboFan. If the > C++ implementation is reasonably fast, I'm fine to stick with that; that gives > us all the benefits we need to safely inline the string iterator next function > in the JSBuiltinReducer. I expect that to give us the most benefit (and should > be closest to the naive for loop implementation). > > Since we fixed the keyed access with the private symbols, the CS generated code > was fairly close the ideal code, with just a few extra checks that you can avoid > with the TurboFan version (plus the TF builtin is always the same performance, > while the JS implementation could decrease if the receiver gets polymorphic, > which is highly unlikely in the String iterator case). > > As far as I understand those numbers compare the old JS implementation? Can you > also add numbers for ToT then? Sorry, I actually measured those numbers wrong __again__. I should have included the invocation from the command line, both outdir and outdir-no-patch both pointed to the version with the C++ port and TF port reverted (in other words, they were both measuring the JS builtin version). So, another try with the invocation fixed does actually show the improvements for surrogate pairs mentioned earlier. I'll try to include proper information this time: 1) This patch vs the old JS builtins version ``` tools/run_perf.py --arch=x64 --outdir=patched --outdir-no-patch=js_stringiterator test/js-perf-test/JSTests.json >>> Running suite: JSTests/StringIterators >>> Stdout (#1): Spread_OneByteShort-StringIterators(Score): 119400 Spread_TwoByteShort-StringIterators(Score): 265060 Spread_WithSurrogatePairsShort-StringIterators(Score): 202460 ForOf_OneByteShort-StringIterators(Score): 221430 ForOf_TwoByteShort-StringIterators(Score): 595180 ForOf_WithSurrogatePairsShort-StringIterators(Score): 361290 ForOf_OneByteLong-StringIterators(Score): 2206 ForOf_TwoByteLong-StringIterators(Score): 2179 ForOf_WithSurrogatePairsLong-StringIterators(Score): 3350 >>> Stdout (#1) - without patch: Spread_OneByteShort-StringIterators(Score): 117680 Spread_TwoByteShort-StringIterators(Score): 172170 Spread_WithSurrogatePairsShort-StringIterators(Score): 121150 ForOf_OneByteShort-StringIterators(Score): 226660 ForOf_TwoByteShort-StringIterators(Score): 268120 ForOf_WithSurrogatePairsShort-StringIterators(Score): 164130 ForOf_OneByteLong-StringIterators(Score): 2066 ForOf_TwoByteLong-StringIterators(Score): 2056 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1440 ... similar subsequent runs ... ``` 2) This patch vs the C++ version: ``` tools/run_perf.py --arch=x64 --outdir=patched --outdir-no-patch=cpp_string_iterator/ test/js-perf-test/JSTests.json >>> Running suite: JSTests/StringIterators >>> Stdout (#1): Spread_OneByteShort-StringIterators(Score): 136750 Spread_TwoByteShort-StringIterators(Score): 262910 Spread_WithSurrogatePairsShort-StringIterators(Score): 195950 ForOf_OneByteShort-StringIterators(Score): 251400 ForOf_TwoByteShort-StringIterators(Score): 621540 ForOf_WithSurrogatePairsShort-StringIterators(Score): 432830 ForOf_OneByteLong-StringIterators(Score): 2275 ForOf_TwoByteLong-StringIterators(Score): 2330 ForOf_WithSurrogatePairsLong-StringIterators(Score): 3743 >>> Stdout (#1) - without patch: Spread_OneByteShort-StringIterators(Score): 66290 Spread_TwoByteShort-StringIterators(Score): 138760 Spread_WithSurrogatePairsShort-StringIterators(Score): 106850 ForOf_OneByteShort-StringIterators(Score): 86800 ForOf_TwoByteShort-StringIterators(Score): 181980 ForOf_WithSurrogatePairsShort-StringIterators(Score): 130820 ForOf_OneByteLong-StringIterators(Score): 769 ForOf_TwoByteLong-StringIterators(Score): 758 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1178 ... similar subsequent runs ... ``` 3) The C++ version vs the JS builtins version: ``` tools/run_perf.py --arch=x64 --outdir=cpp_string_iterator --outdir-no-patch=js_stringiterator test/js-perf-test/JSTests.json >>> Running suite: JSTests/StringIterators >>> Stdout (#1): Spread_OneByteShort-StringIterators(Score): 67040 Spread_TwoByteShort-StringIterators(Score): 131950 Spread_WithSurrogatePairsShort-StringIterators(Score): 101690 ForOf_OneByteShort-StringIterators(Score): 83330 ForOf_TwoByteShort-StringIterators(Score): 181130 ForOf_WithSurrogatePairsShort-StringIterators(Score): 132050 ForOf_OneByteLong-StringIterators(Score): 799 ForOf_TwoByteLong-StringIterators(Score): 809 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1127 >>> Stdout (#1) - without patch: Spread_OneByteShort-StringIterators(Score): 132660 Spread_TwoByteShort-StringIterators(Score): 192330 Spread_WithSurrogatePairsShort-StringIterators(Score): 117890 ForOf_OneByteShort-StringIterators(Score): 221020 ForOf_TwoByteShort-StringIterators(Score): 258490 ForOf_WithSurrogatePairsShort-StringIterators(Score): 157900 ForOf_OneByteLong-StringIterators(Score): 1820 ForOf_TwoByteLong-StringIterators(Score): 1843 ForOf_WithSurrogatePairsLong-StringIterators(Score): 1413 ... similar subsequent runs ... ``` So, my read is that the C++ version isn't a very strong contender, but that might be okay for a baseline if we can significantly improve this in TF reducers
Thanks for the data. That's very useful to know. One important thing about our (new) strategy is that we want to make sure that the baseline version is pretty good already, as that's what most people get (compared to our old strategy where you'd only get reasonable performance if the calling function makes it to crankshaft with ideally monomorphic feedback). With that in mind, I'd pick the TF version, which seems clearly better than the C++ version. My guess is that the inlined version will benefit the most from avoiding the allocations of the iterator result objects (once our escape analysis is working), but that's to be seen. That being said, I agree that this is a bit too much code for this single purpose in the baseline version. If we can rearrange stuff properly so that it's also usable for/shared with other builtins (i.e. String.fromCodePoint and friends), then it's probably fine tho. Danno volunteered to add support for variable number of arguments to CodeStubAssembler, which will enable us to also port stuff like String.fromCodePoint, etc. So, for now, can you create a CodeStubAssembler::StringFromCodePoint that does the core wrapping? https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:714: assembler->StoreObjectField(iterator, JSStringIterator::kStringOffset, This doesn't need a write barrier, since the object is newly allocated in new space. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:717: assembler->StoreObjectField(iterator, JSStringIterator::kNextIndexOffset, This doesn't need a write barrier, you can safely skip it. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:1141: assembler->StoreObjectField(result, JSIteratorResult::kValueOffset, You can skip write barrier for these two stores because of newly allocated new space object.
For some reason rietveld isn't allowing me to edit one of these draft comments, I was trying to ask what you felt those helpers needed (given that the UnicodeEncoding parameter allows them to be useful for other specified functions) https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:714: assembler->StoreObjectField(iterator, JSStringIterator::kStringOffset, On 2016/09/23 18:14:53, Benedikt Meurer wrote: > This doesn't need a write barrier, since the object is newly allocated in new > space. Done. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:717: assembler->StoreObjectField(iterator, JSStringIterator::kNextIndexOffset, On 2016/09/23 18:14:53, Benedikt Meurer wrote: > This doesn't need a write barrier, you can safely skip it. Done. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:734: static compiler::Node* CreateStringFromCodePointInternal( On 2016/09/23 03:46:24, Benedikt Meurer wrote: > You don't need static inside an anonymous namespace. > I'm also not sure about the encoding, why do you need that? > Because w/o the encoding I think this should be on the CodeStubAssembler as > StringFromCodePoint, which we can also utilize for String.fromCodePoint in the > future. The UnicodeEncoding thing is there to control how the character is represented, which would allow it to be used for String.prototype.codePointAt / String.prototype.fromCodePoint. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:789: Node* lead_offset = > If we can rearrange stuff properly so that it's also usable for/shared with other builtins This code here in particular should be very useful for `String.fromCodePoint()`, that's what the `UnicodeEncoding` parameter was originally added for. Similarly, `String.prototype.codePointAt()` is able to make use of the logic in LoadSurrogatePairsInternal() with the UTF32 encoding. Do you have an idea how to improve the "useful for other methods" aspect of this? https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:1141: assembler->StoreObjectField(result, JSIteratorResult::kValueOffset, On 2016/09/23 18:14:53, Benedikt Meurer wrote: > You can skip write barrier for these two stores because of newly allocated new > space object. Done.
Thanks, getting close. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-... src/builtins/builtins-string.cc:734: static compiler::Node* CreateStringFromCodePointInternal( On 2016/09/23 19:14:16, caitp wrote: > On 2016/09/23 03:46:24, Benedikt Meurer wrote: > > You don't need static inside an anonymous namespace. > > I'm also not sure about the encoding, why do you need that? > > Because w/o the encoding I think this should be on the CodeStubAssembler as > > StringFromCodePoint, which we can also utilize for String.fromCodePoint in the > > future. > > The UnicodeEncoding thing is there to control how the character is represented, > which would allow it to be used for String.prototype.codePointAt / > String.prototype.fromCodePoint. Acknowledged. https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:736: compiler::Node* CreateStringFromCodePointInternal(CodeStubAssembler* assembler, Please move this to the CodeStubAssembler as StringFromCodePoint. https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:749: assembler->Branch( Please restructure this a bit and reuse the existing StringFromCharCode, i.e.: if codepoint < 0x1000 then return StringFromCharCode(codepoint) else result = AllocateSeqTwoByteString(2) store to result according to encoding return result This will also give a small performance boot since StringFromCharCode uses single character cache and thereby avoids allocations. https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1122: assembler->LoadAndUntagObjectField(value, String::kLengthOffset); Nit: You can avoid some untagging and tagging here by just adding the two lengths as Smis.
The CQ bit was checked by caitp@igalia.com 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/2358263002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:749: assembler->Branch( On 2016/09/26 04:15:40, Benedikt Meurer wrote: > Please restructure this a bit and reuse the existing StringFromCharCode, i.e.: > > if codepoint < 0x1000 then > return StringFromCharCode(codepoint) > else > result = AllocateSeqTwoByteString(2) > store to result according to encoding > return result > > This will also give a small performance boot since StringFromCharCode uses > single character cache and thereby avoids allocations. Done. https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-... src/builtins/builtins-string.cc:1122: assembler->LoadAndUntagObjectField(value, String::kLengthOffset); On 2016/09/26 04:15:40, Benedikt Meurer wrote: > Nit: You can avoid some untagging and tagging here by just adding the two > lengths as Smis. Acknowledged. (but, they still get untagged for the purposes of LoadSurrogatePairAt())
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 caitp@igalia.com 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...
Final round of comments. LGTM once comments address. Thanks a lot! https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); Comment says 1, but code says 0? Also why not SimpleInstallFunction(string_iterator_prototype, "next", Builtins::kStringIteratorPrototypeNext, 0, true) here? https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); Nit: add explicit break for readability. https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2465: default: Don't add default here. The C++ compiler cannot flag an error if we ever extend the enum in that case. https://codereview.chromium.org/2358263002/diff/200001/src/objects-debug.cc File src/objects-debug.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/objects-debug.cc#n... src/objects-debug.cc:795: CHECK_EQ(instance_type & 1, 0); Uh, this looks fairly optimized for debug code. Please just keep it extremely simple: CHECK(string()->IsSeqString() || string()->IsExternalString());
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 caitp@igalia.com 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/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/26 18:38:22, Benedikt Meurer wrote: > Comment says 1, but code says 0? > > Also why not SimpleInstallFunction(string_iterator_prototype, "next", > Builtins::kStringIteratorPrototypeNext, 0, true) here? AFAIK this intensionally does not include receiver (based on other callers in this file not including the receiver). https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); On 2016/09/26 18:38:22, Benedikt Meurer wrote: > Nit: add explicit break for readability. Done, although clang-format complains unless I really go crazy with the linebreaks there. https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2465: default: On 2016/09/26 18:38:22, Benedikt Meurer wrote: > Don't add default here. The C++ compiler cannot flag an error if we ever extend > the enum in that case. Done. https://codereview.chromium.org/2358263002/diff/200001/src/objects-debug.cc File src/objects-debug.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/objects-debug.cc#n... src/objects-debug.cc:795: CHECK_EQ(instance_type & 1, 0); On 2016/09/26 18:38:22, Benedikt Meurer wrote: > Uh, this looks fairly optimized for debug code. > Please just keep it extremely simple: > > CHECK(string()->IsSeqString() || string()->IsExternalString()); Done.
https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/27 08:55:56, caitp wrote: > On 2016/09/26 18:38:22, Benedikt Meurer wrote: > > Comment says 1, but code says 0? > > > > Also why not SimpleInstallFunction(string_iterator_prototype, "next", > > Builtins::kStringIteratorPrototypeNext, 0, true) here? > > AFAIK this intensionally does not include receiver (based on other callers in > this file not including the receiver). Yes, but then the comment should say that it takes 0 parameters (not including the receiver) :-)
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/25061)
https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/27 08:58:54, Benedikt Meurer wrote: > On 2016/09/27 08:55:56, caitp wrote: > > On 2016/09/26 18:38:22, Benedikt Meurer wrote: > > > Comment says 1, but code says 0? > > > > > > Also why not SimpleInstallFunction(string_iterator_prototype, "next", > > > Builtins::kStringIteratorPrototypeNext, 0, true) here? > > > > AFAIK this intensionally does not include receiver (based on other callers in > > this file not including the receiver). > > Yes, but then the comment should say that it takes 0 parameters (not including > the receiver) :-) Done. https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); On 2016/09/27 08:55:57, caitp wrote: > On 2016/09/26 18:38:22, Benedikt Meurer wrote: > > Nit: add explicit break for readability. > > Done, although clang-format complains unless I really go crazy with the > linebreaks there. Actually, clang-format isn't happy even with the crazy amount of line endings, so I guess I'll manually commit this if the linebreak is that important.
The CQ bit was checked by caitp@igalia.com 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/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); I mean the keyword break, not a line break.
https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); On 2016/09/27 09:16:02, Benedikt Meurer wrote: > I mean the keyword break, not a line break. Oh, I see. Haven't had much coffee yet this morning :). Done.
https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assemble... src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); Thanks! Now grab a coffee. ;-)
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2358263002/#ps260001 (title: "add "break;" statement to switch case")
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.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [builtins] migrate C++ String Iterator builtins to baseline TurboFan Migrate newly added C++ String Iterator builtins to TFJ builtins, per step 4. of the String Iterator Baseline Implementation section of the design doc BUG=v8:5388 R=bmeurer@chromium.org, mstarzinger@chromium.org ========== to ========== [builtins] migrate C++ String Iterator builtins to baseline TurboFan Migrate newly added C++ String Iterator builtins to TFJ builtins, per step 4. of the String Iterator Baseline Implementation section of the design doc BUG=v8:5388 R=bmeurer@chromium.org, mstarzinger@chromium.org Committed: https://crrev.com/f9a2c8b1112c4e915df8bc5f7ea1fccdf7a33ff8 Cr-Commit-Position: refs/heads/master@{#39765} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f9a2c8b1112c4e915df8bc5f7ea1fccdf7a33ff8 Cr-Commit-Position: refs/heads/master@{#39765}
Message was sent while issue was closed.
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2358263002/diff/260001/src/builtins/builtins-... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/260001/src/builtins/builtins-... src/builtins/builtins-string.cc:543: Node* string_instance_type = assembler->LoadInstanceType(string); This is a bug: instead of |string|, you want to use |var_string.value()| here. As it is, this loop either terminates in the first iteration, or never (causing ~50% of all current Canary crashes to be renderer hangs in this infinite loop). There are three more occurrences of |string| in the loop's body below. I'd suggest this: // line 536 var_string.Bind(assembler->ToThisString(context, receiver, "String.prototype[Symbol.iterator]")); ... assembler->Bind(&loop); { Node* string = var_string.value(); Please add tests for this.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:260001) has been created in https://codereview.chromium.org/2374123005/ by jkummerow@chromium.org. The reason for reverting is: Introduces an infinite loop (see comment).. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
