Bug 102662

Summary: DFG should be able to cache closure calls
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, ojan, oliver, ossy, philn, rakuco, ryuan.choi, sam, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 102759, 102871, 102872, 103146    
Bug Blocks:    
Attachments:
Description Flags
work in progress
none
the patch
oliver: review-, webkit-ews: commit-queue-
the patch
webkit-ews: commit-queue-
the patch
buildbot: commit-queue-
the patch barraclough: review+

Filip Pizlo
Reported 2012-11-19 01:42:35 PST
Patch forthcoming.
Attachments
work in progress (53.11 KB, patch)
2012-11-19 01:46 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (57.88 KB, patch)
2012-11-19 18:30 PST, Filip Pizlo 		oliver: review-
webkit-ews: commit-queue-
DetailsFormatted DiffDiff
the patch (63.94 KB, patch)
2012-11-19 21:14 PST, Filip Pizlo 		webkit-ews: commit-queue-
DetailsFormatted DiffDiff
the patch (66.36 KB, patch)
2012-11-19 21:29 PST, Filip Pizlo 		buildbot: commit-queue-
DetailsFormatted DiffDiff
the patch (66.40 KB, patch)
2012-11-19 23:29 PST, Filip Pizlo 		barraclough: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2012-11-19 01:46:21 PST
Created attachment 174916 [details] work in progress
Filip Pizlo
Comment 2 2012-11-19 18:30:44 PST
Created attachment 175107 [details] the patch
WebKit Review Bot
Comment 3 2012-11-19 18:32:59 PST
Attachment 175107 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/bytecode/CodeBlock.h:822: The parameter name "returnAddress" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/bytecode/CodeBlock.h:822: The parameter name "codeOrigin" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGRepatch.cpp:1230: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/JavaScriptCore/dfg/DFGRepatch.cpp:1234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGRepatch.cpp:1235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 5 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 4 2012-11-19 18:38:54 PST
Comment on attachment 175107 [details] the patch Attachment 175107 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14908388
Early Warning System Bot
Comment 5 2012-11-19 18:41:07 PST
Comment on attachment 175107 [details] the patch Attachment 175107 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14916376
Build Bot
Comment 6 2012-11-19 19:13:26 PST
Comment on attachment 175107 [details] the patch Attachment 175107 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14910422
Oliver Hunt
Comment 7 2012-11-19 19:23:03 PST
Comment on attachment 175107 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=175107&action=review r-, i'd like the armv7 errata removal in a separate patch as it makes it difficult to see exactly what is relevant to this patch (also i don't like playing mix-and-match with back and front end code changes :D ) >> Source/JavaScriptCore/dfg/DFGRepatch.cpp:1230 >> + PassRefPtr<ClosureCallStubRoutine> stubRoutine = adoptRef(new ClosureCallStubRoutine( > > Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Style bot is right on this count, should be a RefPtr
Filip Pizlo
Comment 8 2012-11-19 19:43:27 PST
(In reply to comment #7) > (From update of attachment 175107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175107&action=review > > r-, i'd like the armv7 errata removal in a separate patch as it makes it difficult to see exactly what is relevant to this patch See https://bugs.webkit.org/show_bug.cgi?id=102759 > (also i don't like playing mix-and-match with back and front end code changes :D ) All of the other backend changes (introduction of flipping a branchPtrWithPatch(Condition, RegisterID, TrustedImmPtr) to a jump and vice-versa, and therefore renaming some of the previous patchableBranchPtrWithPatch(Condition, Address, TrustedImmPtr) methods to avoid naming collisions) are directly related to closure calls. So, I could either do them in a separate patch but then introduce dead and untested code temporarily, or I can leave them as part of this patch. Do you have a preference? > > >> Source/JavaScriptCore/dfg/DFGRepatch.cpp:1230 > >> + PassRefPtr<ClosureCallStubRoutine> stubRoutine = adoptRef(new ClosureCallStubRoutine( > > > > Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] > > Style bot is right on this count, should be a RefPtr Will change!
Filip Pizlo
Comment 9 2012-11-19 21:14:53 PST
Created attachment 175131 [details] the patch Fixed style. Factored out errata-related changes into https://bugs.webkit.org/show_bug.cgi?id=102759
WebKit Review Bot
Comment 10 2012-11-19 21:18:45 PST
Attachment 175131 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/jit/ClosureCallStubRoutine.h:42: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/jit/ClosureCallStubRoutine.h:42: The parameter name "executable" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 11 2012-11-19 21:23:51 PST
Comment on attachment 175131 [details] the patch Attachment 175131 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14911469
Early Warning System Bot
Comment 12 2012-11-19 21:25:02 PST
Comment on attachment 175131 [details] the patch Attachment 175131 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14899461
Filip Pizlo
Comment 13 2012-11-19 21:29:26 PST
Created attachment 175135 [details] the patch
Build Bot
Comment 14 2012-11-19 22:03:42 PST
Comment on attachment 175135 [details] the patch Attachment 175135 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14911480
Filip Pizlo
Comment 15 2012-11-19 23:29:22 PST
Created attachment 175153 [details] the patch Fix windows, maybe.
Gavin Barraclough
Comment 16 2012-11-20 10:47:12 PST
Comment on attachment 175153 [details] the patch Looks fine, I'd like to see the assembler changes split out though.
Filip Pizlo
Comment 17 2012-11-20 15:55:13 PST
(In reply to comment #16) > (From update of attachment 175153 [details]) > Looks fine, I'd like to see the assembler changes split out though. Sure, I'll land the asm changes first in a separate patch.
Filip Pizlo
Comment 18 2012-11-20 16:22:57 PST
Landed in http://trac.webkit.org/changeset/135330
Filip Pizlo
Comment 19 2012-11-20 16:23:20 PST
Oops. Didn't mean to close the bug. http://trac.webkit.org/changeset/135330 is just the first part of this.
Filip Pizlo
Comment 20 2012-11-20 17:31:12 PST
Second part landed in http://trac.webkit.org/changeset/135336
Ryuan Choi
Comment 21 2012-11-20 18:31:18 PST
(In reply to comment #20) > Second part landed in http://trac.webkit.org/changeset/135336 Are you still uploading patch? isOnList looks missing yet.
Filip Pizlo
Comment 22 2012-11-20 18:34:57 PST
(In reply to comment #21) > (In reply to comment #20) > > Second part landed in http://trac.webkit.org/changeset/135336 > > Are you still uploading patch? > > isOnList looks missing yet. Gah! I'm sorry about that. I'll land it.
Filip Pizlo
Comment 23 2012-11-20 18:35:33 PST
(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #20) > > > Second part landed in http://trac.webkit.org/changeset/135336 > > > > Are you still uploading patch? > > > > isOnList looks missing yet. > > Gah! I'm sorry about that. I'll land it. Landed in http://trac.webkit.org/changeset/135341
Csaba Osztrogonác
Comment 24 2012-11-20 21:47:13 PST
ARM traditional - https://bugs.webkit.org/show_bug.cgi?id=102871 and MIPS needs followup patches - https://bugs.webkit.org/show_bug.cgi?id=102872
Note You need to log in before you can comment on or make changes to this bug.