Bug 129210

Summary: FTL should do polymorphic PutById inlining
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, bunhere, commit-queue, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, rakuco, sam, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 112840    
Attachments:
Description Flags
it begins
none
profiling side is done
none
more stuff
none
almost done
none
the code is written
none
the patch
none
the patch
none
the patch
none
the patch oliver: review+

Description Filip Pizlo 2014-02-22 11:05:41 PST 
Patch forthcoming.
Comment 1 Filip Pizlo 2014-02-22 11:06:19 PST 
Created attachment 224977 [details]
it begins
Comment 2 Filip Pizlo 2014-02-22 12:28:39 PST 
Created attachment 224980 [details]
profiling side is done
Comment 3 Filip Pizlo 2014-02-22 20:22:54 PST 
Created attachment 224990 [details]
more stuff
Comment 4 Filip Pizlo 2014-02-22 20:38:37 PST 
Created attachment 224991 [details]
almost done
Comment 5 Filip Pizlo 2014-02-23 19:52:10 PST 
Created attachment 225023 [details]
the code is written
Comment 6 Filip Pizlo 2014-02-23 20:36:27 PST 
Created attachment 225025 [details]
the patch
Comment 7 WebKit Commit Bot 2014-02-23 20:37:26 PST 
Attachment 225025 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:161:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:187:  The parameter name "chain" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2024:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2028:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2043:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2045:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2052:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:398:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:407:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:409:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:421:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:423:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:434:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:436:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:456:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:461:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:465:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 18 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Filip Pizlo 2014-02-23 20:43:47 PST 
(In reply to comment #7)
> Attachment 225025 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:161:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]

You expected wrong.

> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:187:  The parameter name "chain" adds no information, so it should be removed.  [readability/parameter_name] [5]

Fixed.

> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2024:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2028:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2043:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2045:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2052:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:398:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:407:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:409:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:421:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:423:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:434:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:436:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:456:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:461:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:465:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Fixed.

> Total errors found: 18 in 32 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Filip Pizlo 2014-02-23 20:44:18 PST 
Created attachment 225026 [details]
the patch
Comment 10 WebKit Commit Bot 2014-02-23 20:46:24 PST 
Attachment 225026 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:161:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Filip Pizlo 2014-02-24 09:14:04 PST 
Created attachment 225071 [details]
the patch
Comment 12 WebKit Commit Bot 2014-02-24 09:15:18 PST 
Attachment 225071 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:161:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Filip Pizlo 2014-02-24 09:23:20 PST 
Created attachment 225073 [details]
the patch

Forgot LayoutTests.
Comment 14 WebKit Commit Bot 2014-02-24 09:24:27 PST 
Attachment 225073 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:161:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Oliver Hunt 2014-02-24 10:25:10 PST 
Comment on attachment 225073 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1969
> +        if (!isFTL(m_graph.m_plan.mode)) {
> +            emitPutById(base, identifierNumber, value, isDirect);
> +            return;
> +        }

We will need to support polymorphic put_by_id in the DFG (and baseline, and possibly even the interpreter) to make caching of DOM setters actually useful.
Comment 16 Filip Pizlo 2014-02-24 10:47:38 PST 
(In reply to comment #15)
> (From update of attachment 225073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225073&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1969
> > +        if (!isFTL(m_graph.m_plan.mode)) {
> > +            emitPutById(base, identifierNumber, value, isDirect);
> > +            return;
> > +        }
> 
> We will need to support polymorphic put_by_id in the DFG (and baseline, and possibly even the interpreter) to make caching of DOM setters actually useful.

As I've told you before, we already support caching polymorphic put_by_id in baseline, DFG, and FTL. This patch doesn't change that. This patch has to do with inlining, not caching. 

So if you think that Dom setters are blocked on any of this, then you're in luck because there isn't anything to block on.
Comment 17 Mark Hahnenberg 2014-02-24 11:25:43 PST 
Comment on attachment 225073 [details]
the patch

r=me too!
Comment 18 Filip Pizlo 2014-02-24 17:57:38 PST 
Landed in http://trac.webkit.org/changeset/164620