Bug 195913

Summary: Prune code after ForceOSRExit
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186916
Attachments:
Description Flags
patch none

Description Saam Barati 2019-03-18 14:31:28 PDT
There were issues with how we used to do this. So I rolled it out in r242989 thinking it might not be a perf regression to remove it. It turns out that removing this is a 1% speedometer2 regression. I'll add back the optimization but this time making it sound.
Comment 1 Saam Barati 2019-03-18 14:55:06 PDT
Created attachment 365069 [details]
patch
Comment 2 EWS Watchlist 2019-03-18 14:56:25 PDT
Attachment 365069 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7333:  This { should be at the end of the previous line  [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 3 Saam Barati 2019-03-18 15:06:26 PDT
<rdar://problem/48991872>
Comment 4 Keith Miller 2019-03-18 18:55:23 PDT
Comment on attachment 365069 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365069&action=review

r=me.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7400
> +                    node->removeWithoutChecks();

Maybe put a comment here that this is needed to make sure that we don't emit phantoms for the results of opcodes that we are going to remove.
Comment 5 Saam Barati 2019-03-19 11:52:18 PDT
Comment on attachment 365069 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365069&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7400
>> +                    node->removeWithoutChecks();
> 
> Maybe put a comment here that this is needed to make sure that we don't emit phantoms for the results of opcodes that we are going to remove.

I'm not super sure a comment here is super helpful.

We're also removing nodes because we're bumping nodeIndex in the loop below. So we end up inserting phantoms after some of these things we're converting to Check(). So it serves as two purposes: don't emit Phantom on things defined after ForceOSRExit. And don't have those nodes be anything besides a Check()
Comment 6 WebKit Commit Bot 2019-03-19 15:53:46 PDT
Comment on attachment 365069 [details]
patch

Clearing flags on attachment: 365069

Committed r243176: <https://trac.webkit.org/changeset/243176>
Comment 7 WebKit Commit Bot 2019-03-19 15:53:48 PDT
All reviewed patches have been landed.  Closing bug.