Bug 155862

Created attachment 274871 [details] WIP WIP is here mostly for the C++ RegExpExec function. But this patch is wrong w.r.t how we make test into an intrinsic.

<rdar://problem/25770509>

Created attachment 277402 [details] work in progress: waiting for bug 156562 for accepted pattern for making test an intrinsic.

NOTE: This is the last test case failing in compat-table :) https://github.com/kangax/compat-table/commit/faddf0016be9959761ea5ad93d25e6e4d41c3196

Created attachment 277597 [details] work in progress. This patch passes all tests. It has the RegExpTest intrinsics implemented but perf is still bad. The current regression is as follows: base new simple-regexp-test-folding-fail 3.9705+-0.0665 ! 19.8793+-0.4888 ! definitely 5.0068x slower simple-regexp-test-folding 4.8262+-0.1007 ! 21.2555+-0.3805 ! definitely 4.4042x slower Before the intrinsics were implemented, we were getting about a 10x perf regression on these microbenchmarks. Now, we're around 5x slower. I still need to dig in a bit more to investigate what's causing the regression.

Attachment 277597 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Here's the relevant IR before lowering to B3: Old code: ... 28:<!0:-> LoopHint(MustGen, W:SideState, bc#13) 89:<!0:-> KillStack(MustGen, loc10, W:SideState, ClobbersExit, bc#14) 30:<!0:-> ZombieHint(MustGen, loc10, W:SideState, ClobbersExit, bc#14, ExitInvalid) 90:<!0:-> KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#17) 35:<!0:-> ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#17, ExitInvalid) 91:<!0:-> KillStack(MustGen, loc9, W:SideState, ClobbersExit, bc#26) 38:<!0:-> ZombieHint(MustGen, loc9, W:SideState, ClobbersExit, bc#26, ExitInvalid) 97:<!0:-> StoreBarrier(KnownCell:@60, MustGen, W:SideState, bc#29) 64:<!0:-> RecordRegExpCachedResult(KnownCell:@60, KnownCell:@61, KnownCell:@37, KnownInt32:@22, KnownInt32:@63, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#29) 92:<!0:-> KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid) 43:<!0:-> ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid) 50:<!3:-> ArithAdd(Int32:Kill:@73, Int32:@49, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#38) 93:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#38) 51:<!0:-> MovHint(Untyped:@50, MustGen, loc5, W:SideState, ClobbersExit, bc#38, ExitInvalid) 54:< 1:-> CompareLess(Int32:@50, Int32:@25, Boolean|UseAsOther, Bool, Exits, bc#40) 55:<!0:-> Branch(KnownBoolean:Kill:@54, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#40, ExitInvalid) ... New code: ... 28:<!0:-> LoopHint(MustGen, W:SideState, bc#13) 29:< 2:-> NewRegexp(JS|UseAsOther, Regexpobject, R:HeapObjectCount, W:HeapObjectCount, Exits, bc#14) <============= NEW 92:<!0:-> KillStack(MustGen, loc10, W:SideState, ClobbersExit, bc#14) 30:<!0:-> MovHint(Untyped:@29, MustGen, loc10, W:SideState, ClobbersExit, bc#14, ExitInvalid) 93:<!0:-> KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#17) 35:<!0:-> MovHint(Untyped:@34, MustGen, loc7, W:SideState, ClobbersExit, bc#17, ExitInvalid) 94:<!0:-> KillStack(MustGen, loc9, W:SideState, ClobbersExit, bc#26) 38:<!0:-> MovHint(Untyped:@37, MustGen, loc9, W:SideState, ClobbersExit, bc#26, ExitInvalid) 41:< 1:-> TryGetById(Cell:Kill:@29, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#29) <============= NEW 42:<!0:-> CheckCell(Check:Cell:Kill:@41, MustGen, <0x10d5d70a0, Function>, <host function>, Exits, bc#29) <============= NEW 100:<!0:-> StoreBarrier(KnownCell:@63, MustGen, W:SideState, bc#29) 67:<!0:-> RecordRegExpCachedResult(KnownCell:@63, KnownCell:@64, KnownCell:@37, KnownInt32:@22, KnownInt32:@66, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#29) 95:<!0:-> KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid) 46:<!0:-> ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid) 53:<!3:-> ArithAdd(Int32:Kill:@76, Int32:@52, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#38) 96:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#38) 54:<!0:-> MovHint(Untyped:@53, MustGen, loc5, W:SideState, ClobbersExit, bc#38, ExitInvalid) 57:< 1:-> CompareLess(Int32:@53, Int32:@25, Boolean|UseAsOther, Bool, Exits, bc#40) 58:<!0:-> Branch(KnownBoolean:Kill:@57, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#40, ExitInvalid) ... Because of the need for the TryGetById, we'll end up keeping the NewRegexp alive. We don't currently constant fold away the TryGetById. This ends up doing work in the generated code that accounts for the 5x regression in the simple-regexp-test-folding* micro-benchmarks. We are successful in folding away the call to RegExp.prototype.test. Fil said that this is sufficient. The work to constant fold TryGetByid is out of scope for this bug. Currently running full benchmarks on the patch. If all goes well, will submit for review.

I recommend adding a new, modified version of this microbenchmark that hoists the regexp allocation out of the loop. This way, we'll have both a test to remind us to constant-fold tryGetById (the current version of the test) and a new test that isolates the profitability of constant-folding RegExpTest (the new version of the test). Once all bugs are fixed, the two tests should perform the same, but it's not a top priority.

Created attachment 277651 [details] proposed patch. Rebased patch + real ChangeLog. Still waiting for benchmark results.

Attachment 277651 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #8) > I recommend adding a new, modified version of this microbenchmark that > hoists the regexp allocation out of the loop. Great idea. Here are the new micro-benchmark results: base new simple-regexp-test-folding-fail-with-hoisted-regexp 18.3394+-0.1967 ! 20.4064+-0.2284 ! definitely 1.1127x slower simple-regexp-test-folding-fail 4.1179+-0.1119 ! 20.1755+-0.2900 ! definitely 4.8995x slower simple-regexp-test-folding-with-hoisted-regexp 17.9460+-0.3154 ! 20.1089+-0.2910 ! definitely 1.1205x slower simple-regexp-test-folding 4.9317+-0.0701 ! 21.8603+-0.2295 ! definitely 4.4326x slower With the RegExp construction in the loop, we have the 5x regression. With the RegExp construction hoisted out of the loop, we have about a 12% regression. The difference is because in the new code, we still need to do work to do the TryGetById and CheckCell. Here's the IR before lowering to B3: 33:<!0:-> LoopHint(MustGen, W:SideState, bc#16) ... 46:< 1:-> TryGetById(Cell:@70, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#32) <======== NEW 47:<!0:-> CheckCell(Check:Cell:Kill:@46, MustGen, <0x10ddd70a0, Function>, <host function>, Exits, bc#32) <======== NEW 50:<!0:-> RegExpTest(KnownCell:@49, RegExpObject:@70, String:@42, JS|MustGen|PureInt, Bool, R:RegExpObject_lastIndex,RegExpState, W:RegExpObject_lastIndex,RegExpState, Exits, ClobbersExit, bc#32) predicting Bool 98:<!0:-> KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid) 51:<!0:-> ZombieHint(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid) 58:<!3:-> ArithAdd(Int32:Kill:@89, Int32:@57, Number|MustGen|UseAsOther, Int32, CheckOverflow, Exits, bc#41) 99:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#41) 59:<!0:-> MovHint(Untyped:@58, MustGen, loc5, W:SideState, ClobbersExit, bc#41, ExitInvalid) 62:< 1:-> CompareLess(Int32:@58, Int32:@61, Boolean|UseAsOther, Bool, Exits, bc#43) 63:<!0:-> Branch(KnownBoolean:Kill:@62, MustGen, T:#1/w:10.000000, F:#3/w:1.000000, W:SideState, bc#43, ExitInvalid) In this test, we're not folding away the RegExpTest though in both the old and new code.

(In reply to comment #11) > 50:<!0:-> RegExpTest(KnownCell:@49, RegExpObject:@70, String:@42, > JS|MustGen|PureInt, Bool, R:RegExpObject_lastIndex,RegExpState, > W:RegExpObject_lastIndex,RegExpState, Exits, ClobbersExit, bc#32) > predicting Bool ... > > In this test, we're not folding away the RegExpTest though in both the old > and new code. Saam looked at this with me and noticed that the reason RegExpTest is not folded is because we're OSR entering into the loop, and the loop doesn't know that the RegExp object is constant. Here's the original test: (function() { var regExp = /foo/; for (var i = 0; i < 1000000; ++i) regExp.test("foo"); })(); Here's the new version of the test that will allow us to fold RegExpTest: (function() { for (var j = 0; j < 10; ++j) { (function () { var regExp = /foo/; for (var i = 0; i < 100000; ++i) regExp.test("foo"); })(); } })(); With the new versions of the tests with the RegExp object hoisted out of the loop, the micro-benchmark results are: base new simple-regexp-test-folding-fail-with-hoisted-regexp 9.7717+-0.0654 ! 11.6440+-0.3730 ! definitely 1.1916x slower simple-regexp-test-folding-fail 3.9023+-0.0503 ! 18.8741+-0.1755 ! definitely 4.8367x slower simple-regexp-test-folding-with-hoisted-regexp 10.3621+-0.0655 ! 12.4605+-0.1900 ! definitely 1.2025x slower simple-regexp-test-folding 4.6939+-0.0462 ! 20.3603+-0.1146 ! definitely 4.3376x slower With the new code, the IR before lowering to B3 is: 33:<!0:-> LoopHint(MustGen, W:SideState, bc#16) 98:<!0:-> KillStack(MustGen, loc12, W:SideState, ClobbersExit, bc#17) 35:<!0:-> MovHint(Untyped:@24, MustGen, loc12, W:SideState, ClobbersExit, bc#17, ExitInvalid) 99:<!0:-> KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#20) 40:<!0:-> MovHint(Untyped:@39, MustGen, loc8, W:SideState, ClobbersExit, bc#20, ExitInvalid) 100:<!0:-> KillStack(MustGen, loc11, W:SideState, ClobbersExit, bc#29) 43:<!0:-> MovHint(Untyped:@42, MustGen, loc11, W:SideState, ClobbersExit, bc#29, ExitInvalid) 46:< 1:-> TryGetById(Cell:@24, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#32) <======= NEW 47:<!0:-> CheckCell(Check:Cell:Kill:@46, MustGen, <0x1139d70a0, Function>, <host function>, Exits, bc#32) <======= NEW 118:<!0:-> StoreBarrier(KnownCell:@107, MustGen, W:SideState, bc#32) 111:<!0:-> RecordRegExpCachedResult(KnownCell:@107, KnownCell:@108, KnownCell:@42, KnownInt32:@27, KnownInt32:@110, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#32) <========= RegExpTest is folded. 101:<!0:-> KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid) 51:<!0:-> ZombieHint(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid) 58:<!3:-> ArithAdd(Int32:Kill:@80, Int32:@57, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#41) 102:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#41) 59:<!0:-> MovHint(Untyped:@58, MustGen, loc5, W:SideState, ClobbersExit, bc#41, ExitInvalid) 62:< 1:-> CompareLess(Int32:@58, Int32:@30, Boolean|UseAsOther, Bool, Exits, bc#43) 63:<!0:-> Branch(KnownBoolean:Kill:@62, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#43, ExitInvalid)

Created attachment 277660 [details] proposed patch with the new hoisted regexp tests.

Created attachment 277661 [details] x86_64 benchmark results. Benchmark results shows that perf is neutral except for the simple-regexp-test-folding* micro-benchmarks which have a 5% regression as previously stated.

Attachment 277660 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 277711 [details] in-browser sunspider 1.0.2 results on old code

Created attachment 277712 [details] in-browser sunspider 1.0.2 results on new code

In browser sunspider results are neutral. Jetstream results also appear to be neutral. The score fluctuates up and down by a few points (on both the old and new code) but they are both appear to be near each other's scores. For example: Old code: Score 184.50 ± 2.7204 Benchmark Average Score Benchmark Average Score Benchmark Average Score Latency 119.99 ± 2.3427 regex-dna 118.3 ± 15.61 gcc-loops.cpp 383.3 ± 21.08 3d-cube 113.5 ± 7.133 splay-latency 165.7 ± 3.484 hash-map 333.7 ± 10.24 3d-raytrace 113.4 ± 5.199 tagcloud 145.2 ± 37.59 mandreel 289.1 ± 14.04 base64 80.56 ± 14.79 typescript 128.7 ± 9.043 n-body.c 243.2 ± 11.67 cdjs 183.9 ± 44.67 Throughput 257.28 ± 6.9501 navier-stokes 176.2 ± 5.632 code-first-load 120.3 ± 1.440 bigfib.cpp 318.7 ± 45.19 pdfjs 198.2 ± 10.79 code-multi-load 111.1 ± 5.464 box2d 225.9 ± 18.56 proto-raytracer 308.3 ± 5.182 crypto-aes 129.8 ± 3.637 container.cpp 294.1 ± 13.73 quicksort.c 266.6 ± 2.175 crypto-md5 99.45 ± 2.350 crypto 181.0 ± 9.191 regexp-2010 297.2 ± 2.023 crypto-sha1 100.9 ± 6.996 delta-blue 280.0 ± 5.110 richards 206.1 ± 7.673 date-format-tofte 90.47 ± 3.063 dry.c 318.6 ± 43.54 splay 250.8 ± 11.13 date-format-xparb 124.8 ± 24.96 earley-boyer 177.1 ± 7.652 towers.c 255.5 ± 18.35 mandreel-latency 164.4 ± 15.86 float-mm.c 333.8 ± 14.60 zlib 217.9 ± 5.861 n-body 99.45 ± 2.350 gbemu 240.6 ± 14.15 Geometric Mean 184.50 ± 2.7204 New code: Score 188.95 ± 0.65793 Benchmark Average Score Benchmark Average Score Benchmark Average Score Latency 123.45 ± 3.1012 regex-dna 125.6 ± 2.495 gcc-loops.cpp 390.1 ± 9.796 3d-cube 110.0 ± 13.99 splay-latency 193.6 ± 10.36 hash-map 329.8 ± 14.68 3d-raytrace 112.3 ± 1.121 tagcloud 152.4 ± 6.790 mandreel 300.6 ± 18.21 base64 80.61 ± 8.616 typescript 134.9 ± 10.35 n-body.c 242.0 ± 12.24 cdjs 188.1 ± 12.06 Throughput 262.56 ± 4.0256 navier-stokes 175.5 ± 4.897 code-first-load 123.6 ± 5.641 bigfib.cpp 314.4 ± 10.81 pdfjs 199.9 ± 4.839 code-multi-load 115.2 ± 1.724 box2d 231.2 ± 7.873 proto-raytracer 306.3 ± 3.891 crypto-aes 127.7 ± 1.766 container.cpp 290.4 ± 20.66 quicksort.c 266.8 ± 1.428 crypto-md5 100.0 ± 0.000 crypto 182.9 ± 12.09 regexp-2010 294.7 ± 15.74 crypto-sha1 101.7 ± 3.583 delta-blue 277.7 ± 4.190 richards 202.6 ± 7.360 date-format-tofte 91.14 ± 14.74 dry.c 309.1 ± 66.86 splay 265.5 ± 22.37 date-format-xparb 130.0 ± 3.969 earley-boyer 169.5 ± 10.91 towers.c 259.3 ± 4.292 mandreel-latency 176.8 ± 13.68 float-mm.c 339.0 ± 9.809 zlib 216.7 ± 14.39 n-body 99.45 ± 2.350 gbemu 369.0 ± 12.60 Geometric Mean 188.95 ± 0.65793

Comment on attachment 277660 [details] proposed patch with the new hoisted regexp tests. View in context: https://bugs.webkit.org/attachment.cgi?id=277660&action=review leaving as r? because I have a few questions. Mostly LGTM > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2224 > + Node* regExpObject = get(virtualRegisterForArgument(0, registerOffset)); You should only add this get to the graph if we know we're going to make it an intrinsic (i.e, not return false). > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228 > + if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) > + return false; Couldn't we also exit for BadType here? Do we want to take that into account? > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:658 > + GlobalPropertyInfo(vm.propertyNames->regExpTestFastPrivateName, JSFunction::create(vm, this, 1, String(), regExpProtoFuncTestFast, RegExpTestIntrinsic), DontEnum | DontDelete | ReadOnly), shouldn't this be RegExpTestFastIntrinsic?

(In reply to comment #19) > Comment on attachment 277660 [details] > proposed patch with the new hoisted regexp tests. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277660&action=review > > leaving as r? because I have a few questions. Mostly LGTM > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2224 > > + Node* regExpObject = get(virtualRegisterForArgument(0, registerOffset)); > > You should only add this get to the graph if we know we're going to make it > an intrinsic (i.e, not return false). Good point. Will fix. > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228 > > + if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) > > + return false; > > Couldn't we also exit for BadType here? Do we want to take that into account? Let me look into that. > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:658 > > + GlobalPropertyInfo(vm.propertyNames->regExpTestFastPrivateName, JSFunction::create(vm, this, 1, String(), regExpProtoFuncTestFast, RegExpTestIntrinsic), DontEnum | DontDelete | ReadOnly), > > shouldn't this be RegExpTestFastIntrinsic? Eeek. Thanks for catching that. Will fix and redo tests and benchmarks.

(In reply to comment #20) > (In reply to comment #19) > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228 > > > + if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) > > > + return false; > > > > Couldn't we also exit for BadType here? Do we want to take that into account? > > Let me look into that. Here's what I found: 1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() && callLinkStatus.size() == 1)" before attempting to inline the intrinsic. 2. If we ever bail due to BadType at this call site, callLinkStatus.couldTakeSlowPath() will return true. This ensures that we don't try to inline it again. In addition, if we ever change the inlining code to support multiple call targets in the future, we don't want to invalidate the inlining of this intrinsic if we happened to encounter a BadType. So, in summary, we don't need to and shouldn't add a BadType check here. The existing callLinkStatus.couldTakeSlowPath() check is sufficient.

(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228 > > > > + if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) > > > > + return false; > > > > > > Couldn't we also exit for BadType here? Do we want to take that into account? > > > > Let me look into that. > > Here's what I found: > 1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() && > callLinkStatus.size() == 1)" before attempting to inline the intrinsic. > 2. If we ever bail due to BadType at this call site, > callLinkStatus.couldTakeSlowPath() will return true. > This ensures that we don't try to inline it again. > > In addition, if we ever change the inlining code to support multiple call > targets in the future, we don't want to invalidate the inlining of this > intrinsic if we happened to encounter a BadType. > > So, in summary, we don't need to and shouldn't add a BadType check here. > The existing callLinkStatus.couldTakeSlowPath() check is sufficient. I agree with this analysis.

Created attachment 277740 [details] proposed patch w/ Saam's feedback applied.

Created attachment 277741 [details] in-browser sunspider 1.0.2 results on old code

Created attachment 277742 [details] in-browser sunspider 1.0.2 results on new code

Attachment 277740 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228 > > > > + if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) > > > > + return false; > > > > > > Couldn't we also exit for BadType here? Do we want to take that into account? > > > > Let me look into that. > > Here's what I found: > 1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() && > callLinkStatus.size() == 1)" before attempting to inline the intrinsic. > 2. If we ever bail due to BadType at this call site, > callLinkStatus.couldTakeSlowPath() will return true. > This ensures that we don't try to inline it again. > > In addition, if we ever change the inlining code to support multiple call > targets in the future, we don't want to invalidate the inlining of this > intrinsic if we happened to encounter a BadType. > > So, in summary, we don't need to and shouldn't add a BadType check here. > The existing callLinkStatus.couldTakeSlowPath() check is sufficient. Interesting. It looks like 'StringPrototypeReplaceIntrinsic' is wrong for checking for BadType exits then.

Created attachment 277753 [details] x86_64 benchmark results. Perf appears to be neutral, again with the exception of the 2 simple-regexp-test-folding* microbenchmarks. LongSpider's 3d-cube regression is sometimes reproducible if I re-run the test harness (with --outer 8) with just that test. However, the test itself does not use RegExp.prototype.test. So, I'll consider this noise. There is one other exception though: JSRegress's math-with-out-of-bounds-array-values is consistently about 10-11% faster no matter how many times I re-run it. That test also does not use RegExp.prototype.test. I have no idea how this could have happened. Anyway, perf is neutral on all important benchmarks.

JetStream numbers on the latest (i.e. 2016-49-29 15:48 PDT) patch are also neutral: Old code: Score 190.79 ± 1.3665 Benchmark Average Score Benchmark Average Score Benchmark Average Score Latency 124.03 ± 1.1093 regex-dna 124.9 ± 3.785 gcc-loops.cpp 389.8 ± 3.140 3d-cube 115.5 ± 22.54 splay-latency 194.1 ± 13.04 hash-map 339.1 ± 14.44 3d-raytrace 113.1 ± 2.289 tagcloud 150.7 ± 23.73 mandreel 303.0 ± 13.49 base64 81.29 ± 1.124 typescript 135.7 ± 1.001 n-body.c 244.4 ± 3.383 cdjs 185.3 ± 1.527 Throughput 266.13 ± 3.1652 navier-stokes 177.9 ± 3.383 code-first-load 124.3 ± 0.4246 bigfib.cpp 302.5 ± 7.990 pdfjs 206.8 ± 1.001 code-multi-load 115.0 ± 2.972 box2d 229.8 ± 0.8704 proto-raytracer 309.7 ± 8.418 crypto-aes 129.0 ± 3.637 container.cpp 296.4 ± 3.204 quicksort.c 267.8 ± 3.615 crypto-md5 100.0 ± 0.000 crypto 186.6 ± 3.534 regexp-2010 299.8 ± 5.900 crypto-sha1 100.9 ± 6.996 delta-blue 280.3 ± 5.327 richards 207.0 ± 18.57 date-format-tofte 94.96 ± 6.326 dry.c 326.0 ± 47.76 splay 265.7 ± 14.34 date-format-xparb 131.0 ± 29.91 earley-boyer 173.6 ± 3.718 towers.c 260.5 ± 8.724 mandreel-latency 174.0 ± 26.39 float-mm.c 342.3 ± 0.6296 zlib 221.9 ± 4.360 n-body 100.0 ± 0.000 gbemu 379.2 ± 15.20 Geometric Mean 190.79 ± 1.3665 New code: Score 190.99 ± 1.0815 Benchmark Average Score Benchmark Average Score Benchmark Average Score Latency 124.09 ± 1.1507 regex-dna 124.3 ± 7.522 gcc-loops.cpp 392.4 ± 3.327 3d-cube 109.3 ± 9.194 splay-latency 195.3 ± 10.98 hash-map 325.2 ± 36.04 3d-raytrace 109.8 ± 8.434 tagcloud 158.2 ± 4.268 mandreel 304.7 ± 5.919 base64 84.00 ± 0.000 typescript 132.9 ± 6.359 n-body.c 245.1 ± 2.654 cdjs 186.8 ± 27.96 Throughput 266.51 ± 2.8073 navier-stokes 177.8 ± 3.212 code-first-load 124.1 ± 0.5710 bigfib.cpp 321.5 ± 1.714 pdfjs 204.3 ± 3.084 code-multi-load 113.5 ± 2.092 box2d 227.9 ± 3.253 proto-raytracer 313.9 ± 2.247 crypto-aes 127.7 ± 1.766 container.cpp 298.2 ± 7.238 quicksort.c 267.8 ± 5.045 crypto-md5 100.0 ± 0.000 crypto 188.9 ± 0.8679 regexp-2010 297.2 ± 2.875 crypto-sha1 102.5 ± 0.000 delta-blue 283.2 ± 10.42 richards 213.2 ± 6.081 date-format-tofte 91.29 ± 2.079 dry.c 310.2 ± 62.43 splay 265.9 ± 20.74 date-format-xparb 135.9 ± 11.52 earley-boyer 175.8 ± 1.957 towers.c 260.5 ± 1.252 mandreel-latency 178.9 ± 2.267 float-mm.c 341.6 ± 1.254 zlib 223.0 ± 1.228 n-body 100.0 ± 0.000 gbemu 375.4 ± 6.460 Geometric Mean 190.99 ± 1.0815

Comment on attachment 277740 [details] proposed patch w/ Saam's feedback applied. View in context: https://bugs.webkit.org/attachment.cgi?id=277740&action=review r=me > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238 > + auto isRegExpPropertySame = [&] (JSValue primordialProperty, UniquedStringImpl* propertyUID) { Nit: It might be worth converging this with other implementation. Up to you

(In reply to comment #30) > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238 > > + auto isRegExpPropertySame = [&] (JSValue primordialProperty, UniquedStringImpl* propertyUID) { > > Nit: It might be worth converging this with other implementation. Up to you I'll look into doing that in a subsequent patch.

Thanks for the review. Landed in r200272: <http://trac.webkit.org/r200272>.