The function that runs most in speedometer has about 1100 bytecodes in it (not CodeBlock::instructionCount(), but 1100 bytecode ops). Only about 100 of these bytecode ops ever run. We end up doing a of extra compilation work by keeping control flow after ForceOSRExit. I've hacked up a patch locally that does this: - Blocks that have forceOSRExit remove their terminal and replace it with Unreachable - Place PhantomLocal's for all live-in variables to the ForceOSRExit semantic origin This is quite a hack, but it reduces the total sum of number of basic blacks in the IR after bytecode parsing by ~17% in Speedometer. It seems to reduce compile time by about 3-4%.
Created attachment 330759 [details] WIP running into some validation errors WRT liveness. Looking into it.
(In reply to Saam Barati from comment #1) > Created attachment 330759 [details] > WIP > > running into some validation errors WRT liveness. Looking into it. I think I spot the bug.
Created attachment 330760 [details] WIP
Created attachment 330772 [details] WIP test EWS
Created attachment 330785 [details] patch
(In reply to Saam Barati from comment #5) > Created attachment 330785 [details] > patch Some alternatives that we could do: We can factor out the code into a helper function on Graph that does the transformation. We could then call it from elsewhere since there are other phases that introduce ForceOSRExit nodes. I didn't do this just because it seemed less profitable. We probably introduce the most ForceOSRExit nodes inside the BytecodeParser.
Comment on attachment 330785 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=330785&action=review r=me. > Source/JavaScriptCore/ChangeLog:27 > + it, we could use additional inputs into this mechanism. For example, we could > + profile if a basic block ever executes inside the LLInt/Baseline, and > + remove parts of the CFG based on that. File a bug?
(In reply to Keith Miller from comment #7) > Comment on attachment 330785 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330785&action=review > > r=me. > > > Source/JavaScriptCore/ChangeLog:27 > > + it, we could use additional inputs into this mechanism. For example, we could > > + profile if a basic block ever executes inside the LLInt/Baseline, and > > + remove parts of the CFG based on that. > > File a bug? Filed: https://bugs.webkit.org/show_bug.cgi?id=181447
Comment on attachment 330785 [details] patch Clearing flags on attachment: 330785 Committed r226655: <https://trac.webkit.org/changeset/226655>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36383749>
(In reply to Radar WebKit Bug Importer from comment #11) > <rdar://problem/36383749> Not sure why, but it seems that this cause some regression in assorted-test. https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=typedobj-write-struct-field-standard It would be good if this can fix some minor issues in this patch.
(In reply to Yusuke Suzuki from comment #12) > (In reply to Radar WebKit Bug Importer from comment #11) > > <rdar://problem/36383749> > > Not sure why, but it seems that this cause some regression in assorted-test. > https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=typedobj- > write-struct-field-standard > It would be good if this can fix some minor issues in this patch. It might be worth looking into. What does the test to? It seems really surprising TBH though.
This got rolled out in: https://trac.webkit.org/changeset/230928/webkit But, our bots show rolling this out was a Speedometer 2 regression. Phil showed that rolling it out was a *progression* on certain tests w.r.t OSR entry. I'm going to reland a version of this that's both good for OSR entry and good for Speedometer.
Created attachment 342315 [details] WIP Haven't had time to test it really, but this may be the patch
Attachment 342315 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6867: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342315 [details] WIP Attachment 342315 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8093817 New failing tests: storage/domstorage/storage-close-database-on-idle.html
Created attachment 342352 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 342499 [details] patch
Attachment 342499 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGGraph.cpp:933: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342499 [details] patch r=me.
Comment on attachment 342499 [details] patch Clearing flags on attachment: 342499 Committed r232742: <https://trac.webkit.org/changeset/232742>
This caused crashes: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r232756%20(3650)/js/dom/JSON-stringify-stderr.txt
(In reply to Saam Barati from comment #24) > This caused crashes: > https://build.webkit.org/results/ > Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r232756%20(3650)/js/dom/JSON- > stringify-stderr.txt actually, it seems more likely that this broke it: https://trac.webkit.org/changeset/232741/webkit
This is a 2% ARES-6 progression