Bug 92696

Summary: DFG should hoist structure checks
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo, ggaren, gustavo, mhahnenberg, oliver, philn, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 92728    
Bug Blocks: 91933    
Attachments:
Description Flags
work in progress
none
almost works
none
runs benchmarks (on 64-bit)
none
the patch barraclough: review+, webkit-ews: commit-queue-

Description Filip Pizlo 2012-07-30 17:34:17 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2012-07-30 17:43:15 PDT
Created attachment 155408 [details]
work in progress
Comment 2 Filip Pizlo 2012-07-30 22:58:56 PDT
Created attachment 155443 [details]
almost works

It turns out that to do this right you need the CFA to track the last CheckStructure performed on a variable even if it was subsequently clobbered, so that OSR entry can validate whether the variable still has that structure. Otherwise the subsequent transition watchpoints would not work.
Comment 3 Filip Pizlo 2012-07-30 23:31:44 PDT
Created attachment 155447 [details]
runs benchmarks (on 64-bit)
Comment 4 Filip Pizlo 2012-08-01 15:03:37 PDT
Created attachment 155893 [details]
the patch
Comment 5 Early Warning System Bot 2012-08-01 15:16:36 PDT
Comment on attachment 155893 [details]
the patch

Attachment 155893 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13419235
Comment 6 Early Warning System Bot 2012-08-01 15:21:29 PDT
Comment on attachment 155893 [details]
the patch

Attachment 155893 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13411585
Comment 7 Gavin Barraclough 2012-08-01 17:18:01 PDT
Comment on attachment 155893 [details]
the patch

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

r+, unsure about the lack of any kind of enum / name for the ballot constants.

> Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp:412
> +        noticeStructureCheck(variable, set.singletonStructure());

I feel like I've seen this idiom before - I could be wrong?
- maybe we want a singletonStructureOrNull, or just make singletonStructure() provide this behavior by default?

> Source/JavaScriptCore/dfg/DFGVariableAccessData.h:156
> +        m_votes[1] = 0;

o_O

0 & 1 - are you sure?
Comment 8 Gyuyoung Kim 2012-08-01 17:21:34 PDT
Comment on attachment 155893 [details]
the patch

Attachment 155893 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13421225
Comment 9 Filip Pizlo 2012-08-01 21:33:41 PDT
Landed in http://trac.webkit.org/changeset/124404