Bug 102662 – DFG should be able to cache closure calls

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 174916 [details]
work in progress

[Filip Pizlo]

Created attachment 175107 [details]
the patch

[WebKit Review Bot]

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 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 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 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 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]

(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]

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]

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 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 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]

Created attachment 175135 [details]
the patch

[Build Bot]

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]

Created attachment 175153 [details]
the patch

Fix windows, maybe.

[Gavin Barraclough]

Comment on attachment 175153 [details]
the patch

Looks fine, I'd like to see the assembler changes split out though.

[Filip Pizlo]

(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]

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

[Filip Pizlo]

Oops.  Didn't mean to close the bug.  http://trac.webkit.org/changeset/135330 is just the first part of this.

[Filip Pizlo]

Second part landed in http://trac.webkit.org/changeset/135336

[Ryuan Choi]

(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]

(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]

(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]

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