495329 - Trace JSOP_BINDNAME/JSOP_SETNAME for closures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

See bug 492918 comment 6.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Dep of blocker means blocker.  I sure hope a lot of sites on the web use closures this way, given how many hoops we're jumping through for Dromaeo.

Comment 2

[Brendan Eich [:brendan]]

Not just Dromaeo, JQuery too. Also the Go game, IIRC. Probably others. Be good to have a short list.

/be

Comment 3

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Microbenchmark

The patch gives about a 3.75x speedup on the microbenchmark. It is currently cycling on try.

Comment 5

[Gregor Wagner]

Attachment: Count where BINDNAME Object != Global Object

JSOP_BINDNAME Results for the Web (GMail, GDocs, Facebook, CNN, MSNBC, few dromaeo tests) + Browser Startup.
I tested it with "if(obj != JS_GetGlobalForObject(cx, cx->fp->scopeChain))" right before the object is pushed on the stack in the interpreter.

Overall:
bindname obj != global Object: 135735
bindname obj == global Object: 3698

Comment 6

[Brendan Eich [:brendan]]

Gregor: Can you print JS_GET_CLASS(cx, obj)->name too?

/be

Comment 7

[David Mandelin [:dmandelin]]

Attachment: Patch 2

Refreshed to apply on current tip (after native getter/setter patch).

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 390912 [details] [diff] [review]
Patch

>+/* 

Trailing whitespace waaaahmbulance has been called -- I gave 'em your credit card #.

>+ * js_SetCallArg and js_SetCallVar are extern fastcall copies of the
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

>+ * setter functions. These versions are required in order to set call vars
>+ * from traces.

Wrap before col. 80 but not so early or ragged right as above.

>+ */
>+extern JSBool JS_FASTCALL
>+js_SetCallArg(JSContext *cx, JSObject *obj, jsid id, jsval vp);
>+
>+extern JSBool JS_FASTCALL
>+js_SetCallVar(JSContext *cx, JSObject *obj, jsid id, jsval vp);

Should use v as final parameter's name, not vp.

>+        if (sprop->setter == SetCallArg) {
>+           ci = &js_SetCallArg_ci;

Underindented by one. Whoops, you put up a new patch that fixes this. Closing out this review, hope it's mostly useful still.

>+        } else if (sprop->setter == SetCallVar) {
>+            ci = &js_SetCallVar_ci;
>+        } else {
>+            ABORT_TRACE("can't trace special CallClass setter");
>+        }

No need to brace all of this if-else tree.

>+        

Trailing space, scour the patch for it.

>+        LIns* v_ins = get(&r);
>+        box_jsval(r, v_ins);
>+        LIns* args[] = {
>+            v_ins,
>+            INS_CONST(SPROP_USERID(sprop)),
>+            obj_ins,
>+            cx_ins 
>+        };
>+        LIns* call_ins = lir->insCall(ci, args);
>+        guard(true,
>+              addName(lir->ins2(LIR_eq, call_ins, INS_CONST(JS_TRUE)),

Use false for expected result and lir->ins_eq0 here, eh?

>@@ -10920,8 +10946,15 @@
>         }
>     }
> 
>-    if (obj != globalObj)
>-        ABORT_TRACE("JSOP_BINDNAME must return global object on trace");
>+    /*
>+     * If obj is a js_CallClass object, then we are tracing a reference
>+     * to an upvar in a heavyweight function. We cannot reach this point
>+     * of the trace with a different call object because of the guard
>+     * on the function call, so we can assume the result of the bindname
>+     * is constant on this trace.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Hyper-nit: Wrapping could be closer to col. 80 but at least not ragged right. Vim'll rewrap it for you for free.

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 390963 [details] [diff] [review]
Patch 2

r=me with nits picked, thanks.

/be

Comment 10

[Gregor Wagner]

(In reply to comment #6)
> Gregor: Can you print JS_GET_CLASS(cx, obj)->name too?
> 
> /be

Sure...

1323946 Call
   4531 With
      2 XULElement

Comment 11

[David Mandelin [:dmandelin]]

Pushed with nits fixed as 60a9ef4e1a3d.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/60a9ef4e1a3d

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)