Bug 162330 – Abstract GetByVal IC out of the JIT class and use in the DFG and FTL JITs

• Status: NEW
• Component: JavaScriptCore

[Saam Barati]

For example, if we emit code that checks if the base is a JSArray cell and has indexingType ArrayWithContiguous, we can speed up ES6 sample bench Air.js by 3-5%.
Otherwise, there is a hot GetByVal inside arrayIteratorValueNext that always calls to the C slow path.

[Saam Barati]

Created attachment 289425 [details]
WIP

[WebKit Commit Bot]

Attachment 289425 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3171:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3194:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2607:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2612:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2615:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:775:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:777:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:778:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 8 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Have you considered just using the baseline JIT's inline caches?

[Build Bot]

Comment on attachment 289425 [details]
WIP

Attachment 289425 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2116226

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289427 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289425 [details]
WIP

Attachment 289425 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2116234

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289428 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289425 [details]
WIP

Attachment 289425 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2116233

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289429 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289425 [details]
WIP

Attachment 289425 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2116239

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289430 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

(In reply to comment #3)
> Have you considered just using the baseline JIT's inline caches?

I didn't, but this seems like it might make more sense. I'll investigate later today.

[Saam Barati]

This will definitely help ES6 Sample Bench, which is nice. But it may also help Speedomer. I'm not 100% sure, but it's possible because last time I measured speedometer, it spent ~3-4% of its time in operationGetByVal, so maybe this patch will cause some inline caching to kick in that wasn't before. I'm not 100% if it will, because I haven't looked at what the speedometer getByVal code gets compile to in the DFG/FTL.

[Saam Barati]

Created attachment 289517 [details]
WIP

Doesn't work yet, but I've moved most of the stuff out of the JIT class. I need to write it up now in the baseline/DFG/FTL and implement a repatching policy.

[Saam Barati]

Created attachment 289635 [details]
WIP

It passes a fair amount of tests, but still has bugs. I also still need to tune performance, probably, because I never emit an inline path. I'll fix that.

[Saam Barati]

Created attachment 289658 [details]
WIP

I think this now passes most JSC tests, just a few remaining bugs to work out. Then, I need to focus on tuning performance and implementing GetByVal with string/symbol as the property type.

[Saam Barati]

Created attachment 289661 [details]
WIP

Nevermind, the previous patch failed a bunch of stuff. I think this actually starts to  pass almost everything.

[Saam Barati]

Created attachment 289722 [details]
WIP

I think all tests pass now. There are some programs where emitting an inline path is really important. My next step is to implement a reusable mechanism to do this in the baseline/DFG/FTL JIT. Hopefully polymorphic access can then just use this mechanism to emit code, too. There are some things we're still slower on. I'm confident we can make this patch be a pure win over the old behavior, it will just require some tuning. I also think it'll be a win for truly polymorphic cases, like we see in ES6SampleBench/Air

[WebKit Commit Bot]

Attachment 289722 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:547:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:123:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:124:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:156:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:308:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:311:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:312:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:322:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1424:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1460:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:2632:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1769:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1779:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1794:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1849:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1865:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:772:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:775:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:392:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:393:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:394:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ByValInfo.h:264:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ByValInfo.h:264:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:559:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:560:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:561:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:567:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:568:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:570:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:586:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:588:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:589:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:591:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2619:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2634:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2636:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2671:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2673:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2674:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2675:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2676:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2678:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2679:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2680:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2681:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2683:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 47 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 289722 [details]
WIP

Attachment 289722 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2134571

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289731 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289722 [details]
WIP

Attachment 289722 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2134572

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289732 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289722 [details]
WIP

Attachment 289722 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2134577

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289733 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289722 [details]
WIP

Attachment 289722 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2134591

New failing tests:
js/basic-strict-mode.html

[Build Bot]

Created attachment 289734 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

Created attachment 289906 [details]
WIP

So things are now correct, but I've been introducing various hacks just to try to maintain performance parity on a few benchmarks. I'm maybe 0.5% slower on octane, and neutral on Kraken, but a few tests on Kraken/Octane are clearly slower, so I'm going to try prevent any regressions on those. I'll do more tuning work on this tomorrow. For a few micro benchmarks, this patch is definitely faster than what we had before. On ES6 sample bench/ Air it's still 5% faster. So there are some programs that indeed do prefer this patch over our old behavior.

[Saam Barati]

Created attachment 290130 [details]
benchmarks with DFG/FTL disabled

I'm going to take a break from this patch for a day or two so I can come back to it with fresher eyes. I'll work on random bug fixing or other things in the mean time.

I did a microbenchmarks run with the DFG/FTL disabled just to measure perf in baseline. It seems slower on a few things, but faster on what should be faster. I still need to deal with some regressions when running kraken/octane. I'm running certain benchmarks with DFG/FTL disabled just to get an idea of the code gen we emit when we repatch. Various kraken tests are 1-2% slower, and kraken/ai-astar is 1 faster. I think that this patch should be good to go when it's no slower on common benchmarks with the DFG/FTL disabled.