JM: getelement pic for arguments

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Trunk
mozilla8
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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

7 years ago
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.
Good idea!
(Assignee)

Comment 3

7 years ago
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!
(Assignee)

Comment 5

7 years ago
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.
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

7 years ago
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.
Attachment #495297 - Attachment is obsolete: true
Attachment #495484 - Attachment is obsolete: true
Attachment #496550 - Flags: feedback?(dvander)

Updated

6 years ago
Blocks: 658844
(Assignee)

Comment 8

6 years ago
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.
Attachment #496550 - Attachment is obsolete: true
Attachment #536120 - Flags: review?(dvander)
Attachment #496550 - Flags: feedback?(dvander)
(Assignee)

Comment 9

6 years ago
Created attachment 536121 [details] [diff] [review]
v1 with the test

Sorry forgot to hg add the test.
Attachment #536120 - Attachment is obsolete: true
Attachment #536120 - Flags: review?(dvander)
(Assignee)

Updated

6 years ago
Attachment #536121 - Flags: review?(dvander)
(Assignee)

Updated

6 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

6 years ago
Created attachment 546613 [details] [diff] [review]
v1.1
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Please add a commit comment and from/user line; it's really hard to push the patch otherwise...
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

6 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]
http://hg.mozilla.org/mozilla-central/rev/da50621162f3
Status: NEW → RESOLVED
Last Resolved: 6 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.