512029 - Remove JSStackFrame::callee

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: proposed changes

Taking bug 471425 one step at a time.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: XOW change

dvander asked me to do this -- the hack is not necessary and the comment about interested onlookers never really panned out (we still skip native frames when computing subject principals).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Oops, the first hunk won't apply and doesn't belong in that patch.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: combined patch

Thanks, Blake! Combined patch seems to not throw MochiTests into chaos now.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 396017 [details] [diff] [review]
combined patch

>+++ b/js/src/jsexn.cpp	Fri Aug 21 18:31:40 2009 -0700
>@@ -281,7 +281,7 @@
>         if (fp->fun && fp->argv) {
>             v = JSVAL_NULL;
>             if (checkAccess &&
>-                !checkAccess(cx, fp->callee, callerid, JSACC_READ, &v)) {
>+                !checkAccess(cx, fp->callee(), callerid, JSACC_READ, &v)) {

Here is an example of where the compiler can inline JSStackFrame::callee and see that fp->argv is already non-null, so optimize the expansion of fp->callee() to just fp->argv[-2] with jsval untagging. But it might be better in performance-critical code to do that expansion and optimization by hand, and there are places in jsops.cpp (below) where the compiler can't so optimize, but we should.

In general the code can benefit from asserting along with optimizing -- not in this case, predicated on an fp->argv non-null test, but elsewhere. We don't assert non-null, though, so JS_ASSERT(fp->argv) might be seen as unnecessary right before an fp->argv[-2], but it could be helpful to the reader.

>                 break;
>             }
>             valueCount += fp->argc;
>diff -r 3fe4a43ac9e1 js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp	Fri Aug 21 13:51:55 2009 -0700
>+++ b/js/src/jsfun.cpp	Fri Aug 21 18:31:40 2009 -0700
>@@ -281,7 +281,7 @@
>     while ((parent = OBJ_GET_PARENT(cx, global)) != NULL)
>         global = parent;
> 
>-    argsobj = NewArguments(cx, global, fp->argc, fp->callee);
>+    argsobj = NewArguments(cx, global, fp->argc, fp->callee());

This is a case where we should assert fp->argv and use that fact to simplify.

>     if (!argsobj)
>         return argsobj;
> 
>@@ -809,7 +809,7 @@
>         /* Root env before js_DefineNativeProperty (-> JSClass.addProperty). */
>         fp->scopeChain = env;
>         if (!js_DefineNativeProperty(cx, fp->scopeChain, ATOM_TO_JSID(lambdaName),
>-                                     OBJECT_TO_JSVAL(fp->callee),
>+                                     OBJECT_TO_JSVAL(fp->callee()),

Just fp->argv[-2], no need to assert here.

>                                      CalleeGetter, NULL,
>                                      JSPROP_PERMANENT | JSPROP_READONLY,
>                                      0, 0, NULL)) {
>@@ -824,8 +824,8 @@
>     }
> 
>     callobj->setPrivate(fp);
>-    JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, fp->callee));
>-    STOBJ_SET_SLOT(callobj, JSSLOT_CALLEE, OBJECT_TO_JSVAL(fp->callee));
>+    JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, fp->callee()));
>+    STOBJ_SET_SLOT(callobj, JSSLOT_CALLEE, OBJECT_TO_JSVAL(fp->callee()));

fp->argv[-2], with untagging for the GET_FUNCTION_PRIVATE of course.

>@@ -1190,7 +1190,7 @@
>         JSStackFrame *fp = (JSStackFrame *) obj->getPrivate();
>         if (fp) {
>             JS_ASSERT(fp->fun);
>-            *vp = OBJECT_TO_JSVAL(fp->callee);
>+            *vp = OBJECT_TO_JSVAL(fp->callee());

fp->argv[-2].

>@@ -1323,7 +1323,7 @@
>                 return JS_TRUE;
>             }
> 
>-            *vp = OBJECT_TO_JSVAL(fp->down->callee);
>+            *vp = OBJECT_TO_JSVAL(fp->down->callee());

fp->down->argv[-2].

>diff -r 3fe4a43ac9e1 js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Fri Aug 21 13:51:55 2009 -0700
>+++ b/js/src/jsgc.cpp	Fri Aug 21 18:31:40 2009 -0700
>@@ -2871,8 +2871,8 @@
>               (fp->fun && JSFUN_THISP_FLAGS(fp->fun->flags)));
>     JS_CALL_VALUE_TRACER(trc, (jsval)fp->thisp, "this");
> 
>-    if (fp->callee)
>-        JS_CALL_OBJECT_TRACER(trc, fp->callee, "callee");
>+    if (fp->callee())
>+        JS_CALL_OBJECT_TRACER(trc, fp->callee(), "callee");

I'd test fp->argv and JS_CALL_VALUE_TRACER(trc, fp->argv[-2], "callee"); and then...

> 
>     if (fp->argv) {
>         nslots = fp->argc;

... combine the fp->argv tests.

>@@ -145,6 +144,10 @@
>             js_PutArgsObject(cx, this);
>         }
>     }
>+
>+    inline JSObject *callee() {
>+        return argv ? JSVAL_TO_OBJECT(argv[-2]) : NULL;
>+    }

No explicit inline qualifier on inlined class methods.

>@@ -6122,8 +6122,8 @@
> 
>     for (; fp; fp = fp->down) {
>         fprintf(stderr, "JSStackFrame at %p\n", (void *) fp);
>-        if (fp->callee)
>-            dumpValue(OBJECT_TO_JSVAL(fp->callee));
>+        if (fp->callee())
>+            dumpValue(OBJECT_TO_JSVAL(fp->callee()));

fp->argv and fp->argv[-2].

In jsops.cpp:

>@@ -2374,7 +2373,7 @@
>                  * contains JS_SCRIPT_REGEXPS(script)->length reserved slots
>                  * for the cloned regexps; see fun_reserveSlots, jsfun.c.
>                  */
>-                funobj = fp->callee;
>+                funobj = fp->callee();

Performance matters, and the predicate here is if (fp->fun), which implies fp->argv is non-null but the compiler can't know that. So simplify to funobj = JSVAL_TO_OBJECT(fp->argv[-2]).

>@@ -2743,7 +2742,7 @@
> 
>           BEGIN_CASE(JSOP_GETDSLOT)
>           BEGIN_CASE(JSOP_CALLDSLOT)
>-            obj = fp->callee;
>+            obj = fp->callee();

Ditto.

>@@ -3187,7 +3186,7 @@
>           END_CASE(JSOP_LAMBDA_DBGFC)
> 
>           BEGIN_CASE(JSOP_CALLEE)
>-            PUSH_OPND(OBJECT_TO_JSVAL(fp->callee));
>+            PUSH_OPND(OBJECT_TO_JSVAL(fp->callee()));

Ditto, of course just fp->argv[-2] here, no JSVAL macro untagging and re-tagging required.

In jstracer.cpp:

>@@ -1527,10 +1527,10 @@
>          */
>         unsigned operands = fp->regs->sp - StackBase(fp);
>         slots += operands;
>-        if (fp->callee)
>+        if (fp->callee())
>             slots += fp->script->nfixed + 1 /*argsobj*/;
>         if (depth-- == 0) {
>-            if (fp->callee)
>+            if (fp->callee())

These should test fp->argv.

>@@ -2643,7 +2643,7 @@
>         }
>         for (; n != 0; fp = fp->down) {
>             --n;
>-            if (fp->callee) {
>+            if (fp->callee()) {

Ditto.

>@@ -2666,7 +2665,7 @@
>                  * scope object here.
>                  */
>                 if (!fp->scopeChain) {
>-                    fp->scopeChain = OBJ_GET_PARENT(cx, fp->callee);
>+                    fp->scopeChain = OBJ_GET_PARENT(cx, fp->callee());

Now we are in the shadow of if (fp->argv) (with the last "Ditto." above applied) so this should be optimized to OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(fp->argv[-2])).

>@@ -3392,7 +3391,7 @@
>     exit->calldepth = callDepth;
>     exit->numGlobalSlots = ngslots;
>     exit->numStackSlots = stackSlots;
>-    exit->numStackSlotsBelowCurrentFrame = cx->fp->callee
>+    exit->numStackSlotsBelowCurrentFrame = cx->fp->callee()
>         ? nativeStackOffset(&cx->fp->argv[-2])/sizeof(double)
>         : 0;

Use cx->fp->argv for the ?: condition.

Pre-existing nits: space on both sides of / and indent ? and : to underhang condition properly.

>@@ -4757,13 +4755,12 @@
>     fp->callobj = NULL;
>     fp->argsobj = NULL;
>     fp->varobj = cx->fp->varobj;
>-    fp->callee = exit->nativeCallee();
>     fp->script = NULL;
>-    fp->fun = GET_FUNCTION_PRIVATE(cx, fp->callee);
>     // fp->thisp is really a jsval, so reinterpret_cast here, not JSVAL_TO_OBJECT.
>     fp->thisp = (JSObject *) cx->nativeVp[1];
>     fp->argc = cx->nativeVpLen - 2;
>     fp->argv = cx->nativeVp + 2;
>+    fp->fun = GET_FUNCTION_PRIVATE(cx, fp->callee());

Use fp->argv[-2], compiler probably can't optimize for you based on previous statement's effects.

>@@ -5897,7 +5894,7 @@
> #ifdef DEBUG
>     /* Verify that our state restoration worked. */
>     for (JSStackFrame* fp = cx->fp; fp; fp = fp->down) {
>-        JS_ASSERT_IF(fp->callee, JSVAL_IS_OBJECT(fp->argv[-1]));
>+        JS_ASSERT_IF(fp->callee(), JSVAL_IS_OBJECT(fp->argv[-1]));

JS_ASSERT_IF(fp->argv, ...).

>@@ -8282,7 +8279,7 @@
>      * js_ComputeThisForFrame updates cx->fp->argv[-1], so sample it into 'original' first.
>      */
>     jsval original = JSVAL_NULL;
>-    if (cx->fp->callee) {
>+    if (cx->fp->callee()) {

!!    if (cx->fp->argv) {

>@@ -8295,7 +8292,7 @@
>         ABORT_TRACE_ERROR("js_ComputeThisForName failed");
> 
>     /* In global code, bake in the global object as 'this' object. */
>-    if (!cx->fp->callee) {
>+    if (!cx->fp->callee()) {

!!    if (!cx->fp->argv) {

>@@ -8475,7 +8472,7 @@
>      * This doesn't do layout arithmetic, but it must clear out all the slots defined as
>      * imported by VisitFrameSlots.
>      */
>-    if (fp->callee) {
>+    if (fp->callee()) {

Ditto.

>@@ -10567,7 +10564,7 @@
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_GETDSLOT()
> {
>-    JSObject* callee = cx->fp->callee;
>+    JSObject* callee = cx->fp->callee();
>     LIns* callee_ins = get(&cx->fp->argv[-2]);

The get call's argument gives away the over-general and yet unoptimizable callee() on the line being changed. Instead match jsops.cpp and initialize callee = JSVAL_TO_OBJECT(cx->fp->argv[-2]).

>@@ -11603,7 +11600,7 @@
> 
>         // In non-heavyweight functions, we can safely skip the call
>         // object, if any.
>-        obj = OBJ_GET_PARENT(cx, fp->callee);
>+        obj = OBJ_GET_PARENT(cx, fp->callee());

Predicate here is (fp->fun), which implies fp->argv. Hand-optimize again, the compiler can't do it.

Don't you wish we had a typestate language with local inference and compiler-understood assertions?

/be

Comment 5

[Brendan Eich [:brendan]]

Could have more JSStackFrame inline helpers, e.g. an unconditioned by argv test calleeObject(), which returns JSVAL_TO_OBJECT(argv[-2]) and could JS_ASSERT(argv) first to be clear.

/be

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

fixes callee() cases where argv cannot be NULL

I tried to avoid adding more inline functions since it seems like I'd get N variants (one for each of jsval/JSObject with and without argv assert). Though if people think "fp->calleeValue()" and "fp->calleeObject()" would be more readable I won't object.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> if people think "fp->calleeValue()" and "fp->calleeObject()" would be more
> readable I won't object.

That sounds like a fine idea to me that would enhance readability.  Suppose you're new to SpiderMonkey and reading some fragment of code -- how would fp->argv[-2] make any obvious sense?  It's certainly less obvious than fp->calleeValue().

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 396261 [details] [diff] [review]
v2

Please file a followup bug for calleeValue and calleeObject -- that'll catch all argv[-2] uses, beyond scope of this patch. Thanks,

/be

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/7b7f9ae673cf

follow-up bug+patch forthcoming

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7b7f9ae673cf

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3e02f9467ac1

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2d25b9ba244