Summary: | DFG should have fast virtual calls | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2012-07-10 17:32:05 PDT
Created attachment 151561 [details]
work in progress
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.
Created attachment 151596 [details]
starting to run bigger programs
Still occasionally crashing though.
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.
Created attachment 151621 [details]
the patch
64-bit works and produces the expected speed-up. Working on 32-bit now.
Comment on attachment 151621 [details] the patch Attachment 151621 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13166714 Created attachment 151628 [details]
the patch
Implemented 32-bit. Should be ready for review.
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 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. (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. (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. (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. Landed in http://trac.webkit.org/changeset/122392 |