Bug 669369 - Simplify Parser::setFunctionKinds. r=dmandelin.
authorJason Orendorff <jorendorff@mozilla.com>
Tue, 30 Aug 2011 06:08:30 -0500
changeset 76231 77c5e531f7cbcde17e8a5096ced366fc8d84b0bc
parent 76230 30632635b7fb19012782740b5ee068857950bac3
child 76232 efcd1318cf4d48fc5544a1a84f928d826bea2461
push id3
push userfelipc@gmail.com
push dateFri, 30 Sep 2011 20:09:13 +0000
reviewersdmandelin
bugs669369
milestone9.0a1
Bug 669369 - Simplify Parser::setFunctionKinds. r=dmandelin.
js/src/jsparse.cpp
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -2267,18 +2267,19 @@ CanFlattenUpvar(JSDefinition *dn, JSFunc
          *
          * Assert but check anyway, to protect future changes that bind eval
          * upvars in the parser.
          */
         JS_ASSERT(afunbox);
 
         /*
          * If this function is reaching up across an enclosing funarg, then we
-         * cannot copy dn's value into a flat closure slot (the display stops
-         * working once the funarg escapes).
+         * cannot copy dn's value into a flat closure slot. The flat closure
+         * code assumes the upvars to be copied are in frames still on the
+         * stack.
          */
         if (!afunbox || afunbox->node->isFunArg())
             return false;
 
         /*
          * Reaching up for dn across a generator also means we can't flatten,
          * since the generator iterator does not run until later, in general.
          * See bug 563034.
@@ -2407,172 +2408,127 @@ DeoptimizeUsesWithin(JSDefinition *dn, c
             pnu->pn_dflags |= PND_DEOPTIMIZED;
             ++ndeoptimized;
         }
     }
 
     return ndeoptimized != 0;
 }
 
+static void
+ConsiderUnbranding(JSFunctionBox *funbox)
+{
+    /*
+     * We've already recursively set our kids' kinds, which also classifies
+     * enclosing functions holding upvars referenced in those descendants'
+     * bodies. So now we can check our "methods".
+     *
+     * Despecialize from branded method-identity-based shape to shape- or
+     * slot-based shape if this function smells like a constructor and too many
+     * of its methods are *not* joinable null closures (i.e., they have one or
+     * more upvars fetched via the display).
+     */
+    bool returnsExpr = !!(funbox->tcflags & TCF_RETURN_EXPR);
+#if JS_HAS_EXPR_CLOSURES
+    {
+        JSParseNode *pn2 = funbox->node->pn_body;
+        if (PN_TYPE(pn2) == TOK_UPVARS)
+            pn2 = pn2->pn_tree;
+        if (PN_TYPE(pn2) == TOK_ARGSBODY)
+            pn2 = pn2->last();
+        if (PN_TYPE(pn2) != TOK_LC)
+            returnsExpr = true;
+    }
+#endif
+    if (!returnsExpr) {
+        uintN methodSets = 0, slowMethodSets = 0;
+
+        for (JSParseNode *method = funbox->methods; method; method = method->pn_link) {
+            JS_ASSERT(PN_OP(method) == JSOP_LAMBDA || PN_OP(method) == JSOP_LAMBDA_FC);
+            ++methodSets;
+            if (!method->pn_funbox->joinable())
+                ++slowMethodSets;
+        }
+
+        if (funbox->shouldUnbrand(methodSets, slowMethodSets))
+            funbox->tcflags |= TCF_FUN_UNBRAND_THIS;
+    }
+}
+
 void
 Parser::setFunctionKinds(JSFunctionBox *funbox, uint32 *tcflags)
 {
-    for (;;) {
+    for (; funbox; funbox = funbox->siblings) {
         JSParseNode *fn = funbox->node;
         JSParseNode *pn = fn->pn_body;
 
         if (funbox->kids) {
             setFunctionKinds(funbox->kids, tcflags);
-
-            /*
-             * We've unwound from recursively setting our kids' kinds, which
-             * also classifies enclosing functions holding upvars referenced in
-             * those descendants' bodies. So now we can check our "methods".
-             *
-             * Despecialize from branded method-identity-based shape to shape-
-             * or slot-based shape if this function smells like a constructor
-             * and too many of its methods are *not* joinable null closures
-             * (i.e., they have one or more upvars fetched via the display).
-             */
-            JSParseNode *pn2 = pn;
-            if (PN_TYPE(pn2) == TOK_UPVARS)
-                pn2 = pn2->pn_tree;
-            if (PN_TYPE(pn2) == TOK_ARGSBODY)
-                pn2 = pn2->last();
-
-#if JS_HAS_EXPR_CLOSURES
-            if (PN_TYPE(pn2) == TOK_LC)
-#endif
-            if (!(funbox->tcflags & TCF_RETURN_EXPR)) {
-                uintN methodSets = 0, slowMethodSets = 0;
-
-                for (JSParseNode *method = funbox->methods; method; method = method->pn_link) {
-                    JS_ASSERT(PN_OP(method) == JSOP_LAMBDA || PN_OP(method) == JSOP_LAMBDA_FC);
-                    ++methodSets;
-                    if (!method->pn_funbox->joinable())
-                        ++slowMethodSets;
-                }
-
-                if (funbox->shouldUnbrand(methodSets, slowMethodSets))
-                    funbox->tcflags |= TCF_FUN_UNBRAND_THIS;
-            }
+            ConsiderUnbranding(funbox);
         }
 
         JSFunction *fun = funbox->function();
 
         JS_ASSERT(fun->kind() == JSFUN_INTERPRETED);
 
         if (funbox->tcflags & TCF_FUN_HEAVYWEIGHT) {
             /* nothing to do */
         } else if (funbox->inAnyDynamicScope()) {
             JS_ASSERT(!fun->isNullClosure());
-        } else if (pn->pn_type != TOK_UPVARS) {
-            /*
-             * No lexical dependencies => null closure, for best performance.
-             * A null closure needs no scope chain, but alas we've coupled
-             * principals-finding to scope (for good fundamental reasons, but
-             * the implementation overloads the parent slot and we should fix
-             * that). See, e.g., the JSOP_LAMBDA case in jsinterp.cpp.
-             *
-             * In more detail: the ES3 spec allows the implementation to create
-             * "joined function objects", or not, at its discretion. But real-
-             * world implementations always create unique function objects for
-             * closures, and this can be detected via mutation. Open question:
-             * do popular implementations create unique function objects for
-             * null closures?
-             *
-             * FIXME: bug 476950.
-             */
-            fun->setKind(JSFUN_NULL_CLOSURE);
         } else {
-            AtomDefnMapPtr upvars = pn->pn_names;
-            JS_ASSERT(!upvars->empty());
-
-            if (!fn->isFunArg()) {
-                /*
-                 * This function is Algol-like, it never escapes.
-                 *
-                 * Check that at least one outer lexical binding was assigned
-                 * to (global variables don't count). This is conservative: we
-                 * could limit assignments to those in the current function,
-                 * but that's too much work. As with flat closures (handled
-                 * below), we optimize for the case where outer bindings are
-                 * not reassigned anywhere.
-                 */
-                AtomDefnRange r = upvars->all();
-                for (; !r.empty(); r.popFront()) {
-                    JSDefinition *defn = r.front().value();
-                    JSDefinition *lexdep = defn->resolve();
-
-                    if (!lexdep->isFreeVar()) {
-                        JS_ASSERT(lexdep->frameLevel() <= funbox->level);
-                        break;
-                    }
-                }
-
-                if (r.empty())
-                    fun->setKind(JSFUN_NULL_CLOSURE);
-            } else {
-                uintN nupvars = 0, nflattened = 0;
+            uintN hasUpvars = false;
+            bool canFlatten = true;
+
+            if (pn->pn_type == TOK_UPVARS) {
+                AtomDefnMapPtr upvars = pn->pn_names;
+                JS_ASSERT(!upvars->empty());
 
                 /*
                  * For each lexical dependency from this closure to an outer
                  * binding, analyze whether it is safe to copy the binding's
                  * value into a flat closure slot when the closure is formed.
                  */
                 for (AtomDefnRange r = upvars->all(); !r.empty(); r.popFront()) {
                     JSDefinition *defn = r.front().value();
                     JSDefinition *lexdep = defn->resolve();
 
                     if (!lexdep->isFreeVar()) {
-                        ++nupvars;
-                        if (CanFlattenUpvar(lexdep, funbox, *tcflags)) {
-                            ++nflattened;
-                            continue;
+                        hasUpvars = true;
+                        if (!CanFlattenUpvar(lexdep, funbox, *tcflags)) {
+                            /*
+                             * Can't flatten. Enclosing functions holding
+                             * variables used by this function will be flagged
+                             * heavyweight below. FIXME bug 545759: re-enable
+                             * partial flat closures.
+                             */
+                            canFlatten = false;
+                            break;
                         }
-
-                        /*
-                         * FIXME bug 545759: to test nflattened != 0 instead of
-                         * nflattened == nupvars below, we'll need to avoid n^2
-                         * bugs such as 617430 in uncommenting the following:
-                         *
-                         * if (DeoptimizeUsesWithin(lexdep, funbox->node->pn_body->pn_pos))
-                         *     FlagHeavyweights(lexdep, funbox, tcflags);
-                         *
-                         * For now it's best to avoid this tedious, use-wise
-                         * deoptimization and let fun remain an unoptimized
-                         * closure. This is safe because we leave fun's kind
-                         * set to interpreted, so all functions holding its
-                         * upvars will be flagged as heavyweight.
-                         */
                     }
                 }
-
-                if (nupvars == 0) {
-                    fun->setKind(JSFUN_NULL_CLOSURE);
-                } else if (nflattened == nupvars) {
-                    /*
-                     * We made it all the way through the upvar loop, so it's
-                     * safe to optimize to a flat closure.
-                     */
-                    fun->setKind(JSFUN_FLAT_CLOSURE);
-                    switch (PN_OP(fn)) {
-                      case JSOP_DEFFUN:
-                        fn->pn_op = JSOP_DEFFUN_FC;
-                        break;
-                      case JSOP_DEFLOCALFUN:
-                        fn->pn_op = JSOP_DEFLOCALFUN_FC;
-                        break;
-                      case JSOP_LAMBDA:
-                        fn->pn_op = JSOP_LAMBDA_FC;
-                        break;
-                      default:
-                        /* js_EmitTree's case TOK_FUNCTION: will select op. */
-                        JS_ASSERT(PN_OP(fn) == JSOP_NOP);
-                    }
+            }
+
+            if (!hasUpvars) {
+                /* No lexical dependencies => null closure, for best performance. */
+                fun->setKind(JSFUN_NULL_CLOSURE);
+            } else if (canFlatten) {
+                fun->setKind(JSFUN_FLAT_CLOSURE);
+                switch (PN_OP(fn)) {
+                case JSOP_DEFFUN:
+                    fn->pn_op = JSOP_DEFFUN_FC;
+                    break;
+                case JSOP_DEFLOCALFUN:
+                    fn->pn_op = JSOP_DEFLOCALFUN_FC;
+                    break;
+                case JSOP_LAMBDA:
+                    fn->pn_op = JSOP_LAMBDA_FC;
+                    break;
+                default:
+                    /* js_EmitTree's case TOK_FUNCTION: will select op. */
+                    JS_ASSERT(PN_OP(fn) == JSOP_NOP);
                 }
             }
         }
 
         if (fun->kind() == JSFUN_INTERPRETED && pn->pn_type == TOK_UPVARS) {
             /*
              * One or more upvars cannot be safely snapshot into a flat
              * closure's non-reserved slot (see JSOP_GETFCSLOT), so we loop
@@ -2590,20 +2546,16 @@ Parser::setFunctionKinds(JSFunctionBox *
                 JSDefinition *lexdep = defn->resolve();
                 if (!lexdep->isFreeVar())
                     FlagHeavyweights(lexdep, funbox, tcflags);
             }
         }
 
         if (funbox->joinable())
             fun->setJoinable();
-
-        funbox = funbox->siblings;
-        if (!funbox)
-            break;
     }
 }
 
 /*
  * Walk the JSFunctionBox tree looking for functions whose call objects may
  * acquire new bindings as they execute: non-strict functions that call eval,
  * and functions that contain function statements (definitions not appearing
  * within the top statement list, which don't take effect unless they are