Closed
Bug 616744
Opened 14 years ago
Closed 13 years ago
JM: getelement pic for arguments
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
I am still working on this, trying to optimize and reduce code size. Some things are also x32 specific.
Good idea!
Assignee | ||
Comment 3•14 years ago
|
||
The liveArguments path is totally buggy, btw.
Comment 4•14 years ago
|
||
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!
Assignee | ||
Comment 5•14 years ago
|
||
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.
Updated•14 years ago
|
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
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
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)
Assignee | ||
Comment 9•13 years ago
|
||
Sorry forgot to hg add the test.
Attachment #536120 -
Attachment is obsolete: true
Attachment #536120 -
Flags: review?(dvander)
Assignee | ||
Updated•13 years ago
|
Attachment #536121 -
Flags: review?(dvander)
Assignee | ||
Updated•13 years ago
|
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+
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 12•13 years ago
|
||
Please add a commit comment and from/user line; it's really hard to push the patch otherwise...
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•13 years ago
|
||
(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]
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 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.
Description
•