561923 - JM: pre-define global variables while parsing statically bound gvars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I would like to introduce new alternatives to SET/GETGVAR and friends, that have an extra 16-bit immediate slot, corresponding to the slot on the global object.

The documentation for COMPILE_N_GO requires that callers:

> execute the compiled script once only, in the same scope object used for
> compilation.  The caller may not modify objects on the scope chain between
> compilation and execution.

With this flag present, we can do something. The easiest thing would be to not emit JSOP_DEFVAR, and instead add the global properties during the emitter phase. I'm not sure if this is safe though, since then the global object will collect random properties observable between compilation and execution. It'll also be left in a weird state if compilation fails. It could also introduce compilation failures that would have only been seen at execution.

If none of that is good, we can figure out how to reserve the slots during parsing, or failing that, do nothing and let the method JIT add the properties (since it compiles on first execution).

Comment 1

[Brendan Eich [:brendan]]

COMPILE_N_GO means there's no observation point in between compilation and execution. It's as if you precompiled, then ran the script prolog that contains the JSOP_DEFVARs, then the main script body.

If you can define globals when emitting, then you can be sure none name a getter or setter or other pre-bound property, just a value slot to memorize. This wins.

But in this case you don't need an extra 16-bit immediate. All you need is some way for the decompiler to map the one and only 16-bit slot number to an interned string (atom). Such a mapping can be built in the JSScript, as other maybe-array extensions of the script struct are (JSUpvarArray, etc.).

/be

Comment 2

[Brendan Eich [:brendan]]

The point about leaving effects on SyntaxError is good, though -- forgot to reply to that. One way to do it is to compile the full script including the information on the globals (script->globals()), but do not define these properties on the global until compilation succeeds.

However, you should be able to speculatively emit their slots as immediates, since you can be sure slots are allocated contiguously from the current freeslot value of the global's scope.

/be

Comment 3

[Brendan Eich [:brendan]]

The globals table in each script needs to map only the globals referenced by that script (and not by any functions embedded in it, and so on for functions nested in functions). Function scripts too have their own globals tables, finished in js_NewScriptFromCG likewise.

This implies js::Compiler needs the full mapping from name to global slot, so all the functions nested in a top-level script share the same name to slot mapping.

After compilation succeeds (including bytecode emission predicting the slots of all the globals), run through the globals table in slot order defining properties on the global object. This suggests threading a definition- (or slot-)order link through entries in a hashtable. Since you never free, the table can use an arena based allocation policy.

/be

Comment 4

[Brendan Eich [:brendan]]

May want to keep GVAR ops for now, so add new ops. If you run short ping me -- high time to desugar E4X ops into calls.

/be

Comment 5

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

Awesome, the solution in comment #2 seems like the way to go. At first it seemed like having two immediates would be easier, but seeing the various ways name ops can get emitted, changing the emitter might be worse than changing the decompiler.

Yeah I've kept GVAR ops since I don't want to regress their old functionality, and shimming both features into the same opcode set was not going well. We've still got about 15 slots left so we're good for now.

Comment 6

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

Attachment: wip v1

Whoops - turns out it's harder to predict globalObj->freeslot, since the parser can lazily init standard classes. For now I'm just defining the property immediately if it doesn't already exists. If it can't be added because of OOM or something, that's fine, I can revert to JSOP_NAME and continue parsing.

If the error is anything else, I'm not sure what to do. Aborting parsing as noted earlier would mask a SyntaxError that would have been seen otherwise. I could just swallow the exception, maybe?

Comment 7

[Brendan Eich [:brendan]]

Confusion: don't define properties on the global during parsing, just build up the list/hash of all globals in the js::Compiler. Then after successful compiler run bind the names.

The freeslot issue goes away if you just forcibly call js_InitRegExpClass.

The hybrid style bug that produces

bool
js_Mumble(...) {
   ...
}

is biting here. Better to put the { on its own line, if you can't put this in a class and put all of the bool return type, method declarator, and { on the same line!

/be

Comment 8

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

Can we pre-reserve the slots for standard classes, even if we don't init them?  I believe webkit does, and it means that they can use fixed slots for known names, among other virtues.

Comment 9

[Brendan Eich [:brendan]]

We absolutely can reserve at least a slot for each constructor, since we know how many of those there are in order to reserve (via JSCLASS_GLOBAL_FLAGS) the slots for the original values of Object, Function, etc.

JSProto_LIMIT is the number of standard class constructors or named prototype-ish objects (Math). You could double the reservation and add some js_InitClass magic to consume the second bunch of reserved slots when it defines the named classes.

This would require communicating the reserved slot down via obj->defineProperty or js_DefineFunction called from js_InitClass, somehow. Maybe there's a way to do it unobtrusively. These both bottom out in js_DefineNativeProperty on the global object.

One thought: change js_InitClass to cut through this layering by separating cases, creating the needed prototype or constructor function (the latter via js_NewFunction), then bind it to its reserved slot using JSScope::addProperty. Do this only for the classes with key != JSProto_Null. This confines the damage to js_InitClass.

/be

Comment 10

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

Attachment: wip v2

Brendan, something like this?

Comment 11

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

PS - I will fix style typos before I checkin.

Comment 12

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

js_InitMathClass does not go through js_InitClass. I don't think the parser can lazily instantiate it though...

Comment 13

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

Attachment: wip v3

Comment 14

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

I was insufficiently clear, I apologize:

When we JS_NewObject an object with JSCLASS_GLOBAL, we reserve N slots, defined such that Math is _always_ slot 1, String is _always_ slot 2, etc.  There is no need to communicate it down, since the numbering is fixed.  Reference to Math becomes a fixed slot(1) access on the global, irrespective of global shape.  (If any standard classes are deletable, we will have to cry a bit.)  The parser no longer has to initialize standard classes (it could, if we want to do that asynchronously in a speculative-parsing HTML5 awesomeness world, but that happens on another thread, so...)

For extra credit, we could use the new fixed location of the standard classes' constructors to speed up prototype lookup for implicit uses of them, perhaps!

Comment 15

[Brendan Eich [:brendan]]

Re: comment 12: yeah, no Math instantiation at compile time.

The standard classes are writable and configurable. Comment 9 and comment 14 are close, but the idea in comment 14 of recognizing "Object", "RegExp", etc. in the compiler and hardcoding slots is missing from comment 9 -- but delete means we have to do a name lookup.

js> Object.getOwnPropertyDescriptor(this, "Math")
({value:Math, writable:true, enumerable:false, configurable:true})
js> Object.getOwnPropertyDescriptor(this, "RegExp")
({value:function RegExp() {[native code]}, writable:true, enumerable:false, configurable:true})

With var in global code, you get non-configurability -- no delete. That is the vaunted integrity property of var which, in a closure, means you can't mess with name bindings:

js> Object.getOwnPropertyDescriptor(this, 'foozle')
({value:42, writable:true, enumerable:true, configurable:false})

And it's what enabled the GVAR optimization all those years ago: the slot won't change, even though precompilation ruled out memozing it into the GVAR op's immediate operand (indirect through fp->slots instead). But we coulda rewritten global ops on the fly, copy-on-writing JSScripts, etc. Still can if the scripts (including functions nested in global code) are live and global-happy, but it is not obviously worth it.

Can't wait for this patch to land!

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #14) 
> For extra credit, we could use the new fixed location of the standard classes'
> constructors to speed up prototype lookup for implicit uses of them, perhaps!

This is what the fixed locations for the *original* values of the standard class's constructors is for (the slots reserved by JSCLASS_GLOBAL_FLAGS). ES3 inconsistently, and ES5 always, make implicit uses of the standard constructors refer to the original, not current (overwritable, deleteable), values of these bindings. This is what jsproto.tbl etc. is for. See bug 304376.

We could also pin *current* value of a global standard constructor to a slot, even in the face of delete, provioded we had machinery to recycle the slot to the same name, reserving the slot until a new assignment or var (or internal API call) created such a name. But we don't have that machinery. Worth a bug?

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 442358 [details] [diff] [review]
wip v3

>+            if (cg->inFunction() && cg->heavyweight())
>+                return JS_TRUE;
>+
>+

Double blank line to silently comment on heavyweight's nastiness, I get it -- but one blank line will do ;-)

>+            JSCodeGenerator *globalCg = cg->globalScope->cg;
>+            if (globalCg != cg) {
>+                uint32 slot = globalCg->globalUses[cookie].slot;
>+
>+                /* Fall back to NAME if we can't add a slot. */
>+                if (!cg->addGlobalUse(atom, slot, &cookie))
>+                    return JS_TRUE;

Need to avoid suppressing OOM here. Use bool return value for failure only, use FREE_UPVAR_COOKIE in cookie to signal overflow.

Did you try putting globalUses in the js::Compiler reachable from every cg->compiler? Then you wouldn't need this extra logic with globalCg. But with this logic, you can bind globals used in global code in the parser, while this case handles globals in functions. Is that necessary? It seems you could bind 'em all here, using cg->compiler->....

>+JSCodeGenerator::addGlobalUse(JSAtom *atom, uint32 slot, uint32 *indexp)
>+{
>+    JSAtomListElement *ale = globalMap.lookup(atom);
>+    if (ale) {
>+        *indexp = ALE_INDEX(ale);
>+        return true;
>+    }
>+
>+    /* Don't bother encoding indexes >= uint16 */
>+    if (globalUses.length() >= UINT16_LIMIT)
>+        return false;

FREE_UPVAR_COOKIE and return true.

>+    /* Find or add an existing atom table entry. */
>+    ale = atomList.add(parser, atom);
>+    if (!ale)
>+        return false;

Good OOM propagation.

>+    *indexp = uint32(globalUses.length());
>+
>+    JSGlobalSlotArray::Entry entry;
>+    entry.atomIndex = ALE_INDEX(ale);
>+    entry.slot = slot;
>+    if (!globalUses.append(entry))
>+        return false;

Ditto.

>+    /* The map optimization can fail safely. */
>+    ale = globalMap.add(parser, atom);
>+    if (ale)
>+        ALE_SET_INDEX(ale, *indexp);

Oops, OOM suppressed here.

>@@ -3483,26 +3483,84 @@ js_InitClass(JSContext *cx, JSObject *ob

Need a common subroutine factored out here, or recombine the logic with more ifs. More below (- lines deleted):

>         if ((clasp->flags & JSCLASS_IS_ANONYMOUS) &&
>             (obj->getClass()->flags & JSCLASS_IS_GLOBAL) &&
>             key != JSProto_Null) {

Here we do nothing now, may as well invert the condition and lose the empty then.

>         } else {
>+            uint32 attrs = (clasp->flags & JSCLASS_IS_ANONYMOUS) ?
>+                           JSPROP_READONLY | JSPROP_PERMANENT : 0;
>+
>+            if (obj->isNative() && key != JSProto_Null) {

Want (obj->getClass()->flags & JSCLASS_IS_GLOBAL) instead of obj->isNative() (it implies native) here. But wait, it may be that having a non-null key is enough and you can assert. Yeah, try that.

>+                JSObject *obj2;
>+                JSProperty *prop;
>+
>+                if (!obj->lookupProperty(cx, ATOM_TO_JSID(atom), &obj2, &prop))
>+                    goto bad;
>+
>+                if (!prop) {

JS_LOCK_OBJ(cx, obj) here.

>+                    JSScope *scope = obj->scope();
>+                    uint32 slot = JSSLOT_START(obj->getClass()) + JSProto_LIMIT + key;
>+
>+                    JSScopeProperty *sprop = scope->addProperty(cx, ATOM_TO_JSID(atom),
>+                                                                JS_PropertyStub, JS_PropertyStub,
>+                                                                slot, attrs, 0, 0);
>+                    if (!sprop)
>+                        goto bad;
>+
>+                    obj->setSlot(slot, OBJECT_TO_JSVAL(proto));

JS_UNLOCK_SCOPE(cx, scope) here and avoid goto bad without unlocking.

>+                    named = JS_TRUE;
>+                } else {
>+                    obj->dropProperty(cx, prop);
>+                }
>+            }
>+
>+            if (!named) {
>+                named = obj->defineProperty(cx, ATOM_TO_JSID(atom),
>+                                            OBJECT_TO_JSVAL(proto),
>+                                            JS_PropertyStub, JS_PropertyStub,
>+                                            attrs);
>+                if (!named)
>+                    goto bad;
>+            }
>         }
> 
>         ctor = proto;
>     } else {
>         /* Define the constructor function in obj's scope. */
>-        fun = js_DefineFunction(cx, obj, atom, constructor, nargs,
>-                                JSFUN_STUB_GSOPS);

Ugh, the whole duplicative blob above is just for Math? I think so. Check, and if so skip it or definitely refactor a common helper.

>-        named = (fun != NULL);
>-        if (!fun)
>-            goto bad;
>+        if (obj->isNative() && key != JSProto_Null) {
>+            JSObject *obj2;
>+            JSProperty *prop;
>+
>+            if (!obj->lookupProperty(cx, ATOM_TO_JSID(atom), &obj2, &prop))
>+                goto bad;
>+
>+            if (!prop) {
>+                JSScope *scope = obj->scope();
>+                uint32 slot = JSSLOT_START(obj->getClass()) + JSProto_LIMIT + key;
>+
>+                JSScopeProperty *sprop = scope->addProperty(cx, ATOM_TO_JSID(atom),
>+                                                            JS_PropertyStub, JS_PropertyStub,
>+                                                            slot, 0, 0, 0);
>+                
>+                if (!sprop)
>+                    goto bad;
>+
>+                named = JS_TRUE;
>+
>+                fun = js_NewFunction(cx, NULL, constructor, nargs, 0, obj, atom);
>+                if (!fun)
>+                    goto bad;
>+
>+                obj->setSlot(slot, OBJECT_TO_JSVAL(fun));
>+            } else {
>+                obj->dropProperty(cx, prop);
>+            }
>+        }

Ditto. When I hack fast but find myself copying, I lie down for a minute, come back, and refactor ;-).

/be

Comment 18

[Brendan Eich [:brendan]]

David, I suspect js_InitClass should branch early (after proto has been created) on key != JSProto_Null and do the special thing first, else the old thing. This may avoid duplication in the no-constructor vs. constructor path.

/be

Comment 19

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

(In reply to comment #14)
> For extra credit, we could use the new fixed location of the standard classes'
> constructors to speed up prototype lookup for implicit uses of them, perhaps!

For the other half of this bug ("unbound globals") we could compile the monomorphic inline cache as if it already hit once, for global properties that are already there but deletable. Maybe we could even brand the standard class names and bake in their addresses. I'll worry about this in the next bug though.

(In reply to comment #17)
> Is that necessary? It seems you could bind 'em all here, using
> cg->compiler->....

globalUses is to build each script's atom+slot mapping, the VM looks in this table to get the slot (decompiler to get the atom).

I don't know why I was suppressing OOM. It was definitely intentional, but I'm not sure what I was thinking.

Will refactor the js_InitClass path.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> (In reply to comment #17)
> > Is that necessary? It seems you could bind 'em all here, using
> > cg->compiler->....
> 
> globalUses is to build each script's atom+slot mapping, the VM looks in this
> table to get the slot (decompiler to get the atom).

I know, but I'm arguing for (a) making this table live in js::Compiler since it is not per-JSCodeGenerator, rather per-compilation-unit where the unit is global code; (b) building this table only in the emitter (after useless expression elimination). Then you wouldn't need as much code in BindVarOrConst, but you'd lose the ability to fall back there on the *GVAR ops.

(a) and (b) are separable. I think (a) is a clear win, since the BindVarOrConst code you added (as with the *GVAR code already there) applies only if tc is a cg (tc->compiling()). You really do want cg->compiler->globalScope, then you don't have to propagate cg->globalScope to cg2->globalScope, etc.

Another note on the patch:

+            if (!globalObj->isNative())
+                return JS_TRUE;

The global object must be native, this is effectively required elsewhere (e.g. js_FindClassObject) but perhaps not asserted. Could you file a bug to batten down this hatch? Thanks.

/be

Comment 21

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

Attachment: wip v4

passing trace tests, still got cleanups to go & implementing both JIT paths

Comment 22

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

Filed follow-up bug 563040 as per comment #20.

Comment 23

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

Attachment: wip v5

tracer + method jit support added

jaeger only, this is a 3.5% (30ms) win on SS, 5.5% (350ms) win on v8
no noticeable change with tracing on SS, maybe very small win on v8

final cleanups coming before landing

Comment 24

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

http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/07916d022ab2

I moved GlobalScope from CG to Compiler, but I had trouble moving the binding from BindVarOrConst to BindNameToSlot. It has a few expectations I was breaking. I'll have another go at it in bug 562729.

Comment 25

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

Comment on attachment 442858 [details] [diff] [review]
wip v5

Dave Mandelin suggested trying to get this on tracemonkey to ease upcoming merge pains.

How valuable do you think it is to move work from BindVarOrConst? It does put all of the logic in one place which is nice, but then BindNameToSlot gets a non-JOF_ATOM op with a FREE_UPVAR_COOKIE. This seems to confuse the emitter. BindNameToSlot also has to be able to undo the [G|S]ETGLOBAL op back to NAME for some nodes. That's kind of gross, a PND_GLOBAL would help.

Maybe I could keep re-using PND_GVAR, keep the JSOP_*GVAR stuff until the last minute - but use a FREE_UPVAR_COOKIE to distinguish and rewrite to JSOP_*GLOBAL in the emitter.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> Maybe I could keep re-using PND_GVAR, keep the JSOP_*GVAR stuff until the last
> minute - but use a FREE_UPVAR_COOKIE to distinguish and rewrite to JSOP_*GLOBAL
> in the emitter.

Is it ok to have cg->nvars advanced to one more than the atom index of the gvar's name? If so, this does sound better.

/be

Comment 27

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

It seems like the eager binding in BindVarOrConst() is the right place to do both GVAR and GLOBAL. Otherwise BindNameToSlot() has to first try binding the definition before binding the use.

Unless... are we guaranteed that BindNameToSlot() will see all definitions before uses? Keeping in mind we might eventually want examples like this to use fast globals as well:

   function f() {
      return x;
   }
   var x;

If we have that guarantee, then it might actually make sense to move the GVAR binding into BindNameToSlot() as well.

Comment 28

[Brendan Eich [:brendan]]

BindNameToSlot won't run in def before use order, just in AST traversal order. So your original design seems good (modulo cg->compiler->globalScope).

/be

Comment 29

[Brendan Eich [:brendan]]

One principle I've used in the compiler is "act immediately on available information". While this can backfire in general with optimizing compilers (see the PEG [Program Expression Graph, not Parsing Expression Grammar, ["Equality Saturation: a new Approach to Optimization", Ross Tate, Michael Stepp, Zachary Tatlock, Sorin Lerner @UCSD]), it seems to be winning in a simple compiler such as ours. So yeah, BindVarOrConst in the front side of the front end.

/be

Comment 30

[Brendan Eich [:brendan]]

New patch with globalScope in js::Compiler, or did that not work out?

/be

Comment 31

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

Attachment: part 1: init classes in reserved slots

breaking this up into smaller pieces

Comment 32

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

Attachment: part 2: don't resolve for constant lookups in the parser

Comment 33

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

Attachment: part 3: static binding

2% interpreter win on ss
2% tracing win on v8

interpreter on v8 stays the same, some tests win but crypto gets 10% slower. i'm not sure why yet, will investigate.

Comment 34

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

Attachment: part 3.1: static binding

can't reproduce that on other machines, but new patch for a small change to SCOPE_INJECTED flagged

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 443257 [details] [diff] [review]
part 1: init classes in reserved slots

>+            uint32 slot = JSSLOT_START(obj->getClass()) + index;
>+            sprop = scope->addProperty(cx, id, JS_PropertyStub, JS_PropertyStub,
>+                                       slot, attrs, 0, 0);
>+
>+            JS_UNLOCK_SCOPE(cx, scope);
>+
>+            if (!sprop)

Nit: lose the second blank line.

>+                return false;
>+
>+            named = JS_TRUE;
>+            return true;
>+        }
>+
>+        JS_UNLOCK_SCOPE(cx, scope);
>+    }
>+
>+    named = obj->defineProperty(cx, id, v, JS_PropertyStub, JS_PropertyStub, attrs);
>+

Lose this one too.

>+    return named;
>+}
>+
> JSObject *
> js_InitClass(JSContext *cx, JSObject *obj, JSObject *parent_proto,
>              JSClass *clasp, JSNative constructor, uintN nargs,
>@@ -3388,8 +3440,8 @@ js_InitClass(JSContext *cx, JSObject *ob
>     JSProtoKey key;
>     JSObject *proto, *ctor;
>     jsval cval, rval;
>-    JSBool named;
>     JSFunction *fun;
>+    JSBool named = JS_FALSE;

How about bool/true/false? May need a !! or two.

>+        if (!(clasp->flags & JSCLASS_IS_ANONYMOUS) ||
>+            !(obj->getClass()->flags & JSCLASS_IS_GLOBAL) ||
>+            key == JSProto_Null)

API compat requires us to tolerate either but not both of the second and third conditions, so we need this code or equivalent. The assertion in DefineStandardSlot covers the "xor" aspect. Just noting.

>+        {
>+            uint32 attrs = (clasp->flags & JSCLASS_IS_ANONYMOUS) ?
>+                           JSPROP_READONLY | JSPROP_PERMANENT : 0;

?: style wants ? and : stacked under the (.

r=me with nits picked.

/be

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 443258 [details] [diff] [review]
part 2: don't resolve for constant lookups in the parser

>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp
>--- a/js/src/jsemit.cpp
>+++ b/js/src/jsemit.cpp
>@@ -68,6 +68,7 @@
> #include "jsscript.h"
> #include "jsautooplen.h"
> #include "jsstaticcheck.h"
>+#include "jsscopeinlines.h"

Bonus points for following style used, e.g. in jstracer.cpp:

jstracer.cpp:#include "jsautooplen.h"        // generated headers last

for ordering and grouping (blank line para separators) these.

/be

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 443409 [details] [diff] [review]
part 3.1: static binding

>+JSCodeGenerator::addGlobalUse(JSAtom *atom, uint32 slot, uint32 *indexp)
>+{
>+    JSAtomListElement *ale = globalMap.lookup(atom);
>+    if (ale) {
>+        *indexp = ALE_INDEX(ale);
>+        return true;
>+    }
>+
>+    /* Don't bother encoding indexes >= uint16 */
>+    if (globalUses.length() >= UINT16_LIMIT) {
>+        *indexp = FREE_UPVAR_COOKIE;
>+    }

Missing "return true;" here.

>+    /* The map optimization can fail safely. */
>+    ale = globalMap.add(parser, atom);
>+    if (!ale)
>+        return false;
>+
>+    ALE_SET_INDEX(ale, *indexp);
>+
>+    return true;

Kill blank line here -- it's house style to bunch up success-case code in a single para.

> #define TCF_FUN_ENTRAINS_SCOPES 0x400000
> 
> /*
>+ * Function's lexical scope is poisoned by an eval() or with.
>+ */
>+#define TCF_SCOPE_INJECTED      0x800000
>+
>+/*
>+ * Compiling an eval() script.
>+ */
>+#define TCF_COMPILE_FOR_EVAL   0x1000000

Why are these needed? Just test callerFrame (more on "with" below).

>@@ -2698,6 +2730,16 @@ Parser::functionDef(uintN lambda, bool n
>     /* Initialize early for possible flags mutation via destructuringExpr. */
>     JSTreeContext funtc(tc->parser);
> 
>+    /* Look for WITH statements that defeat global binding. */
>+    JSStmtInfo *stmt = outertc->topStmt;
>+    while (stmt) {
>+        if (stmt->type == STMT_WITH) {
>+            tc->flags |= TCF_SCOPE_INJECTED;
>+            break;
>+        }
>+        stmt = stmt->down;
>+    }
>+

This is wrong, it pollutes the whole tc even if the "with" covers little code. Ask cdleary about what he is doing in another bug for precise "with" pollution.

>@@ -3257,33 +3308,82 @@ OuterLet(JSTreeContext *tc, JSStmtInfo *
>  * stack frame slots.
>  */
> static bool
>-BindGvar(JSParseNode *pn, JSTreeContext *tc, bool inWith = false)
>+BindGvar(JSParseNode *pn, JSTreeContext *tc)

You want inWith here still.

> {
>     JS_ASSERT(pn->pn_op == JSOP_NAME);
>     JS_ASSERT(!tc->inFunction());
> 
>+    if (!tc->compiling() || tc->parser->callerFrame)
>+        return true;
>+
>+    JSCodeGenerator *cg = (JSCodeGenerator *) tc;
>+
>+    JS_ASSERT(cg->compiler()->globalScope);
>+    GlobalScope *globalScope = cg->compiler()->globalScope;
>+    JSObject *globalObj = globalScope->globalObj;

Don't assert first, just load and deref.

>+    if (cg->compileAndGo() && globalObj && !(pn->pn_dflags & PND_CONST) &&
>+        !cg->compilingForEval())

The tc->parser->callerFrame test already covered !cg->compilingForEval().

>+    {
>+        JS_LOCK_OBJ(cx, globalObj);
>+        JSScope *scope = globalObj->scope();
>+        if (JSScopeProperty *sprop = scope->lookup(ATOM_TO_JSID(pn->pn_atom))) {
>+            /*
>+             * If the property was found, bind the slot immediately if
>+             * we can. If we can't, don't bother emitting a GVAR op,
>+             * since it's unlikely that it will optimize either.
>+             */
>+            uint32 index;
>+            if (!sprop->configurable() &&
>+                SPROP_HAS_VALID_SLOT(sprop, globalObj->scope()) &&
>+                sprop->hasDefaultGetterOrIsMethod() &&
>+                sprop->hasDefaultSetter() &&
>+                cg->addGlobalUse(pn->pn_atom, sprop->slot, &index) &&
>+                index != FREE_UPVAR_COOKIE)
>+            {
>+                pn->pn_op = JSOP_GETGLOBAL;
>+                pn->pn_cookie = index;
>+                pn->pn_dflags |= PND_BOUND | PND_GVAR;
>+            }
>+
>+            JS_UNLOCK_SCOPE(cx, scope);
>+
>             return true;

Nit about blank line.

>+        }
>+        JS_UNLOCK_SCOPE(cx, scope);

Maybe blank line after brace and before JS_UNLOCK_SCOPE call.

>+
>+        uint32 slot = globalScope->globalFreeSlot + globalScope->defs.length();
>+        if (!globalScope->defs.append(pn->pn_atom))
>+            return false;
>+
>+        uint32 index;
>+        if (!cg->addGlobalUse(pn->pn_atom, slot, &index))
>+            return false;
>+
>+        if (index != FREE_UPVAR_COOKIE) {
>+            pn->pn_op = JSOP_GETGLOBAL;
>+            pn->pn_cookie = index;
>             pn->pn_dflags |= PND_BOUND | PND_GVAR;

Need inWith deoptimization still.

>         }
>+
>+        return true;
>+    }
>+
>+    /* Index pn->pn_atom so we can map fast global number to name. */
>+    JSAtomListElement *ale = cg->atomList.add(tc->parser, pn->pn_atom);
>+    if (!ale)
>+        return false;
>+
>+    /* Defend against cg->ngvars 16-bit overflow. */
>+    uintN slot = ALE_INDEX(ale);
>+    if ((slot + 1) >> 16)
>+        return true;
>+
>+    if ((uint16)(slot + 1) > cg->ngvars)
>+        cg->ngvars = (uint16)(slot + 1);
>+
>+    pn->pn_op = JSOP_GETGVAR;
>+    pn->pn_cookie = MAKE_UPVAR_COOKIE(tc->staticLevel, slot);
>+    pn->pn_dflags |= PND_BOUND | PND_GVAR;

And here. Why remove inWith?

>@@ -3303,7 +3403,7 @@ BindVarOrConst(JSContext *cx, BindData *
> 
>     if (stmt && stmt->type == STMT_WITH) {
>         data->fresh = false;
>-        return tc->inFunction() || BindGvar(pn, tc, true);
>+        return true;

This was a bugfix. See bug 561011.

>@@ -7085,6 +7190,13 @@ Parser::memberExpr(JSBool allowCallSynta
>                     /* Select JSOP_EVAL and flag tc as heavyweight. */
>                     pn2->pn_op = JSOP_EVAL;
>                     tc->flags |= TCF_FUN_HEAVYWEIGHT;
>+
>+                    /*
>+                     * If inside a function, free variable references cannot
>+                     * be guaranteed to reach the global scope.
>+                     */
>+                    if (tc->inFunction())
>+                        tc->flags |= TCF_SCOPE_INJECTED;

This is not true. What's the issue? We have def-use chains, we know the free variable refs and heavyweight functions. Apart from non-strict eval and with, nothing can shadow.

/be

Comment 38

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

(In reply to comment #37)

> You want inWith here still.
> Need inWith deoptimization still.
>
>-        return tc->inFunction() || BindGvar(pn, tc, true);
>+        return true;
>
> And here. Why remove inWith?

Okay, I see why this is necessary now: it bumps ngvars. This test case passes with my patch because DEFVAR isn't emitted, but it would fail again without compile-and-go. Thanks for catching this.

> The tc->parser->callerFrame test already covered !cg->compilingForEval().

Except global/indirect eval(), where callerFrame is NULL but |var| wants to be configurable. This is JSFRAME_EVAL for the compiler.

> This is not true. What's the issue?

Heavyweight is too heavy, but the def-use chains don't correctly de-optimize for "injected" scopes. A free-variable reference will always bind to a global var declaration if one exists and all else fails; after talking IRL I will fix this instead.

Comment 39

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

Fixing the def-use chains is harder than I anticipated. As discussed IRL the idea was to detect polluted scopes in LeaveFunction() and deoptimize unresolved lexical dependencies. That breaks this example:

 function f() {
  with ({})
    let (x)
     (function () { return x; })()
 }

One solution might be to delay the |with| deoptimization until the end of the |with| scope, rather than in LeaveFunction, and then only deoptimizing the newly added lexdeps. That solves my use case, but our def-use chains are still wrong here:

 function f() {
  var x;
  with ({})
   (function () { return x; })()
 }

The use of |x| incorrectly binds to the definition at the top of |f|. Of course, since |f| is heavyweight, we'll emit NAME "x" and it won't matter. This bug is probably orthogonal.

Given how fuzzy our def-use chains are in the presence of |with|, it seems like there are a few ways to proceed with this patch:

1. Go back to TCF_SCOPE_INJECTED, the quick hack to detect invalid free variable def-use chains in the emitter.
2. Deoptimize in LeaveFunction if the funtc used eval. Add a TCF_FUN_DECLARED_IN_WITH and keep a more restricted version of the hack.
3. Do the full fix of def-use chains for globals, leave the other problems for another bug.

Comment 40

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

Note that all three of these should be equally accurate & precise for statically binding globals.

Comment 41

[Brendan Eich [:brendan]]

(In reply to comment #39)
> One solution might be to delay the |with| deoptimization until the end of the
> |with| scope, rather than in LeaveFunction, and then only deoptimizing the
> newly added lexdeps. That solves my use case, but our def-use chains are still
> wrong here:
> 
>  function f() {
>   var x;
>   with ({})
>    (function () { return x; })()
>  }
> 
> The use of |x| incorrectly binds to the definition at the top of |f|. Of
> course, since |f| is heavyweight, we'll emit NAME "x" and it won't matter. This
> bug is probably orthogonal.

This is not a bug, it's a "works as designed". It's suboptimal but in precisely the way that you want to fix: we shouldn't give up on name optimizations just due to with inducing heavyweight in its containing function.

The right thing is door #3.

/be
> 
> Given how fuzzy our def-use chains are in the presence of |with|, it seems like
> there are a few ways to proceed with this patch:
> 
> 1. Go back to TCF_SCOPE_INJECTED, the quick hack to detect invalid free
> variable def-use chains in the emitter.
> 2. Deoptimize in LeaveFunction if the funtc used eval. Add a
> TCF_FUN_DECLARED_IN_WITH and keep a more restricted version of the hack.
> 3. Do the full fix of def-use chains for globals, leave the other problems for
> another bug.

Comment 42

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

> This is not a bug, it's a "works as designed". It's suboptimal

Given that this is design, do we want to "correct" the use-def chains and not propagate polluted lexdeps, or keep the propagation and just mark as deoptimized?

Comment 43

[Brendan Eich [:brendan]]

Simpler to keep the propagation and mark as deoptimized. Factor DeoptimizeUsesWithin(JSDefinition *dn, JSFunctionBox *funbox, uint32& tcflags) into DeoptimizeUsesWithin(JSDefinition *dn, const JSTokenPos &pos) and call that with the with statement's pn_pos and the dn in question.

/be

Comment 44

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

 var a = 3;
 with ({a: 2}) {
   function f() {
     return a;   // <--- 
   }
   print(a);
 }

this is a global version of the second example in comment #39, so the marked use is not in the global tc's lexdeps at the bottom of the |with| statement.

Comment 45

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

LeaveFunction's tc->decls lookup needs to know not to reach past a |with| scope. maybe associating a parse node with the JSStmtInfo is enough.

Comment 46

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

Attachment: part 3: deoptimize uses inside polluted scopes

Comment 47

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

Attachment: part 4: static binding

Comment 48

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

Attachment: part 4: static binding

refactored slightly to make bug 562729 easier.

Comment 49

[Brendan Eich [:brendan]]

Comment on attachment 444007 [details] [diff] [review]
part 3: deoptimize uses inside polluted scopes


>@@ -2292,7 +2290,8 @@ Parser::setFunctionKinds(JSFunctionBox *
>                             ++nflattened;
>                             continue;
>                         }
>-                        DeoptimizeUsesWithin(lexdep, funbox, tcflags);
>+                        if (DeoptimizeUsesWithin(lexdep, funbox->node->pn_body->pn_pos) > 0)

DeoptimizeUsesWithin returns unsigned so use != 0 not > 0 (style nit). Or change it to return bool (ndeoptimized != 0), I think better.

>@@ -2466,6 +2465,17 @@ LeaveFunction(JSParseNode *fn, JSTreeCon
>             }
> 
>             JSAtomListElement *outer_ale = tc->decls.lookup(atom);
>+
>+            /*
>+             * Make sure to deoptimize lexical dependencies that are polluted
>+             * by eval or with, to safely statically bind globals (see bug 561923).
>+             */
>+            if ((funtc->flags & TCF_FUN_USES_EVAL) ||
>+                (outer_ale && tc->innermostWith &&
>+                 ALE_DEFN(outer_ale)->pn_pos < tc->innermostWith->pn_pos)) {

The pn_pos < comparison doesn't handle cases such as this:

function outer(x) {
  if (tuesday) {
    with (x) {
      function inner() { return y; }
    }
  } else {
    var y = 42;
  }
}

because !outer_ale (we haven't parsed the var y = 42; yet).

This example shows that function in 'with' is our "function statement" extension to ECMA-262, so perhaps not worth worrying about?

If we do worry about it, I suggest just dropping this third && clause (ALE_DEFN(outer_ale)->pn_pos < ...)). Unless the outer_dn and tc->innermostWith are within the same block (same blockid), you can't compare source positions to determine dominance or anything like that.

/be

Comment 50

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

(In reply to comment #49)

> change it to return bool (ndeoptimized != 0), I think better.

Okay.

> The pn_pos < comparison doesn't handle cases such as this:
> ...
> This example shows that function in 'with' is our "function statement"

This case will be okay because we also deoptimize when leaving the with statement. So I think the third clause is safe, removing it would deoptimize this (which we probably also don't care about):

 with ({}) {
   var y;
   (function () { return y; })();
 }

Comment 51

[Brendan Eich [:brendan]]

(In reply to comment #50)
> (In reply to comment #49)
> 
> > change it to return bool (ndeoptimized != 0), I think better.
> 
> Okay.
> 
> > The pn_pos < comparison doesn't handle cases such as this:
> > ...
> > This example shows that function in 'with' is our "function statement"
> 
> This case will be okay because we also deoptimize when leaving the with
> statement. So I think the third clause is safe, removing it would deoptimize
> this (which we probably also don't care about):
> 
>  with ({}) {
>    var y;
>    (function () { return y; })();
>  }

Yes, but that is within the same block (braced with body). You could check for matching pn_blockid, but then when the var was in a different block you might misjudge.

Since var hoists, there's little benefit in trying to compute dominance. (Sanity check for let, write a testcase trying to break your patch.)

/be

Comment 52

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

(In reply to comment #51)

Whoops, yes, I meant:

 with ({}) {
   let y;
   (function () { return y; })();
 }

Which would be suboptimal without the third clause (even though we'll still emit NAME).

re: Let. I wrote a bunch of tests based on what I thought the semantics were, but one breaks regardless of my patch.

 function f() {
   var x = 3;
   {
     print(x);
     let x = 2;
   }
 }

This prints "undefined", which suggests the definition of |x| is hoisted to the top of the inner block. But when |with| is involved, regardless of my patch:

 function f() {
   with ({x: 3}) {
     print(x);
     let x = 2;
   }
 }

This prints "3", which makes sense given that js_LexicalLookup won't create use-def chains inside the |with|. But it doesn't seem consistent with the earlier example.

Well, even with that, the semantics don't change before and after this patch. Will attach the test cases.

Comment 53

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

Attachment: part 3: deoptimize uses inside polluted scopes

Changed DeoptimizeUsesWithin to return bool, added test cases.

Please double-check tests 2 and 4 to make sure I've understood the semantics correctly. They fail on tm-tip and pass with this patch.

Comment 54

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

Attachment: part 2: don't resolve for constant lookups in the parser

Turns out calling sprop->get() is just wrong, wasn't hitting in TM because an earlier cleanup patch of mine accidentally changed the condition in LookupCompileTimeConstants().

Re-requesting review, will land this one early.

Comment 55

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

Attachment: part 4: static binding

rebased JSOP_FORGLOBAL for iterator changes

Comment 56

[Brendan Eich [:brendan]]

(In reply to comment #52)
> (In reply to comment #51)
> 
> Whoops, yes, I meant:
> 
>  with ({}) {
>    let y;
>    (function () { return y; })();
>  }
> 
> Which would be suboptimal without the third clause (even though we'll still
> emit NAME).

We should not emit NAME here -- the with is above the let y's block.

This one you could handle by checking whether outer_dn && outer_dn->pn_pos is within with_body->pn_pos. Add a contains method on TokenPos.

>  function f() {
>    with ({x: 3}) {
>      print(x);
>      let x = 2;
>    }
>  }
> 
> This prints "3", which makes sense given that js_LexicalLookup won't create
> use-def chains inside the |with|. But it doesn't seem consistent with the
> earlier example.

Looks like a bug -- mrbkap should comment.

/be

Comment 57

[Brendan Eich [:brendan]]

Comment on attachment 444594 [details] [diff] [review]
part 3: deoptimize uses inside polluted scopes

>@@ -5355,7 +5369,21 @@ Parser::statement()
>         pn->pn_pos.end = pn2->pn_pos.end;
>         pn->pn_right = pn2;
>         tc->flags |= TCF_FUN_HEAVYWEIGHT;
>+        tc->innermostWith = oldWith;
>+
>+        /*
>+         * Make sure to deoptimize lexical dependencies inside the |with|
>+         * to safely optimize binding globals (see bug 561923).
>+         */
>+        JSAtomListIterator iter(&tc->lexdeps);
>+        JSAtomListElement *ale;
>+        while ((ale = iter()) != NULL) {
>+            JSDefinition *lexdep = ALE_DEFN(ale)->resolve();
>+            DeoptimizeUsesWithin(lexdep, pn->pn_pos);
>+        }

Looks like you can scope JSAtomListElement *ale to the while loop for C++ l33tness.

Good tests -- wonder when -2 and -4 broke, or whether they ever worked.

/be

Comment 58

[Brendan Eich [:brendan]]

Comment on attachment 444794 [details] [diff] [review]
part 4: static binding

>+    GlobalSlotArray::Entry entry;
>+    entry.atomIndex = ALE_INDEX(ale);
>+    entry.slot = slot;

Small-world codebase, could just initialize entry = {...}, right?

>+    bool compilingForEval() { return !!(flags & TCF_COMPILE_FOR_EVAL); }

Isn't the !! unnecessary here?

>+BEGIN_CASE(JSOP_FORGLOBAL)
>+{
>+    JS_ASSERT(regs.sp - 1 >= StackBase(fp));
>+    JS_ASSERT(!JSVAL_IS_PRIMITIVE(regs.sp[-1]));
>+    AutoValueRooter tvr(cx);
>+    if (!IteratorNext(cx, JSVAL_TO_OBJECT(regs.sp[-1]), tvr.addr()))

Avoid the tvr by using JOF_TMPSLOT, so you can use &regs.sp[0] here (and lose the braces).

>+    if (globalScope.defs.length()) {
>+        JS_ASSERT(globalObj->scope()->freeslot == globalScope.globalFreeSlot);
>+        for (size_t i = 0; i < globalScope.defs.length(); i++) {
>+            JSAtom *atom = globalScope.defs[i];
>+            jsid id = ATOM_TO_JSID(atom);
>+            JSProperty *prop;
>+
>+            if (!js_DefineNativeProperty(cx, globalObj, id, JSVAL_VOID, JS_PropertyStub,
>+                                         JS_PropertyStub, JSPROP_ENUMERATE|JSPROP_PERMANENT,

Nit: space on each side of |.

Non-nit: this runs for eval code too, IIRC, but eval-created vars are not JSPROP_PERMANENT.

Aside: in ES5 strict mode they're not created in the caller's scope, but I guess the bug to implement this aspect of ES5 strict mode hasn't been fixed yet.

>+                                         0, 0, &prop)) {
>+                goto out;
>+            }
>+
>+            JS_ASSERT(prop);
>+            JS_ASSERT(((JSScopeProperty*)prop)->slot == globalScope.globalFreeSlot + i);

Need to globalObj->dropProperty(cx, prop) here.

/be

Comment 59

[Brendan Eich [:brendan]]

New part 4 patch coming?

/be

Comment 60

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

Attachment: part 4: static binding

> Isn't the !! unnecessary here?

Windows doesn't like coercing integers to bools.

> Non-nit: this runs for eval code too, IIRC, but eval-created vars are not
JSPROP_PERMANENT.

This path is not reachable for eval code, so I put an assert to clarify.

Comment 61

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

Attachment: part 4: static binding

whoops, attached wrong patch

Comment 63

[Brendan Eich [:brendan]]

Comment on attachment 445166 [details] [diff] [review]
part 4: static binding

Looks like all the uses of SystemAllocPolicy should be ContextAllocPolicy -- silent failure on OOM is no fun. r=me with those fixed.

/be

Comment 64

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

Attachment: part 5: fix jsd to use JSCLASS_GLOBAL_FLAGS

discovered by dmandelin on JM branch, JSD does not use JSCLASS_GLOBAL_FLAGS. not sure who can review this?

Comment 65

[Robert Sayre]

These patches were checked in, but they don't compile on any platform. I maintain this is a serious defect. :P

Comment 66

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

So after spot-fixing that, a few columns went from green to orange. I'll back out each part until it's green again.

Comment 67

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2cdc68e84ee4

Comment 68

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

only landed partially

Comment 69

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

Landed on JM a long time ago.