Bug 90924 – DFG should have fast virtual calls

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

[Filip Pizlo]

Currently they involve C++ function calls.  Patch forthcoming.

[Filip Pizlo]

Created attachment 151561 [details]
work in progress

[Filip Pizlo]

Created attachment 151587 [details]
it made some virtual calls

But it's still probably broken.

Haven't written the same code for 32-bit yet.

[Filip Pizlo]

Created attachment 151596 [details]
starting to run bigger programs

Still occasionally crashing though.

[Filip Pizlo]

Created attachment 151599 [details]
might work

This appears to run SunSpider, V8, and Kraken.  Now on to real programs.

32-bit still not implemented.  Will put up on EWS anyway.

[Filip Pizlo]

Created attachment 151621 [details]
the patch

64-bit works and produces the expected speed-up.  Working on 32-bit now.

[Early Warning System Bot]

Comment on attachment 151621 [details]
the patch

Attachment 151621 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13166714

[Filip Pizlo]

Created attachment 151628 [details]
the patch

Implemented 32-bit.  Should be ready for review.

[Filip Pizlo]

Created attachment 151636 [details]
the patch

Found and fixed an issue in 32-bit register shuffling for conveying the callee to the virtual call thunk.

[Gavin Barraclough]

Comment on attachment 151636 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151636&action=review

Please revert JSFunction.h & define up the value (1<<29)-1.  Other comments optional.

> Source/JavaScriptCore/bytecode/CodeOrigin.h:51
> +        : bytecodeIndex((1 << 29) - 1)

You should really define up a constant for this value, & comment why it is this value.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1029
> +    if (calleeTagGPR == GPRInfo::nonArgGPR0) {

Do we not have a helper for this kind of shuffling? - if not, we probably really should...

> Source/JavaScriptCore/dfg/DFGThunks.cpp:109
> +    // need to remove it and drop it on the floor, since we don't care about it.

Is it possible to write a test case for this? - when would the C code throw, an unexpected OOM? - that could be tricky to test for if so... :-/

> Source/JavaScriptCore/runtime/JSFunction.h:157
> +        

PLease revert this file.

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 151636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151636&action=review
> 
> Please revert JSFunction.h & define up the value (1<<29)-1.  Other comments optional.

Will do.

> 
> > Source/JavaScriptCore/bytecode/CodeOrigin.h:51
> > +        : bytecodeIndex((1 << 29) - 1)
> 
> You should really define up a constant for this value, & comment why it is this value.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1029
> > +    if (calleeTagGPR == GPRInfo::nonArgGPR0) {
> 
> Do we not have a helper for this kind of shuffling? - if not, we probably really should...

We sort of do, setupTwoStubArgs.  But it's defined conditionally.

I'll create a helper called shuffleTwoRegisters(GPRReg source1, GPRReg source2, GPRReg target1, GPRReg target2) and have setupTwoStubArgs() call it.  I'll also remove the template argument passing, since the method is going to be inlined anyway.

> 
> > Source/JavaScriptCore/dfg/DFGThunks.cpp:109
> > +    // need to remove it and drop it on the floor, since we don't care about it.
> 
> Is it possible to write a test case for this? - when would the C code throw, an unexpected OOM? - that could be tricky to test for if so... :-/

I think we already have tests for this.  Like, try to call something that isn't callable - that'll trip it.

I'll add a test if there isn't one already.

> 
> > Source/JavaScriptCore/runtime/JSFunction.h:157
> > +        
> 
> PLease revert this file.

[Filip Pizlo]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 151636 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151636&action=review
> > 
> > Please revert JSFunction.h & define up the value (1<<29)-1.  Other comments optional.
> 
> Will do.
> 
> > 
> > > Source/JavaScriptCore/bytecode/CodeOrigin.h:51
> > > +        : bytecodeIndex((1 << 29) - 1)
> > 
> > You should really define up a constant for this value, & comment why it is this value.
> > 
> > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1029
> > > +    if (calleeTagGPR == GPRInfo::nonArgGPR0) {
> > 
> > Do we not have a helper for this kind of shuffling? - if not, we probably really should...
> 
> We sort of do, setupTwoStubArgs.  But it's defined conditionally.
> 
> I'll create a helper called shuffleTwoRegisters(GPRReg source1, GPRReg source2, GPRReg target1, GPRReg target2) and have setupTwoStubArgs() call it.  I'll also remove the template argument passing, since the method is going to be inlined anyway.

Actually - I'll save that for another patch.  I realized just after typing this that this would make the patch rather large.  I will save this for later.

> 
> > 
> > > Source/JavaScriptCore/dfg/DFGThunks.cpp:109
> > > +    // need to remove it and drop it on the floor, since we don't care about it.
> > 
> > Is it possible to write a test case for this? - when would the C code throw, an unexpected OOM? - that could be tricky to test for if so... :-/
> 
> I think we already have tests for this.  Like, try to call something that isn't callable - that'll trip it.
> 
> I'll add a test if there isn't one already.
> 
> > 
> > > Source/JavaScriptCore/runtime/JSFunction.h:157
> > > +        
> > 
> > PLease revert this file.

[Gavin Barraclough]

(In reply to comment #11)
> Actually - I'll save that for another patch.  I realized just after typing this that this would make the patch rather large.  I will save this for later.

*nod, sounds like a good plan.

[Filip Pizlo]

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