Bug 90924 - DFG should have fast virtual calls
Summary: DFG should have fast virtual 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: 91049
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-10 17:32 PDT by Filip Pizlo
Modified: 2012-07-11 22:39 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (23.97 KB, patch)
2012-07-10 17:35 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it made some virtual calls (38.39 KB, patch)
2012-07-10 21:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to run bigger programs (39.69 KB, patch)
2012-07-10 22:04 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
might work (39.25 KB, patch)
2012-07-10 22:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (39.93 KB, patch)
2012-07-11 00:34 PDT, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (45.49 KB, patch)
2012-07-11 00:51 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (45.83 KB, patch)
2012-07-11 01:05 PDT, Filip Pizlo
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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