Bug 129210

Summary:

FTL should do polymorphic PutById inlining

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

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:

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+

Filip Pizlo

Reported 2014-02-22 11:05:41 PST

Patch forthcoming.

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2014-02-22 11:06:19 PST

Created attachment 224977 [details] it begins

Filip Pizlo

Comment 2 2014-02-22 12:28:39 PST

Created attachment 224980 [details] profiling side is done

Filip Pizlo

Comment 3 2014-02-22 20:22:54 PST

Created attachment 224990 [details] more stuff

Filip Pizlo

Comment 4 2014-02-22 20:38:37 PST

Created attachment 224991 [details] almost done

Filip Pizlo

Comment 5 2014-02-23 19:52:10 PST

Created attachment 225023 [details] the code is written

Filip Pizlo

Comment 6 2014-02-23 20:36:27 PST

Created attachment 225025 [details] the patch

WebKit Commit Bot

Comment 7 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.

Filip Pizlo

Comment 8 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.

Filip Pizlo

Comment 9 2014-02-23 20:44:18 PST

Created attachment 225026 [details] the patch

WebKit Commit Bot

Comment 10 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.

Filip Pizlo

Comment 11 2014-02-24 09:14:04 PST

Created attachment 225071 [details] the patch

WebKit Commit Bot

Comment 12 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.

Filip Pizlo

Comment 13 2014-02-24 09:23:20 PST

Created attachment 225073 [details] the patch Forgot LayoutTests.

WebKit Commit Bot

Comment 14 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.

Oliver Hunt

Comment 15 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.

Filip Pizlo

Comment 16 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.

Mark Hahnenberg

Comment 17 2014-02-24 11:25:43 PST

Comment on attachment 225073 [details] the patch r=me too!

Filip Pizlo

Comment 18 2014-02-24 17:57:38 PST

Landed in http://trac.webkit.org/changeset/164620

Note You need to log in before you can comment on or make changes to this bug.