Last Comment Bug 616744 - JM: getelement pic for arguments
: JM: getelement pic for arguments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla8
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks: 658844
  Show dependency treegraph
 
Reported: 2010-12-04 15:42 PST by Tom Schuster [:evilpie]
Modified: 2011-07-27 12:27 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (9.93 KB, patch)
2010-12-04 15:46 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v1 (6.36 KB, patch)
2010-12-06 04:17 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review
WIP 2 (6.84 KB, patch)
2010-12-09 11:35 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v1 (8.98 KB, patch)
2011-05-30 10:20 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v1 with the test (9.70 KB, patch)
2011-05-30 10:24 PDT, Tom Schuster [:evilpie]
dvander: review+
Details | Diff | Review
v1.1 (9.12 KB, patch)
2011-07-18 13:13 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review

Description Tom Schuster [:evilpie] 2010-12-04 15:42:27 PST
After my first contact with the PIC, i got the idea to do something harder. So i eventually figured, we would always take a slow path for arguments objects. Believe me i wasn't disappointed with the amount of thinking, i had to built this.
Happily the jit brought massive speedup.
Comment 1 Tom Schuster [:evilpie] 2010-12-04 15:46:18 PST
Created attachment 495297 [details] [diff] [review]
WIP

I am still working on this, trying to optimize and reduce code size. Some things are also x32 specific.
Comment 2 David Anderson [:dvander] 2010-12-04 15:58:13 PST
Good idea!
Comment 3 Tom Schuster [:evilpie] 2010-12-04 16:02:29 PST
The liveArguments path is totally buggy, btw.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-04 18:19:38 PST
Once you're done with this, I'd love to see what sort of numbers you get on http://dromaeo.com/?prototype before and after the patch!
Comment 5 Tom Schuster [:evilpie] 2010-12-06 04:17:47 PST
Created attachment 495484 [details] [diff] [review]
v1

So I got this working with arguments with live arguments (no pointer to FrameEntry), and arguments < the number of formal arguments of the function. So the only thing left are the overflow arguments. I think i would need one more register, feedback appreciated.
Comment 6 David Anderson [:dvander] 2010-12-06 10:21:01 PST
This approach seems good - normally when there is lots of internal control flow, I'd prefer making separate ICs (one for each major case), but here the cases are pretty small and it seems similar to what we do for call objects. But just to reduce the function length you may want to separate it into separate functions.

Grabbing extra registers in ICs is tricky. I think you can get away with two if you use the stack to save before you clobber. If not, you could take a snapshot of the register state in the compiler, and save that in the IC. That way you know what's available and what might need to be saved (typed arrays do this, bug 594247).
Comment 7 Tom Schuster [:evilpie] 2010-12-09 11:35:52 PST
Created attachment 496550 [details] [diff] [review]
WIP 2

So got the basic thing working. Just need to convert some statics to offset use, and possibly some comments? I used push/pop to solve my register problem.
+Feedback and i will finish this.
Comment 8 Tom Schuster [:evilpie] 2011-05-30 10:20:38 PDT
Created attachment 536120 [details] [diff] [review]
v1

So this basically the same as WIP 2. I only made it non platform depend and added one test.
Comment 9 Tom Schuster [:evilpie] 2011-05-30 10:24:34 PDT
Created attachment 536121 [details] [diff] [review]
v1 with the test

Sorry forgot to hg add the test.
Comment 10 David Anderson [:dvander] 2011-07-06 14:48:45 PDT
Comment on attachment 536121 [details] [diff] [review]
v1 with the test

Review of attachment 536121 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=me with one comment:

::: js/src/methodjit/PolyIC.cpp
@@ +2358,5 @@
> +
> +    Address privateData(typeReg, offsetof(JSObject, privateData));
> +    Jump liveArguments = masm.branchPtr(Assembler::NotEqual, privateData, ImmPtr(0));
> +   
> +    masm.loadPayload(Address(typeReg, JSObject::getFixedSlotOffset(ArgumentsObject::DATA_SLOT)), objReg);

This must be loadPrivate()
Comment 11 Tom Schuster [:evilpie] 2011-07-18 13:13:45 PDT
Created attachment 546613 [details] [diff] [review]
v1.1
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-26 07:03:11 PDT
Please add a commit comment and from/user line; it's really hard to push the patch otherwise...
Comment 13 Tom Schuster [:evilpie] 2011-07-27 06:54:51 PDT
(In reply to comment #12)
> Please add a commit comment and from/user line; it's really hard to push the
> patch otherwise...
Sorry, i didn't think of that as i am used to commit patches by myself. Because i have the rights now i just did it :)
http://hg.mozilla.org/integration/mozilla-inbound/rev/da50621162f3
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-27 12:27:46 PDT
http://hg.mozilla.org/mozilla-central/rev/da50621162f3

Note You need to log in before you can comment on or make changes to this bug.