404935 - Simplifying js_CompileScript/js_CompileFunctionBody invocations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently in all cases js_CompileScript and js_CompileFunctionBody are used like in the following patterns:

    if (!js_InitParseContext(cx, &pc, NULL, 0, file, filename, 1))
        return NULL;
    js_InitCompilePrincipals(cx, &pc, principals);
    script = js_CompileScript(cx, obj, &pc);
    js_FinishParseContext(cx, &pc);

and

    ok = js_InitParseContext(cx, &pc, chars, length, NULL, filename, lineno);
    if (ok) {
        js_InitCompilePrincipals(cx, &pc, principals);
        ok = js_CompileFunctionBody(cx, &pc, fun);
        js_FinishParseContext(cx, &pc);
    }

For simpler code it would be nice to move the initialization/finishing of the parse context into js_CompileScript and js_CompileFunctionBody themselves.

Comment 1

[Igor Bukanov]

Attachment: v1

Beyond moving js_InitParseContext and js_FinishParseContext into js_CompileScript/js_CompileFunctionBody the patch replaces JSFRAME_COMPILE_N_GO by a tree context flag and moves initialization of TCF_IN_FUNCTION right after creating the tree context for simpler code.

AFAICS it is also possible to remove JSFRAME_SCRIPT_OBJECT as it is set by the script object compiler and is used with comments:


    /*
     * A Script object can be used to split an eval into a compile step done
     * at construction time, and an execute step done separately, possibly in
     * a different scope altogether.  We therefore cannot do any name-to-slot
     * optimizations, but must lookup names at runtime.  Note that script_exec
     * ensures that its caller's frame has a Call object, so arg and var name
     * lookups will succeed.
     */
    fp = cx->fp;
    if (fp->flags & JSFRAME_SCRIPT_OBJECT)
        return JS_TRUE;

But the comments are not true since the global variable optimization is applied to the single script execution so it is perfectly applicable for Script instances as well.

Comment 2

[Igor Bukanov]

The patch shrinks the size of an optimized js shell executable by 1K down to 491084.

Comment 3

[Igor Bukanov]

Attachment: v2

The new version consistently uses tc->flags & TCF_IN_FUNCTION to check for function compilation replacing various checks like class_of_varobj == &js_FunctionClass or fp->fun && !(fp->flags & JSFARAME_SPECIAL).

Comment 4

[Igor Bukanov]

Recording patch dependancy

Comment 5

[Igor Bukanov]

This cleanup patch simplifies a fix for 398219.

Comment 6

[Igor Bukanov]

Attachment: v3

The new version stores JSFunction of the compiled function in JSTreeContext and get away from setting up a special compiler frame for functions bringing more simplifications to the code.

Comment 7

[Igor Bukanov]

Attachment: v4

This is v3 plus couple of cleanups.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 289902 [details] [diff] [review]
v4

>Index: js/src/jsapi.c
>===================================================================

>-    if (!js_InitParseContext(cx, &pc, chars, length, NULL, filename, lineno))
>-        return NULL;
>-    js_InitCompilePrincipals(cx, &pc, principals);
>-    script = js_CompileScript(cx, obj, &pc);
>-    js_FinishParseContext(cx, &pc);
>+    tcflags = JS_HAS_COMPILE_N_GO_OPTION(cx) ? TCF_COMPILE_N_GO : 0;
>+    script = js_CompileScript(cx, obj, principals, tcflags,
>+                              chars, length, NULL, filename, 1);

Should that 1 be lineno?

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> >+    script = js_CompileScript(cx, obj, principals, tcflags,
> >+                              chars, length, NULL, filename, 1);
> 
> Should that 1 be lineno?

Thanks for spotting this! I will update the patch in a moment.

Comment 10

[Igor Bukanov]

Attachment: v5

This is v4 + lineno fix.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 289959 [details] [diff] [review]
v5

Oh, I've been wanting to get rid of stupid compiler-fake frames forever! Thanks for doing this. Comments:

> JS_PUBLIC_API(JSBool)
> JS_EvaluateUCScriptForPrincipals(JSContext *cx, JSObject *obj,
>                                  JSPrincipals *principals,
>                                  const jschar *chars, uintN length,
>                                  const char *filename, uintN lineno,
>                                  jsval *rval)
> {
>-    uint32 options;
>     JSScript *script;
>     JSBool ok;
> 
>     CHECK_REQUEST(cx);
>-    options = cx->options;
>-    cx->options = options | JSOPTION_COMPILE_N_GO;
>-    script = JS_CompileUCScriptForPrincipals(cx, obj, principals, chars, length,
>-                                             filename, lineno);
>-    cx->options = options;
>+    script = js_CompileScript(cx, obj, principals, TCF_COMPILE_N_GO,
>+                              chars, length, NULL, filename, lineno);

Subtle difference: the JSOPTION_COMPILE_N_GO could be picked up by any activation of js_EmitFunctionBody, js_CompileScript, or js_CompileFunctionBody (thence propagated back into the emitter via JSFRAME_COMPILE_N_GO). Great to get rid of the redundant flags, but are you sure you're not deoptimizing Narcissus? It counts on top-level consts from one .js file being copy-propagated to switch case labels in functions whose definitions are evaluated after the first .js file.

>@@ -1644,46 +1637,49 @@ js_LookupCompileTimeConstant(JSContext *
> 
>             /*
>              * Try looking in the variable object for a direct property that
>              * is readonly and permanent.  We know such a property can't be
>              * shadowed by another property on obj's prototype chain, or a
>              * with object or catch variable; nor can prop's value be changed,
>              * nor can prop be deleted.
>              */
>-            if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
>-                JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
>-                if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
>+            if (cg->treeContext.flags & TCF_IN_FUNCTION) {
>+                if (js_LookupLocal(cx, cg->treeContext.fun, atom, NULL) !=
>+                    JSLOCAL_NONE) {
>                     break;
>-            }
>-
>-            ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj, &prop);
>-            if (ok) {
>-                if (pobj == obj &&
>-                    (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {
>+                }
>+            } else if (cg->treeContext.flags & TCF_COMPILE_N_GO) {

This part looks like an incompatible change that will indeed deoptimize Narcissus's tableswitches to condswitches.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Subtle difference: the JSOPTION_COMPILE_N_GO could be picked up by any
> activation of js_EmitFunctionBody, js_CompileScript, or js_CompileFunctionBody
> (thence propagated back into the emitter via JSFRAME_COMPILE_N_GO).

In the current code JSOPTION_COMPILE_N_GO is checked in 2 places. The first case is js_LookupCompileTimeConstant:

JSBool
js_LookupCompileTimeConstant(JSContext *cx, JSCodeGenerator *cg, JSAtom *atom,
                             jsval *vp)
{
...
    fp = cx->fp;
    do {
... 
        obj = fp->varobj;
        if (obj == fp->scopeChain) {
...

            if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
                JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
                if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
                    break;
            }

            ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj, &prop);
            if (ok) {
                if (pobj == obj &&
                    (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {
...

When compiling a function (either one nested in a script or via calling js_CompileFunction) obj is fun->object that was created right before parsing. As such it does not have any non-hidden properties so pobj != obj in that case and JSFRAME_COMPILE_N_GO has no influence on the function parser/emitter.

The second case is regexp emitter:

        JS_ASSERT(pn->pn_op == JSOP_REGEXP);
        if (cx->fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO)) {
            ok = EmitObjectOp(cx, pn->pn_pob, JSOP_OBJECT, cg);
        } else {
            ok = EmitIndexOp(cx, JSOP_REGEXP,
                             IndexParsedObject(pn->pn_pob, &cg->regexpList),
                             cg);
        }

Here indeed JSFRAME_COMPILE_N_GO affects the function frame. But if JSFRAME_COMPILE_N_GO is set with the current code when js_CompileFunction is called then the code must ensure that the function will not be called again including a possibility for a recursive call via a function name or arguments.callee. Since there are no such checks, then using JSOP_OBJECT for regexp in a function is a premature optimization and as such is a bug.

So the changes from the patch to set TCF_COMPILE_N_GO only for a script do not affect the first case and fixes the premature optimization in the second case.

Comment 13

[Igor Bukanov]

(In reply to comment #11)
> Oh, I've been wanting to get rid of stupid compiler-fake frames forever! 

The patch removes the compiler frames for functions. The frames for scripts still exists as witnessed by MaybeSetupFrame. It is possible to remove them as well, but this is for another bug.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > Subtle difference: the JSOPTION_COMPILE_N_GO could be picked up by any
> > activation of js_EmitFunctionBody, js_CompileScript, or js_CompileFunctionBody
> > (thence propagated back into the emitter via JSFRAME_COMPILE_N_GO).
> 
> In the current code JSOPTION_COMPILE_N_GO is checked in 2 places. The first
> case is js_LookupCompileTimeConstant:
> 
> JSBool
> js_LookupCompileTimeConstant(JSContext *cx, JSCodeGenerator *cg, JSAtom *atom,
>                              jsval *vp)
> {
> ...
>     fp = cx->fp;
>     do {
> ... 
>         obj = fp->varobj;
>         if (obj == fp->scopeChain) {
> ...
> 
>             if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
>                 JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
>                 if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
>                     break;
>             }

Note your recent change here -- before, a hidden property would be found in the directly referenced obj (pobj == obj).

> 
>             ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj,
> &prop);
>             if (ok) {
>                 if (pobj == obj &&
>                     (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {

This previous logic was intentional, for Narcissus functions that switch on global constants.

Such functions cannot be called with dynamic scope, so if the compiler knows that they are being compiled as part of an evaluation (compile-and-go) of the top-level program in which they are defined, it can safely constant-propagate.

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)

> Note your recent change here -- before, a hidden property would be found in the
> directly referenced obj (pobj == obj).

Here is the code before the change:

if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
    ok = js_LookupHiddenProperty(cx, obj, ATOM_TO_JSID(atom),
                                             &pobj, &prop);
    if (!ok)
        break;
    if (prop) {
#ifdef DEBUG
        JSScopeProperty *sprop = (JSScopeProperty *)prop;

        /*
         * Any hidden property must be a formal arg or local var,
         * which will shadow a global const of the same name.
         */
        JS_ASSERT(sprop->getter == js_GetArgument ||
                  sprop->getter == js_GetLocalVariable);
#endif
        OBJ_DROP_PROPERTY(cx, pobj, prop);
        break;
    }
}

As with the current code, it was just looking for a function or variable name to break the loop when hitting one. The code was not looking for any property value.

> This previous logic was intentional, for Narcissus functions that switch on
> global constants.

Consider the code fragment again:

if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
    JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
    if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
        break;
}

ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj, &pprop);
if (ok) {
    if (pobj == obj &&
        (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {


When compiling a function obj is fun->object created right before the parsing. As such it does not have any properties besides hidden ones. Narcissus does not affect that. Thus the code would not find any properties in the object and can be optimized as:

if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
    JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
    if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
        break;
} else {
    ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj, &pprop);
    if (ok) {
        if (pobj == obj &&
            (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {

Now, when OBJ_GET_CLASS(cx, obj) != &js_FunctionClass and we are compiling a script of any kind, the loop will terminate here since for a script the code generator never has a parent. As such the check fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO) can be done before querying the property:

if (OBJ_GET_CLASS(cx, obj) == &js_FunctionClass) {
    JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, obj));
    if (js_LookupLocal(cx, fp->fun, atom, NULL) != JSLOCAL_NONE)
        break;
} else if (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO)) {
    ok = OBJ_LOOKUP_PROPERTY(cx, obj, ATOM_TO_JSID(atom), &pobj, &pprop);
    if (ok) {
        if (pobj == obj) {

This is effectively the transformation leading to the patch. So I continue to claim that the patch does not deoptimize here.

What happens is that the current code has already deoptimized js_CompileFunctionBody callers as it stops the loop in js_LookupCompileTimeConstant after it processes last JSCodeGenerator instance. For js_CompileFunctionBody it means that the global scope would never be looked up in the loop (the patch does nor change that). If my analysis is correct, I will file another bug about it.

Comment 16

[Igor Bukanov]

Attachment: v5b

The new version syncs the patch with CVS head, there are no changes in the patch itself.

Comment 17

[Brendan Eich [:brendan]]

I built a debug js shell with NARCISSUS=1 on the cmd line, and gdb'ed it in js/narcissus like so:

1662                        /*
1663                         * We're compiling code that will be executed immediately,
(gdb) 
1664                         * not re-executed against a different scope chain and/or
1665                         * variable object.  Therefore we can get constant values
1666                         * from our variable object here.
1667                         */
1668                        ok = OBJ_GET_ATTRIBUTES(cx, obj, ATOM_TO_JSID(atom), prop,
1669                                                &attrs);
1670                        if (ok && !(~attrs & (JSPROP_READONLY | JSPROP_PERMANENT)))
1671                            ok = OBJ_GET_PROPERTY(cx, obj, ATOM_TO_JSID(atom), vp);
1672                    }
1673                    if (prop)
(gdb) b 1671
Breakpoint 1 at 0x3da21: file jsemit.c, line 1671.
(gdb) r -f js.js -
Starting program: /Users/brendaneich/Hacking/trunk/mozilla/js/src.old/Darwin_DBG.OBJ/js -f js.js -
Reading symbols for shared libraries . done

Breakpoint 1, js_LookupCompileTimeConstant (cx=0x5001e0, cg=0xbfffdd30, atom=0x2fbd74, vp=0xbfffd5a8) at jsemit.c:1671
1671                            ok = OBJ_GET_PROPERTY(cx, obj, ATOM_TO_JSID(atom), vp);
(gdb) call js_AtomToPrintableString(cx, atom)
$1 = 0x506d40 "FUNCTION"
(gdb) n
1673                    if (prop)
(gdb) p/x *vp
$2 = 0x95
(gdb) p $>>1
$3 = 74
(gdb) p *cg
$4 = {
  treeContext = {
    flags = 129, 
    ngvars = 26, 
    globalUses = 21, 
    loopyGlobalUses = 3, 
    topStmt = 0x0, 
    topScopeStmt = 0x0, 
    blockChain = 0x0, 
    blockNode = 0x1810610, 
    decls = {
      list = 0x0, 
      table = 0x18595e0, 
      count = 17
    }, 
    parseContext = 0xbfffdf0c
  }, 
  codePool = 0xbfffde30, 
  notePool = 0xbfffde10, 
  codeMark = 0xbfffde40, 
  noteMark = 0xbfffde20, 
  prolog = {
    base = 0x1811e10 "", 
    limit = 0x1811f10 "l", 
    next = 0x1811e40 "t?/", 
    notes = 0x1811250 "?-?5?8ɸ?", 
    noteCount = 21, 
    noteMask = 63, 
    lastNoteOffset = 39, 
    currentLine = 332
  }, 
  main = {
    base = 0x1811f10 "l", 
    limit = 0x1812110 "V", 
    next = 0x1812099 '?' <repeats 119 times>, "V", 
    notes = 0x1811290 "?-6", 
    noteCount = 218, 
    noteMask = 255, 
    lastNoteOffset = 393, 
    currentLine = 334
  }, 
  current = 0xbfffdd8c, 
  firstLine = 1, 
  atomList = {
    list = 0x0, 
    table = 0x1850320, 
    count = 50
  }, 
  stackDepth = 0, 
  maxStackDepth = 7, 
  ntrynotes = 1, 
  lastTryNode = 0x1819578, 
  spanDeps = 0x0, 
  jumpTargets = 0x0, 
  jtFreeList = 0x0, 
  numSpanDeps = 0, 
  numJumpTargets = 0, 
  spanDepTodo = 0, 
  arrayCompSlot = 0, 
  emitLevel = 1, 
  constList = {
    list = 0x18587e0, 
    table = 0x0, 
    count = 3
  }, 
  objectList = {
    length = 26, 
    lastPob = 0x18586c0
  }, 
  regexpList = {
    length = 0, 
    lastPob = 0x0
  }, 
  parent = 0x0
}
(gdb) up
#1  0x000404ea in EmitSwitch (cx=0x5001e0, cg=0x18610d8, pn=0x18107d8, stmtInfo=0xbfffd724) at jsemit.c:2749
2749                        ok = js_LookupCompileTimeConstant(cx, cg, pn4->pn_atom, &v);
(gdb) p cg
$5 = (JSCodeGenerator *) 0x18610d8
(gdb) down
#0  js_LookupCompileTimeConstant (cx=0x5001e0, cg=0xbfffdd30, atom=0x2fbd74, vp=0xbfffd5a8) at jsemit.c:1673
1673                    if (prop)
(gdb) p cg
$6 = (JSCodeGenerator *) 0xbfffdd30

So the optimization works in current code. It should work too in your code (sorry for misreading), but I wanted to confirm, since your last comment doubts that the pre-existing code worked as designed. Can you build and gdb as above to confirm? Thanks,

/be

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> So the optimization works in current code.

The optimization will work (and I hope the patch would not change that) for any Compile*Script user. The issues is whether it works with js_CompileFunctionBody. Currently it would not and the code does not try to optimize a case like:

const a = 10;
const b = 11;

var f = Function(x, ""+
"   switch (x) {\n"+
"     case a: return true;\n"+
"     case b: return true;\n"+
"   }\n"+
"   return false;\n"
"}");

Yet if the function would be defined inside eval, the optimization will happen.

Comment 19

[Igor Bukanov]

I checked with gdb and with the patch I observed const propagation from the global scope.

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> I checked with gdb and with the patch I observed const propagation from the
> global scope.

That was for narcissus.

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #18)
> Yet if the function would be defined inside eval, the optimization will happen.

Ok, could use another bug to make the Function case work, but it's not a big deal to me -- please file if you think it's worth fixing.

/be

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 290372 [details] [diff] [review]
v5b

Good stuff, thanks for cleaning up a years-in-the-making mess of redundant flags and fake frames (ok, still have to get rid of the top-level script fake frames -- new bug?).

/be

Comment 23

[Igor Bukanov]

I checked in the patch from comment 16 to the CVS trunk:

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.376; previous revision: 3.375
done
Checking in js/src/jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.118; previous revision: 3.117
done
Checking in js/src/jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.286; previous revision: 3.285
done
Checking in js/src/jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.82; previous revision: 3.81
done
Checking in js/src/jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.236; previous revision: 3.235
done
Checking in js/src/jsinterp.h;
/cvsroot/mozilla/js/src/jsinterp.h,v  <--  jsinterp.h
new revision: 3.66; previous revision: 3.65
done
Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.401; previous revision: 3.400
done
Checking in js/src/jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.312; previous revision: 3.311
done
Checking in js/src/jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.54; previous revision: 3.53
done
Checking in js/src/jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.165; previous revision: 3.164
done
Checking in js/src/jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.38; previous revision: 3.37
done
Checking in js/src/jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.178; previous revision: 3.177
done