Closed Bug 922861 Opened 7 years ago Closed 7 years ago

IonMonkey: Add GetElemIC stubs for getter calls

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: djvj, Assigned: efaust)

References

Details

Attachments

(1 file, 2 obsolete files)

Dromaeo DOM Traversal (Prototype) hits this fallback case about 60k times.
Attached patch Untested v1 (obsolete) — Splinter Review
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Just tested the patch (with minor fix to actually enable the getter stubs).

Results (with patch on left, without patch on right):
http://dromaeo.com/?id=205989,205990

Improvements across the board, with major 40-50% improvements in DOM Traversal.  Yay.
So this patch applies on top of those for bug 922493, so I'm making that a blocker for this one.

Once we close this off, we're looking pretty spiffy on Dromaeo.  Thanks for quick turnaround on this.  It's nice to get to a definitive resolution for the last of the Dromaeo regressions before Summit.
Depends on: 922493
Polished for review.
Attachment #812855 - Attachment is obsolete: true
Attachment #813240 - Flags: review?(kvijayan)
So polished it doesn't build. Oops. Fixed.
Attachment #813240 - Attachment is obsolete: true
Attachment #813240 - Flags: review?(kvijayan)
Attachment #813310 - Flags: review?(kvijayan)
Comment on attachment 813310 [details] [diff] [review]
Cache getter calls in GetElementIC

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

See comment, otherwise looks ok.

::: js/src/jit/IonCaches.cpp
@@ +1017,5 @@
>      // Rejoin jump.
>      attacher.jumpRejoin(masm);
>  
>      // Jump to next stub.
> +    masm.bind(failures);

Something feels off about |failures| potentially being passed in from the outside, but bound in the callee.

It seems like it might be cleaner to move the label binding and |jumpNextStub| out of |GenerateCallGetter| and into its callers.
Attachment #813310 - Flags: review?(kvijayan) → review+
(In reply to Kannan Vijayan [:djvj] from comment #6)
> Comment on attachment 813310 [details] [diff] [review]
> Cache getter calls in GetElementIC
> 
> Review of attachment 813310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comment, otherwise looks ok.
> 
> ::: js/src/jit/IonCaches.cpp
> @@ +1017,5 @@
> >      // Rejoin jump.
> >      attacher.jumpRejoin(masm);
> >  
> >      // Jump to next stub.
> > +    masm.bind(failures);
> 
> Something feels off about |failures| potentially being passed in from the
> outside, but bound in the callee.
> 
> It seems like it might be cleaner to move the label binding and
> |jumpNextStub| out of |GenerateCallGetter| and into its callers.

Recall that all but one caller don't have an external label, since they pass NULL by default for the |failures| parameter. This idiom happens elsewhere in the CodeGenerator, and indeed, even in GenerateReadSlot() in the ICs, so I don't feel too strongly against using it again. Perhaps a JS_ASSERT(!failures.bound()) or whatever should be added?
https://hg.mozilla.org/integration/mozilla-inbound/rev/a195c8d3b537

Landed. If we feel really bad about the feedback in comment 7, we can back it out.
https://hg.mozilla.org/mozilla-central/rev/a195c8d3b537
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Either this bug or bug 922493 was a nice 4-5% win on Dromaeo-CSS.  ;)
You need to log in before you can comment on or make changes to this bug.