Closed
Bug 592973
Opened 14 years ago
Closed 14 years ago
JM: Add var & arg escape information to JSScript
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(2 files, 3 obsolete files)
41.32 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
3.84 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #471448 -
Flags: review?(dmandelin)
Comment 2•14 years ago
|
||
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.
Attachment #471448 -
Flags: review?(dmandelin)
Assignee | ||
Comment 3•14 years ago
|
||
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
Attachment #471448 -
Attachment is obsolete: true
Assignee | ||
Comment 4•14 years ago
|
||
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?
Attachment #474846 -
Flags: review?(brendan)
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 474846 [details] [diff] [review] patch v2 Dave, could you review the method JIT changes?
Attachment #474846 -
Flags: review?(dmandelin)
Updated•14 years ago
|
Attachment #474846 -
Flags: review?(dmandelin) → review+
Comment 6•14 years ago
|
||
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
Attachment #474846 -
Flags: review?(brendan) → review+
Comment 7•14 years ago
|
||
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
Assignee | ||
Comment 8•14 years ago
|
||
Fixes a bug where |f(x) { function x() {} }| wasn't rewriting the argument pn to JSOP_NAME. Attachment is interdiff for easier review
Attachment #475602 -
Flags: review?(brendan)
Comment 9•14 years ago
|
||
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
Assignee | ||
Comment 10•14 years ago
|
||
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.
Attachment #474846 -
Attachment is obsolete: true
Attachment #475602 -
Attachment is obsolete: true
Attachment #476320 -
Flags: review?(brendan)
Attachment #475602 -
Flags: review?(brendan)
Comment 11•14 years ago
|
||
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
Attachment #476320 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 12•14 years ago
|
||
This is failing an xpcshell test that closes over destructuring arguments. Follow-up fix coming shortly.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
Fixes the bug in comment #13. Decompilation output doesn't seem to be affected, passes trace-tests and the failing xpcshell test.
Attachment #476952 -
Flags: review?(brendan)
Comment 15•14 years ago
|
||
Comment on attachment 476952 [details] [diff] [review] interdiff Glad TOK_VAR worked out -- it ought to have, AST-shape-wise. /be
Attachment #476952 -
Flags: review?(brendan) → review+
Comment 16•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fb50e5ff2dab
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•