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-

Filip Pizlo
Reported 2012-07-30 17:34:17 PDT
Patch forthcoming.
Attachments
work in progress (25.61 KB, patch)
2012-07-30 17:43 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
almost works (57.04 KB, patch)
2012-07-30 22:58 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
runs benchmarks (on 64-bit) (64.16 KB, patch)
2012-07-30 23:31 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (80.95 KB, patch)
2012-08-01 15:03 PDT, Filip Pizlo 		barraclough: review+
webkit-ews: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2012-07-30 17:43:15 PDT
Created attachment 155408 [details] work in progress
Filip Pizlo
Comment 2 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.
Filip Pizlo
Comment 3 2012-07-30 23:31:44 PDT
Created attachment 155447 [details] runs benchmarks (on 64-bit)
Filip Pizlo
Comment 4 2012-08-01 15:03:37 PDT
Created attachment 155893 [details] the patch
Early Warning System Bot
Comment 5 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
Early Warning System Bot
Comment 6 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
Gavin Barraclough
Comment 7 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?
Gyuyoung Kim
Comment 8 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
Filip Pizlo
Comment 9 2012-08-01 21:33:41 PDT
Landed in http://trac.webkit.org/changeset/124404
Note You need to log in before you can comment on or make changes to this bug.