592973 - JM: Add var & arg escape information to JSScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

To optimize formal args as local variables, we need to known which ones escape. We already do this for local variables using a hack in the parser.

The goal of this patch is to unhackify the local var analysis results and then add arguments support.

Comment 1

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

Attachment: patch

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 471448 [details] [diff] [review]
patch

Just as a reminder, in our discussion we decided that the nvars test is probably a bug that would cause let vars to get excluded from the analysis.

+    if (JOF_OPTYPE(pn->pn_op) == JOF_LOCAL &&
+        pn->pn_cookie.slot() < cg->fun->u.i.nvars &&
+        ShouldMarkEscapingName(cg, pn))

The rest looks good, but I think we can do some ongoing C++ification:

- add accessors to replace the 'flags & FOOBAR' lines of code added by this patch.

- make nEscapingVars, nEscapingArgs, and escaping private and add needed accessors. IMO (and IME on TM), when members have a special layout, or require extra arithmetic or masking, C++ private + accessors are especially important to avoid bugs and get other coders to use the members correctly.

Comment 3

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

Attachment: patch v2

Changes:
 * Use the term "closed" instead of "escaping"
 * Be more C++ per review comments (couldn't do private because of emptyScript)
 * Correctly mark escaping slots in block objects

Comment 4

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

Comment on attachment 474846 [details] [diff] [review]
patch v2

Please ignore the test case change, it's reverted in my queue.

Brendan, could you review the VM changes?

Comment 5

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

Comment on attachment 474846 [details] [diff] [review]
patch v2

Dave, could you review the method JIT changes?

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 474846 [details] [diff] [review]
patch v2

>+static inline bool
>+ShouldMarkClosedName(JSCodeGenerator *cg, JSParseNode *pn)
>+{
>+    return !cg->callsEval() &&
>+           pn->pn_defn &&
>+           (((JSDefinition *)pn)->pn_dflags & PND_CLOSED);
>+}

Why not make this a cg method?

"Mark" is for GC marking (not "trace"). What verb do you mean here? Maybe "Note".

>+            if (JOF_OPTYPE(pn2->pn_op) == JOF_QARG &&
>+                ShouldMarkClosedName(cg, pn2)) {

Condition fits on one line.

>@@ -550,6 +549,10 @@ struct JSCodeGenerator : public JSTreeCo
>     GlobalUseVector globalUses;     /* per-script global uses */
>     JSAtomList      globalMap;      /* per-script map of global name to globalUses vector */
> 
>+    typedef js::Vector<uint32, 8, js::ContextAllocPolicy> SlotVector;
>+    SlotVector      closedArgs;
>+    SlotVector      closedVars;
>+


Comment what these are (level/slot cookies).

>     wscript->nslots = script->nslots;
>     wscript->staticLevel = script->staticLevel;
>     wscript->principals = script->principals;
>+    wscript->usesArguments = script->usesArguments;

Are we missing anything here? The warn-on-two-arg-eval flag?

>+        if (script->usesEval) {
>+            CopyValuesToCallObject(callobj, nargs, fp->formalArgs(), nvars, fp->slots());
>+        } else {
>+            JSScript *script = fun->u.i.script;
>+
>+            uint32 nclosed = script->nClosedArgs;
>+            for (uint32 i = 0; i < nclosed; i++) {
>+                uint32 e = script->getClosedArg(i);
>+                callobj.dslots[e] = fp->formalArg(e);
>+            }
>+
>+            nclosed = script->nClosedVars;
>+            for (uint32 i = 0; i < nclosed; i++) {
>+                uint32 e = script->getClosedVar(i);
>                 callobj.dslots[nargs + e] = fp->slots()[e];
>             }

Comment lightly?

>@@ -3005,7 +3005,7 @@ js_PutBlockObject(JSContext *cx, JSBool 
>     JS_ASSERT(count >= 1);
> 
>     if (normalUnwind) {
>-        uintN slot = JSSLOT_BLOCK_DEPTH + 1;
>+        uintN slot = JSSLOT_BLOCK_FIRST_FREE_SLOT;

(Thanks.)

>diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl
>--- a/js/src/jsopcode.tbl
>+++ b/js/src/jsopcode.tbl
>@@ -611,5 +611,3 @@ OPDEF(JSOP_GLOBALDEC,     244,"globaldec
> OPDEF(JSOP_CALLGLOBAL,    245,"callglobal",    NULL,  3,  0,  2, 19,  JOF_GLOBAL|JOF_NAME|JOF_CALLOP)
> OPDEF(JSOP_FORGLOBAL,     246,"forglobal",     NULL,  3,  1,  1, 19,  JOF_GLOBAL|JOF_NAME|JOF_FOR|JOF_TMPSLOT)
> 
>-OPDEF(JSOP_DEFUPVAR,      247,"defupvar",      NULL,  3,  0,  0, 19,  JOF_LOCAL|JOF_NAME)
>-

Yay!

>+    script = JSScript::NewScriptFromCG(cx, &cg);

Method names are usually camelCaps but this is a static ctor, and I kinda like the New. Never mind!

>     if (!JS_XDRUint32(xdr, &nconsts))
>         return JS_FALSE;
>+    if (!JS_XDRUint32(xdr, &nClosedArgs))
>+        return JS_FALSE;
>+    if (!JS_XDRUint32(xdr, &nClosedVars))
>+        return JS_FALSE;
>+    if (!JS_XDRUint32(xdr, &usesArguments))
>+        return JS_FALSE;

Can you pack this last bool with something else using a bit-set? Not sure it matters, but if any flags pack, don't burn a uint32 in the XDR data for each bit.

The counts should pack in 16 bits, no? Just asking -- ok to leave as 32 if they might be > 65535 (even if 65536).

>@@ -219,6 +243,7 @@ struct JSScript {
>     bool            warnedAboutTwoArgumentEval:1; /* have warned about use of
>                                                      obsolete eval(s, o) in
>                                                      this script */
>+    bool            usesArguments;    /* script uses arguments */

Make this a bool usesArguments:1 and consider putting it next to usesEval.

>@@ -229,6 +254,8 @@ struct JSScript {
>     uint32          lineno;     /* base line number of script */
>     uint16          nslots;     /* vars plus maximum stack depth */
>     uint16          staticLevel;/* static level for display maintenance */
>+    uint16          nClosedArgs; /* number of args which are closed over. */
>+    uint16          nClosedVars; /* number of vars which are closed over. */

Yeah, 16 bits each for nClosed{Args,Vars}.

How does this pack on 64-bit?

>-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 70)
>+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 71)

Are tm and m-c now in sync and using 70 as the maximum subtrahend?

>+FrameState::isClosedVar(uint32 slot)
>+{
>+    JS_ASSERT(closedVars[slot] == 0 || closedVars[slot] == 1);
>+    return !!closedVars[slot];
>+}
>+
> } /* namspace mjit */
> } /* namspace js */
> 
>diff --git a/js/src/methodjit/FrameState.cpp b/js/src/methodjit/FrameState.cpp
>--- a/js/src/methodjit/FrameState.cpp
>+++ b/js/src/methodjit/FrameState.cpp
>@@ -75,7 +75,7 @@ FrameState::init(uint32 nargs)
>     uint8 *cursor = (uint8 *)cx->malloc(sizeof(FrameEntry) * nslots +       // entries[]
>                                         sizeof(FrameEntry *) * nslots +     // base[]
>                                         sizeof(FrameEntry *) * nslots +     // tracker.entries[]
>-                                        sizeof(uint32) * nlocals            // escaping[]
>+                                        sizeof(uint32) * nslots             // closedVars[]

Do you need 32 bits for closedVars' element type? The closedVars[i] uses look like byte-wide (or bit-wide) booleans but I may be missing something.

r=me with the above addressed.

/be

Comment 7

[Brendan Eich [:brendan]]

David, don't forget to update the last sentence of the comment on the last code paragraph in jsparse.cpp:BindLet:

    /*
     * Store pn temporarily in what would be shape-mapped slots in a cloned
     * block object (once the prototype's final population is known, after all
     * 'let' bindings for this block have been parsed). We free these slots in
     * jsemit.cpp:EmitEnterBlock so they don't tie up unused space in the so-
     * called "static" prototype Block.
     */

/be

Comment 8

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

Attachment: interdiff of changes

Fixes a bug where |f(x) { function x() {} }| wasn't rewriting the argument pn to JSOP_NAME.

Attachment is interdiff for easier review

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 475602 [details] [diff] [review]
interdiff of changes

>+JSCodeGenerator::shouldNoteClosedName(JSParseNode *pn)
> {
>-    return !cg->callsEval() &&
>-           pn->pn_defn &&
>-           (((JSDefinition *)pn)->pn_dflags & PND_CLOSED);
>+    JSDefinition *dn = (JSDefinition *)pn;
>+    return !callsEval() && pn->pn_defn && (dn->pn_dflags & PND_CLOSED);

No need for dn, just use pn -- and it looks odd to cast and init dn before the pn->pn_defn test.

Even with this, could use an isClosed() const bool flag testing inline method alongside other PND_* testers.

>+    wscript->compileAndGo = script->compileAndGo;
>+    wscript->usesEval = script->usesEval;
>+    wscript->warnedAboutTwoArgumentEval = script->warnedAboutTwoArgumentEval;
>     wscript->usesArguments = script->usesArguments;

Order as in struct, so uses{Eval,Arguments} are adjacent and same order w.r.t. other bitfields as in declaration.

> MakeDefIntoUse(JSDefinition *dn, JSParseNode *pn, JSAtom *atom, JSTreeContext *tc)
> {
>     /*
>-     * If dn is var, const, or let, and it has an initializer, then we must
>-     * rewrite it to be an assignment node, whose freshly allocated left-hand
>-     * side becomes a use of pn.
>+     * If dn is arg, or in [var, const, let], and has an initializer, then we

Lose the , after ] here to avoid "and has an initializer" seeming to apply to "is arg" too.

>+     * must rewrite it to be an assignment node, whose freshly allocated
>+     * left-hand side becomes a use of pn.
>      */
>-    if (dn->isBindingForm()) {
>+    if (dn->isArgOrBindingForm()) {

Where's the testcase for this case? How'd we do without it so far?

>+enum ScriptBits {
>+    ScriptBit_NoScriptRval,
>+    ScriptBit_SavedCallerFun,
>+    ScriptBit_HasSharps,
>+    ScriptBit_StrictModeCode,
>+    ScriptBit_UsesEval,
>+    ScriptBit_CompileAndGo,
>+    ScriptBit_UsesArguments
>+};

No need for ScriptBit_ prefix, I think.

>     uint8 *cursor = (uint8 *)cx->malloc(sizeof(FrameEntry) * nslots +       // entries[]
>                                         sizeof(FrameEntry *) * nslots +     // base[]
>                                         sizeof(FrameEntry *) * nslots +     // tracker.entries[]
>-                                        sizeof(uint32) * nslots             // closedVars[]
>+                                        sizeof(bool) * nslots               // closedVars[]

Is bool packed well? JSPackedBool might be better, cross-compiler/platform and for non-increasing size order (why couldn't bool be int64 or some ILP64 target?).

/be

Comment 10

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

Attachment: patch v3

Addresses comments.

The reason for isArgOrBindingForm() is shown in the second test case, where a named function statement aliases an argument. But it was wrong to test the pn here at all, because it's not a definition anymore. I've reverted this change for a simpler test.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 476320 [details] [diff] [review]
patch v3

>+    if (!JS_XDRUint16(xdr, &nClosedArgs))
>+        return JS_FALSE;
>+    if (!JS_XDRUint16(xdr, &nClosedVars))
>+        return JS_FALSE;
>+    if (!JS_XDRUint8(xdr, &scriptBits))
>+        return JS_FALSE;

I'd still argue you should use a uint32 nClosed = (nClosedArg << 16) | nClosedVars when encoding (and cracked apart the opposite way after the JS_XDRUint32 call when decoding), and use an explicit uint32 for scriptBits. r=me with that. Thanks,

/be

Comment 12

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

This is failing an xpcshell test that closes over destructuring arguments. Follow-up fix coming shortly.

Comment 13

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

Hrm. The problem is with this code:

  function f([a, b, c]) {
    /* inner function that closes over a, b, c */
  }

functionArguments() creates a TOK_ASSIGN node. We skip EmitVariables(), which skips MaybeEmitVarDecl, which skips all of the closed name marking.

It seems wrong to put a MaybeEmitVarDecl in EmitDestructuringLHS. Should the TOK_ASSIGN be a TOK_VAR?



It seems wrong to call MaybeEmitVarDecl in EmitDestructuringLHS.

Comment 14

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

Attachment: interdiff

Fixes the bug in comment #13. Decompilation output doesn't seem to be affected, passes trace-tests and the failing xpcshell test.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 476952 [details] [diff] [review]
interdiff

Glad TOK_VAR worked out -- it ought to have, AST-shape-wise.

/be

Comment 16

[Robert Sayre]

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