562729 - JM: statically bind global functions where possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Same idea as bug 561923, but separating out for sanity. This code:

function f() { }
f();
print(f);

This results in "CALLNAME" and "NAME". We would like this to be CALLGLOBAL ; GETGLOBAL or at the very least the unbound variants that are easy to inline cache.

Comment 1

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

Attachment: patch

passes js & reftests

Comment 2

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

Early push to JM: http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/5a9a35350982

Comment 3

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

Attachment: rebased

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 445169 [details] [diff] [review]
rebased

>+    /*
>+     * Previously this would be JSOP_NAME in some cases, which would push a
>+     * global object (see bug 555246 for more details). Now we're pushing
>+     * NULL instead, which creates more trace instability. As a quick hack
>+     * until bug 556277 fixes all |this| problems, do a best-case test to see
>+     * if we can push a global object.

Use the FIXME: bug 556277 convention here and in jstracer.cpp for the parallel code, and please do make sure this all goes away.

>+    if (JSScopeProperty *sprop = scope->lookup(ATOM_TO_JSID(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() &&
>+            pn->pn_type != TOK_FUNCTION &&
>+            cg->addGlobalUse(atom, sprop->slot, &index) &&

Per previous review on ContextAllocPolicy (needed elsewhere for OOM reporting), don't include this in the && chain, instead use an if in the braced then below, and fail immediately if addGlobalUse fails.

>+            index != FREE_UPVAR_COOKIE)

But fail soft if index == FREE_UPVAR_COOKIE.

>+        {
>+            pn->pn_cookie = index;
>+            pn->pn_dflags |= PND_GVAR | PND_BOUND;
>+            pn->pn_op = JSOP_GETGLOBAL;
>+        }
>+
>+        JS_UNLOCK_SCOPE(cx, scope);

Don't forget to unlock before failing immediately on OOM.

>@@ -2989,6 +3086,12 @@ Parser::functionDef(uintN lambda, bool n
>         pn->pn_body = body;
>     }
> 
>+    if (!outertc->inFunction() && topLevel && funAtom && !lambda && outertc->compiling()) {

Nit: outertc->compiling() && !outertc->inFunction(), reads a bit better. Wonder if gcc can optimize into one mask and test?

/be

Comment 5

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

This landed on JM a long time ago.