Closed Bug 562729 Opened 12 years ago Closed 11 years ago

JM: statically bind global functions where possible

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: general → dvander
Attached patch patch (obsolete) — Splinter Review
passes js & reftests
Attached patch rebasedSplinter Review
Attachment #444473 - Attachment is obsolete: true
Attachment #445169 - Flags: review?(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
Attachment #445169 - Flags: review?(brendan) → review+
This landed on JM a long time ago.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.