Bug 129210 - FTL should do polymorphic PutById inlining
Summary: FTL should do polymorphic PutById inlining
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 112840
  Show dependency treegraph
 
Reported: 2014-02-22 11:05 PST by Filip Pizlo
Modified: 2014-02-24 17:57 PST (History)
14 users (show)

See Also:


Attachments
it begins (14.38 KB, patch)
2014-02-22 11:06 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
profiling side is done (20.05 KB, patch)
2014-02-22 12:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more stuff (55.46 KB, patch)
2014-02-22 20:22 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
almost done (73.43 KB, patch)
2014-02-22 20:38 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the code is written (88.84 KB, patch)
2014-02-23 19:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (94.74 KB, patch)
2014-02-23 20:36 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (94.45 KB, patch)
2014-02-23 20:44 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (92.12 KB, patch)
2014-02-24 09:14 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (94.86 KB, patch)
2014-02-24 09:23 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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