Bug 90924

Summary: DFG should have fast virtual calls
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo, ggaren, mhahnenberg, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 91049    
Bug Blocks:    
Attachments:
Description Flags
work in progress
none
it made some virtual calls
none
starting to run bigger programs
none
might work
none
the patch
webkit-ews: commit-queue-
the patch
none
the patch barraclough: review+

Description Filip Pizlo 2012-07-10 17:32:05 PDT
Currently they involve C++ function calls.  Patch forthcoming.
Comment 1 Filip Pizlo 2012-07-10 17:35:25 PDT
Created attachment 151561 [details]
work in progress
Comment 2 Filip Pizlo 2012-07-10 21:22:38 PDT
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.
Comment 3 Filip Pizlo 2012-07-10 22:04:44 PDT
Created attachment 151596 [details]
starting to run bigger programs

Still occasionally crashing though.
Comment 4 Filip Pizlo 2012-07-10 22:29:08 PDT
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.
Comment 5 Filip Pizlo 2012-07-11 00:34:34 PDT
Created attachment 151621 [details]
the patch

64-bit works and produces the expected speed-up.  Working on 32-bit now.
Comment 6 Early Warning System Bot 2012-07-11 00:48:08 PDT
Comment on attachment 151621 [details]
the patch

Attachment 151621 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13166714
Comment 7 Filip Pizlo 2012-07-11 00:51:39 PDT
Created attachment 151628 [details]
the patch

Implemented 32-bit.  Should be ready for review.
Comment 8 Filip Pizlo 2012-07-11 01:05:38 PDT
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.
Comment 9 Gavin Barraclough 2012-07-11 01:15:10 PDT
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.
Comment 10 Filip Pizlo 2012-07-11 01:20:35 PDT
(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.
Comment 11 Filip Pizlo 2012-07-11 01:22:43 PDT
(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.
Comment 12 Gavin Barraclough 2012-07-11 01:38:38 PDT
(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.
Comment 13 Filip Pizlo 2012-07-11 17:12:38 PDT
Landed in http://trac.webkit.org/changeset/122392