Bug 181409 – Reduce graph size by replacing terminal nodes in blocks that have a ForceOSRExit with Unreachable

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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%.

[Saam Barati]

Created attachment 330759 [details]
WIP

running into some validation errors WRT liveness. Looking into it.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 330760 [details]
WIP

[Saam Barati]

Created attachment 330772 [details]
WIP

test EWS

[Saam Barati]

Created attachment 330785 [details]
patch

[Saam Barati]

(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.

[Keith Miller]

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?

[Saam Barati]

(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

[WebKit Commit Bot]

Comment on attachment 330785 [details]
patch

Clearing flags on attachment: 330785

Committed r226655: <https://trac.webkit.org/changeset/226655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36383749>

[Yusuke Suzuki]

(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.

[Saam Barati]

(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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 342315 [details]
WIP

Haven't had time to test it really, but this may be the patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

Created attachment 342499 [details]
patch

[EWS Watchlist]

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.

[Keith Miller]

Comment on attachment 342499 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 342499 [details]
patch

Clearing flags on attachment: 342499

Committed r232742: <https://trac.webkit.org/changeset/232742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

This caused crashes:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r232756%20(3650)/js/dom/JSON-stringify-stderr.txt

[Saam Barati]

(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

[Saam Barati]

This is a 2% ARES-6 progression