Closed Bug 795803 Opened 7 years ago Closed 7 years ago

IonMonkey: Enable ICing of JSNative and PropertyOp getters

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: djvj, Unassigned)

References

Details

(Whiteboard: [ion:p1:fx18])

Branching this out from from bug 786126 to make it easier to track.  Enable the generation of IC stubs for calling of PropertyOp getters.
Blocks: 786126
Bug found: bad ordering of checks in TryAttachNativeStub - some checks on the resulting holder object occurred before the null check on the result of lookupProperty (indicating property not found).

Fixed, and new try build is:

https://tbpl.mozilla.org/?tree=Try&rev=e4bb51cbb3e0

The orange in M(1) is a crash inside GetPcScript, when it tries to walk up the stack frame to get script info and fails.  This is because in |buildOOLFakeExitFrame| we use the masm's |framePushed()| value to create the exit frame descriptor.  This is supposed to describe the frame size.  However, the masm instance used in the OOL stub generator is completely disconnected from the masm instance used to generate the mainline code, which means their |framePushed()| won't be synced.

Need to thread the framePushed() into the IC during Ion code generation, and use that to initialize the OOL masm's framePushed().
After fixing the frameSize issue in OOL exit frames, this was the try result:

https://tbpl.mozilla.org/?tree=Try&rev=2815b2d60dc1

The error in M(3) was that I was passing the PropertyName directly into the getter without checking for a tinyId/shortId in those cases where those are necessary.

Found and fixed that today, new try is at:

https://tbpl.mozilla.org/?tree=Try&rev=d57427546d22
Fixed up the 32-bit build issues, rebased to tip, minor changes to ensure that the exit frame from stub code contained a pointer to the stub IonCode so it would be properly rooted, and merged the jsnative and property-op patches into one patch.  New try:

https://tbpl.mozilla.org/?tree=Try&rev=640ff09a3a11
There was a silly assertion issue introduced in those fixes above which would have led to a lot of try failures.  The |pushWithPatch| doesn't update |framePushed| on the macro assembler (as opposed to all the calls to |Push|), which was being checked for consistency with an assert in the code.  Changed code to manually adjust |framePushed| For the pushWithPatch.

New try is at:
https://tbpl.mozilla.org/?tree=Try&rev=32d7866c00f2
Changed title to reflect the fact that the patch supports both JSNative and PropertyOp getters.
Summary: IonMonkey: Enable ICing of PropertyOp getters → IonMonkey: Enable ICing of JSNative and PropertyOp getters
The oranges in the previous try are from the tip I was working off of, and relate to memory leaks that are unrelated to my patch (checked on tbpl).

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3d21b84a63

See bug 786126 for reviews.  Jan reviewed two separate patches which I've consolidated into one for the push since they sit right on top of each other in the code.
https://hg.mozilla.org/mozilla-central/rev/fa3d21b84a63
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 798383
Depends on: 798589
Duplicate of this bug: 785465
Depends on: 848088
You need to log in before you can comment on or make changes to this bug.