Closed Bug 616744 Opened 9 years ago Closed 8 years ago

JM: getelement pic for arguments

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
I am still working on this, trying to optimize and reduce code size. Some things are also x32 specific.
The liveArguments path is totally buggy, btw.
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!
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #495484 - Attachment is patch: true
Attachment #495484 - Attachment mime type: application/octet-stream → text/plain
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).
OS: Windows XP → All
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #495297 - Attachment is obsolete: true
Attachment #495484 - Attachment is obsolete: true
Attachment #496550 - Flags: feedback?(dvander)
Blocks: 658844
Attached patch v1 (obsolete) — Splinter Review
So this basically the same as WIP 2. I only made it non platform depend and added one test.
Attachment #496550 - Attachment is obsolete: true
Attachment #536120 - Flags: review?(dvander)
Attachment #496550 - Flags: feedback?(dvander)
Attached patch v1 with the testSplinter Review
Sorry forgot to hg add the test.
Attachment #536120 - Attachment is obsolete: true
Attachment #536120 - Flags: review?(dvander)
Attachment #536121 - Flags: review?(dvander)
Assignee: general → evilpies
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()
Attachment #536121 - Flags: review?(dvander) → review+
Attached patch v1.1Splinter Review
Keywords: checkin-needed
Please add a commit comment and from/user line; it's really hard to push the patch otherwise...
Keywords: checkin-needed
(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
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/da50621162f3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.