Bug 102662 - DFG should be able to cache closure calls
Summary: DFG should be able to cache closure calls
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: 102759 102871 102872 103146
Blocks:
  Show dependency tree / graph
 
Reported: 2012-11-19 01:42 PST by Filip Pizlo
Modified: 2013-03-18 10:31 PDT (History)
17 users (show)

See Also:

Attachments
work in progress (53.11 KB, patch)
2012-11-19 01:46 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (57.88 KB, patch)
2012-11-19 18:30 PST, Filip Pizlo 		oliver: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (63.94 KB, patch)
2012-11-19 21:14 PST, Filip Pizlo 		webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (66.36 KB, patch)
2012-11-19 21:29 PST, Filip Pizlo 		buildbot: commit-queue-
Details | Formatted Diff | Diff
the patch (66.40 KB, patch)
2012-11-19 23:29 PST, Filip Pizlo 		barraclough: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-19 01:42:35 PST 
Patch forthcoming.
Comment 1 Filip Pizlo 2012-11-19 01:46:21 PST 
Created attachment 174916 [details]
work in progress
Comment 2 Filip Pizlo 2012-11-19 18:30:44 PST 
Created attachment 175107 [details]
the patch
Comment 3 WebKit Review Bot 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.
Comment 4 Early Warning System Bot 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
Comment 5 Early Warning System Bot 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
Comment 6 Build Bot 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
Comment 7 Oliver Hunt 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
Comment 8 Filip Pizlo 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!
Comment 9 Filip Pizlo 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
Comment 10 WebKit Review Bot 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.
Comment 11 Early Warning System Bot 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
Comment 12 Early Warning System Bot 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
Comment 13 Filip Pizlo 2012-11-19 21:29:26 PST 
Created attachment 175135 [details]
the patch
Comment 14 Build Bot 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
Comment 15 Filip Pizlo 2012-11-19 23:29:22 PST 
Created attachment 175153 [details]
the patch

Fix windows, maybe.
Comment 16 Gavin Barraclough 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.
Comment 17 Filip Pizlo 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.
Comment 18 Filip Pizlo 2012-11-20 16:22:57 PST 
Landed in http://trac.webkit.org/changeset/135330
Comment 19 Filip Pizlo 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.
Comment 20 Filip Pizlo 2012-11-20 17:31:12 PST 
Second part landed in http://trac.webkit.org/changeset/135336
Comment 21 Ryuan Choi 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.
Comment 22 Filip Pizlo 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.
Comment 23 Filip Pizlo 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
Comment 24 Csaba Osztrogonác 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