Bug 181545 – Move ExitProfile to UnlinkedCodeBlock so it can be shared amongst CodeBlocks backed by the same UnlinkedCodeBlock

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

This will follow the theme of putting other optimization information on UnlinkedCodeBlock. This allows the unlinked code cache to remember OSR exit data. Obviously this can make some programs worse: if the same exact code is run in a different way. However, it's probably a solid heuristic in practice, since the same code tends to run in similar ways, and do similar things. Doing this appears to be a 1% speedup on Speedometer.

[Saam Barati]

Created attachment 331152 [details]
WIP

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/bytecode/DFGExitProfile.h:178:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/DFGExitProfile.h:179:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/DFGExitProfile.h:181:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 15 files


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

[Saam Barati]

Created attachment 331236 [details]
patch

[Michael Saboff]

Comment on attachment 331236 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 331236 [details]
patch

Clearing flags on attachment: 331236

Committed r226928: <https://trac.webkit.org/changeset/226928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36486548>

[Saam Barati]

This broke the CLOOP build. Fixing now.

[Saam Barati]

(In reply to Saam Barati from comment #8)
> This broke the CLOOP build. Fixing now.

Fixed CLOOP build in:
https://trac.webkit.org/changeset/226942/webkit

[Filip Pizlo]

Comment on attachment 331236 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:284
> -            && m_graph.baselineCodeBlockFor(originalOrigin.semantic)->hasExitSite(FrequentExitSite(HoistingFailed))) {
> +            && m_graph.hasExitSite(originalOrigin.semantic, HoistingFailed)) {

This introduced a licm-dragons regression because you should have called hasGlobalExitSite.

[Saam Barati]

Comment on attachment 331236 [details]
patch

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

B

>> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:284
>> +            && m_graph.hasExitSite(originalOrigin.semantic, HoistingFailed)) {
> 
> This introduced a licm-dragons regression because you should have called hasGlobalExitSite.

How is this different than what was here before?

[Filip Pizlo]

(In reply to Saam Barati from comment #11)
> Comment on attachment 331236 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331236&action=review
> 
> B
> 
> >> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:284
> >> +            && m_graph.hasExitSite(originalOrigin.semantic, HoistingFailed)) {
> > 
> > This introduced a licm-dragons regression because you should have called hasGlobalExitSite.
> 
> How is this different than what was here before?

Global exit sites don’t store the bytecode index.

[Saam Barati]

(In reply to Filip Pizlo from comment #12)
> (In reply to Saam Barati from comment #11)
> > Comment on attachment 331236 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331236&action=review
> > 
> > B
> > 
> > >> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:284
> > >> +            && m_graph.hasExitSite(originalOrigin.semantic, HoistingFailed)) {
> > > 
> > > This introduced a licm-dragons regression because you should have called hasGlobalExitSite.
> > 
> > How is this different than what was here before?
> 
> Global exit sites don’t store the bytecode index.

I understand that. But the code wasn’t using global exit site before my change either...

[Saam Barati]

(In reply to Saam Barati from comment #13)
> (In reply to Filip Pizlo from comment #12)
> > (In reply to Saam Barati from comment #11)
> > > Comment on attachment 331236 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=331236&action=review
> > > 
> > > B
> > > 
> > > >> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:284
> > > >> +            && m_graph.hasExitSite(originalOrigin.semantic, HoistingFailed)) {
> > > > 
> > > > This introduced a licm-dragons regression because you should have called hasGlobalExitSite.
> > > 
> > > How is this different than what was here before?
> > 
> > Global exit sites don’t store the bytecode index.
> 
> I understand that. But the code wasn’t using global exit site before my
> change either...

Oh wait. Yeah it was. I see