532477 - On trace, replace js_SetCallArg/js_SetCallVar with specialized code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Similar to bug 530225, but for the set case.  This situation is simpler, since we were already basically calling the sprop setter on trace directly; this is just unwinding the onion-layers of that setter and leaving just the guts.  This applies on top of bug 530225.

Again, this only works if I can assume all the relevant slots already exist, etc; if not then we need to go through JS_SetReservedSlot.  I'd really like to avoid that if I can (esp. the locking).

And again, we could poke dslots directly here, I think.  Would that be ok?

This speeds up the slice test on dromaeo from 170 runs/s (after bug 530225) to
200 runs/s or so.  The overall js string tests go from 280 runs/s to 460 runs/s with this patch and the one in bug 530225.  That's almost within spitting distance of Safari (500 runs/s) and faster than Chrome (405 runs/s).

Comment 1

[David Mandelin [:dmandelin]]

This looks good, aside from the adj_slot issue I mentioned in the other bug.

This is really good work. I'm pleased (and impressed) that it yields such a big speedup.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, I meant bug 530255 throughout.

Comment 3

[Andreas Gal :gal]

I would like to randomly express my dismay with the fact that "string" tests in Dromaeo are gated by closure access performance. We need a decent benchmark suite.

Comment 4

[Brendan Eich [:brendan]]

Boris, hope you don't mind taking ownership since you are patching. If you want to hand off patches, just call for a volunteer new assignee. Great to see your work here, you are inducted into the TraceMonkey JIT-grooming troop!

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 415692 [details] [diff] [review]
Proposed patch

>+template<typename T>
>+inline JSBool
>+SetInClosure(JSContext* cx, JSObject* call, uint32 slot, jsval v)
>+{
>+    JS_ASSERT(OBJ_GET_CLASS(cx, call) == &js_CallClass);
>+    JS_ASSERT(slot < T::maxObjSlots(call));
>+
>+    /*
>+     * Do we also want to assume that this will always live in dslots
>+     * (which is the case for Call objects, in fact), or not be quite
>+     * so brazen?

This code is a brazen hussy, smooching jsfun.cpp and its manly static assertions that uphold the dslots guarantee! Please change this comment to reference the static asserts over there, and don't be coy about it! ;-)

>+    JS_ASSERT(sprop->flags & SPROP_HAS_SHORTID);
>     LIns* args[] = {
>         box_jsval(v, v_ins),
>-        INS_CONST(SPROP_USERID(sprop)),
>+        INS_CONST(uint32(sprop->shortid)),

This is tricky -- you need a uint16 cast, if I'm not mistaken, not uint32.

r=me with these changes and dmandelin's blessing,

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> This is tricky -- you need a uint16 cast, if I'm not mistaken, not uint32.

Could be, but that code change is gone anyway with LIR-ification.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Better, stronger, faster

This passes dmandelin's upvar tests (at least as much so as m-c does, at least), and gets us up to 650 runs/s or so on the dromaeo-object-string tests.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, I ran full dromaeo (including the DOM tests, etc, not what runs on our tbox).  A lot of that stuff doesn't hit the upvar code, so the speedup across the board was only about 6%.  Still, better than nothing.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 415807 [details] [diff] [review]
Better, stronger, faster

>+    if (!callobj->getPrivate()) {
>+        // Due to the parent guard in guardCallee, we know that on trace we
>+        // will also not have a non-null private in the Call object.  So all we

Nit: French spacing frowned upon in local style.

>+        // need to do is write the value to the Call object's slots.

Nit: s/slots/slot/

This comment could be more explicit: "Because of the parent guard in guardCallee ensures this Call object is the same one on trace, and because once a Call object loses its frame it never regains one, ...."

>+        JS_ASSERT(sprop->flags & SPROP_HAS_SHORTID);
>+        int32 offset = sprop->shortid;
>+        if (sprop->setter == SetCallArg)
>+            offset += ArgClosureTraits::slot_offset(callobj);
>+        else if (sprop->setter == SetCallVar)
>+            offset += VarClosureTraits::slot_offset(callobj);
>+        else
>+            RETURN_STOP("can't trace special CallClass setter");
>+        LIns* base =
>+            lir->insLoad(LIR_ldp, callobj_ins, offsetof(JSObject, dslots));

Nit: no need to break this across two lines. Might be easier to read with a blank line before, though.

>+        lir->insStorei(box_jsval(v, v_ins), base, offset*sizeof(jsval));

Nit: space around binary ops.

/be

Comment 10

[Brendan Eich [:brendan]]

Er, my wording is slightly off ("Because of the blah ensures blargh"), but you get the idea ;-).

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed the nits locally.

Comment 12

[Brendan Eich [:brendan]]

The intarwebs misled me on "French spacing" meaning two spaces after full stop, can you believe it? The wikipedia page adverts to the crazy inversion of the original meaning. I'll try to change. Vive la France!

/be

Comment 13

[David Mandelin [:dmandelin]]

Comment on attachment 415807 [details] [diff] [review]
Better, stronger, faster

6% speedup across the board from a 15-line patch is a big win in my book.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Well, it needs bug 530255 as well for the 6% thing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/d292473f8ebe

Comment 17

[Boris Zbarsky [:bzbarsky]]

And pushed http://hg.mozilla.org/tracemonkey/rev/99670bf7d9ac to fix build bustage.

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d292473f8ebe