Bug 739808: remove null closure no-clone optimization, r=luke, a=akeybl
authorDavid Mandelin <dmandelin@mozilla.com
Mon, 14 May 2012 17:07:08 -0700
changeset 81840 6cf09fa77444
parent 81839 d81a789ad0cd
child 81841 f10a1ef4d89c
child 81843 dd21a1df7b1e
child 81844 5713c92407dd
push id158
push userdmandelin@mozilla.com
push dateThu, 31 May 2012 01:34:50 +0000
reviewersluke, akeybl
bugs739808
milestone10.0.5esrpre
Bug 739808: remove null closure no-clone optimization, r=luke, a=akeybl
js/src/frontend/BytecodeEmitter.cpp
js/src/frontend/SemanticAnalysis.cpp
js/src/jit-test/tests/basic/testInitDictionary.js
js/src/jsfun.h
js/src/jsfuninlines.h
js/src/jsinterp.cpp
js/src/methodjit/Compiler.cpp
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -6332,31 +6332,16 @@ frontend::EmitTree(JSContext *cx, Byteco
 #if JS_HAS_DESTRUCTURING
                 if (!wantval &&
                     pn2->isKind(TOK_ASSIGN) &&
                     !MaybeEmitGroupAssignment(cx, bce, op, pn2, &op)) {
                     return JS_FALSE;
                 }
 #endif
                 if (op != JSOP_NOP) {
-                    /*
-                     * Specialize JSOP_SETPROP to JSOP_SETMETHOD to defer or
-                     * avoid null closure cloning. Do this only for assignment
-                     * statements that are not completion values wanted by a
-                     * script evaluator, to ensure that the joined function
-                     * can't escape directly.
-                     */
-                    if (!wantval &&
-                        pn2->isKind(TOK_ASSIGN) &&
-                        pn2->isOp(JSOP_NOP) &&
-                        pn2->pn_left->isOp(JSOP_SETPROP) &&
-                        pn2->pn_right->isOp(JSOP_LAMBDA) &&
-                        pn2->pn_right->pn_funbox->joinable()) {
-                        pn2->pn_left->setOp(JSOP_SETMETHOD);
-                    }
                     if (!EmitTree(cx, bce, pn2))
                         return JS_FALSE;
                     if (Emit1(cx, bce, op) < 0)
                         return JS_FALSE;
                 }
             }
         }
         break;
@@ -7102,36 +7087,29 @@ frontend::EmitTree(JSContext *cx, Byteco
                     return JS_FALSE;
             } else {
                 JS_ASSERT(pn3->isKind(TOK_NAME) ||
                           pn3->isKind(TOK_STRING));
                 jsatomid index;
                 if (!bce->makeAtomIndex(pn3->pn_atom, &index))
                     return JS_FALSE;
 
-                /* Check whether we can optimize to JSOP_INITMETHOD. */
                 ParseNode *init = pn2->pn_right;
                 bool lambda = init->isOp(JSOP_LAMBDA);
-                if (lambda)
+                if (lambda) {
                     ++methodInits;
-                if (op == JSOP_INITPROP && lambda && init->pn_funbox->joinable()) {
+                    ++slowMethodInits;
+                }
+                /*
+                 * Disable NEWOBJECT on initializers that set __proto__, which has
+                 * a non-standard setter on objects.
+                 */
+                if (pn3->pn_atom == cx->runtime->atomState.protoAtom)
                     obj = NULL;
-                    op = JSOP_INITMETHOD;
-                    pn2->setOp(op);
-                } else {
-                    /*
-                     * Disable NEWOBJECT on initializers that set __proto__, which has
-                     * a non-standard setter on objects.
-                     */
-                    if (pn3->pn_atom == cx->runtime->atomState.protoAtom)
-                        obj = NULL;
-                    op = JSOP_INITPROP;
-                    if (lambda)
-                        ++slowMethodInits;
-                }
+                op = JSOP_INITPROP;
 
                 if (obj) {
                     JS_ASSERT(!obj->inDictionaryMode());
                     if (!DefineNativeProperty(cx, obj, ATOM_TO_JSID(pn3->pn_atom),
                                               UndefinedValue(), NULL, NULL,
                                               JSPROP_ENUMERATE, 0, 0)) {
                         return false;
                     }
--- a/js/src/frontend/SemanticAnalysis.cpp
+++ b/js/src/frontend/SemanticAnalysis.cpp
@@ -651,19 +651,16 @@ SetFunctionKinds(FunctionBox *funbox, ui
 
             for (AtomDefnRange r = upvars->all(); !r.empty(); r.popFront()) {
                 Definition *defn = r.front().value();
                 Definition *lexdep = defn->resolve();
                 if (!lexdep->isFreeVar())
                     FlagHeavyweights(lexdep, funbox, tcflags);
             }
         }
-
-        if (funbox->joinable())
-            fun->setJoinable();
     }
 }
 
 /*
  * Walk the FunctionBox 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
deleted file mode 100644
--- a/js/src/jit-test/tests/basic/testInitDictionary.js
+++ /dev/null
@@ -1,51 +0,0 @@
-
-var shapes = {};
-
-function stringify(a) {
-  assertEq(shapes[shapeOf(a)], undefined);
-  shapes[shapeOf(a)] = 1;
-  var b = "";
-  for (var c in a) {
-    b += c + ":";
-    if (typeof a[c] == "function")
-      b += "function,";
-    else
-      b += a[c] + ",";
-  }
-  return b;
-}
-
-function test1() {
-  return stringify({a: 0, b: 1, a: function() {} });
-}
-for (var i = 0; i < 3; i++)
-  assertEq(test1(), "a:function,b:1,");
-
-// This does not cause the object to go to dictionary mode, unlike the above.
-function test2() {
-  return stringify({a: 0, b: 1, a: 2, b: 3});
-}
-assertEq(test2(), "a:2,b:3,");
-
-function test3() {
-  return stringify({
-    aa:0,ab:1,ac:2,ad:3,ae:4,af:5,ag:6,ah:7,ai:8,aj:9,
-    ba:0,bb:1,bc:2,bd:3,be:4,bf:5,bg:6,bh:7,bi:8,bj:9,
-    ca:0,cb:1,cc:2,cd:3,ce:4,cf:5,cg:6,ch:7,ci:8,cj:9,
-    da:0,db:1,dc:2,dd:3,de:4,df:5,dg:6,dh:7,di:8,dj:9,
-    ea:0,eb:1,ec:2,ed:3,ee:4,ef:5,eg:6,eh:7,ei:8,ej:9,
-    fa:0,fb:1,fc:2,fd:3,fe:4,ff:5,fg:6,fh:7,fi:8,fj:9,
-    ga:0,gb:1,gc:2,gd:3,ge:4,gf:5,gg:6,gh:7,gi:8,gj:9,
-    ha:0,hb:1,hc:2,hd:3,he:4,hf:5,hg:6,hh:7,hi:8,hj:9,
-    ia:0,ib:1,ic:2,id:3,ie:4,if:5,ig:6,ih:7,ii:8,ij:9,
-    ja:0,jb:1,jc:2,jd:3,je:4,jf:5,jg:6,jh:7,ji:8,jj:9,
-    ka:0,kb:1,kc:2,kd:3,ke:4,kf:5,kg:6,kh:7,ki:8,kj:9,
-    la:0,lb:1,lc:2,ld:3,le:4,lf:5,lg:6,lh:7,li:8,lj:9,
-    ma:0,mb:1,mc:2,md:3,me:4,mf:5,mg:6,mh:7,mi:8,mj:9,
-    na:0,nb:1,nc:2,nd:3,ne:4,nf:5,ng:6,nh:7,ni:8,nj:9,
-    oa:0,ob:1,oc:2,od:3,oe:4,of:5,og:6,oh:7,oi:8,oj:9,
-    pa:0,pb:1,pc:2,pd:3,pe:4,pf:5,pg:6,ph:7,pi:8,pj:9,
-        });
-}
-for (var i = 0; i < HOTLOOP + 2; i++)
-  assertEq(test3(), "aa:0,ab:1,ac:2,ad:3,ae:4,af:5,ag:6,ah:7,ai:8,aj:9,ba:0,bb:1,bc:2,bd:3,be:4,bf:5,bg:6,bh:7,bi:8,bj:9,ca:0,cb:1,cc:2,cd:3,ce:4,cf:5,cg:6,ch:7,ci:8,cj:9,da:0,db:1,dc:2,dd:3,de:4,df:5,dg:6,dh:7,di:8,dj:9,ea:0,eb:1,ec:2,ed:3,ee:4,ef:5,eg:6,eh:7,ei:8,ej:9,fa:0,fb:1,fc:2,fd:3,fe:4,ff:5,fg:6,fh:7,fi:8,fj:9,ga:0,gb:1,gc:2,gd:3,ge:4,gf:5,gg:6,gh:7,gi:8,gj:9,ha:0,hb:1,hc:2,hd:3,he:4,hf:5,hg:6,hh:7,hi:8,hj:9,ia:0,ib:1,ic:2,id:3,ie:4,if:5,ig:6,ih:7,ii:8,ij:9,ja:0,jb:1,jc:2,jd:3,je:4,jf:5,jg:6,jh:7,ji:8,jj:9,ka:0,kb:1,kc:2,kd:3,ke:4,kf:5,kg:6,kh:7,ki:8,kj:9,la:0,lb:1,lc:2,ld:3,le:4,lf:5,lg:6,lh:7,li:8,lj:9,ma:0,mb:1,mc:2,md:3,me:4,mf:5,mg:6,mh:7,mi:8,mj:9,na:0,nb:1,nc:2,nd:3,ne:4,nf:5,ng:6,nh:7,ni:8,nj:9,oa:0,ob:1,oc:2,od:3,oe:4,of:5,og:6,oh:7,oi:8,oj:9,pa:0,pb:1,pc:2,pd:3,pe:4,pf:5,pg:6,ph:7,pi:8,pj:9,");
--- a/js/src/jsfun.h
+++ b/js/src/jsfun.h
@@ -79,20 +79,17 @@
  *
  * NB: JSFUN_EXPR_CLOSURE reuses JSFUN_STUB_GSOPS, which is an API request flag
  * bit only, never stored in fun->flags.
  *
  * If we need more bits in the future, all flags for interpreted functions can
  * move to u.i.script->flags. For now we use function flag bits to minimize
  * pointer-chasing.
  */
-#define JSFUN_JOINABLE      0x0001  /* function is null closure that does not
-                                       appear to call itself via its own name
-                                       or arguments.callee */
-
+// 0x0001 was JSFUN_JOINABLE, but was deleted for bug 739808.
 #define JSFUN_PROTOTYPE     0x0800  /* function is Function.prototype for some
                                        global object */
 
 #define JSFUN_EXPR_CLOSURE  0x1000  /* expression closure: function(x) x*x */
                                     /* 0x2000 is JSFUN_TRCINFO:
                                        u.n.trcinfo is non-null */
 #define JSFUN_INTERPRETED   0x4000  /* use u.i if kind >= this value else u.n */
 #define JSFUN_FLAT_CLOSURE  0x8000  /* flat (aka "display") closure */
@@ -156,17 +153,17 @@ struct JSFunction : public JSObject_Slot
 #define JS_LOCAL_NAME_TO_ATOM(nameWord)  ((JSAtom *) ((nameWord) & ~(jsuword) 1))
 #define JS_LOCAL_NAME_IS_CONST(nameWord) ((((nameWord) & (jsuword) 1)) != 0)
 
     bool mightEscape() const {
         return isInterpreted() && (isFlatClosure() || !script()->bindings.hasUpvars());
     }
 
     bool joinable() const {
-        return flags & JSFUN_JOINABLE;
+        return false;
     }
 
     JSObject &compiledFunObj() {
         return *this;
     }
 
   private:
     /*
@@ -175,18 +172,16 @@ struct JSFunction : public JSObject_Slot
      * a compiler-created function object use the first one to hold a mutable
      * methodAtom() state variable, needed for correct foo.caller handling.
      */
     enum {
         METHOD_ATOM_SLOT  = JSSLOT_FUN_METHOD_ATOM
     };
 
   public:
-    inline void setJoinable();
-
     /*
      * Method name imputed from property uniquely assigned to or initialized,
      * where the function does not need to be cloned to carry a scope chain or
      * flattened upvars.
      */
     JSAtom *methodAtom() const {
         return (joinable() && getSlot(METHOD_ATOM_SLOT).isString())
                ? &getSlot(METHOD_ATOM_SLOT).toString()->asAtom()
--- a/js/src/jsfuninlines.h
+++ b/js/src/jsfuninlines.h
@@ -53,24 +53,16 @@ js::IsConstructing(CallReceiver call)
 
 inline bool
 JSFunction::inStrictMode() const
 {
     return script()->strictModeCode;
 }
 
 inline void
-JSFunction::setJoinable()
-{
-    JS_ASSERT(isInterpreted());
-    setSlot(METHOD_ATOM_SLOT, js::NullValue());
-    flags |= JSFUN_JOINABLE;
-}
-
-inline void
 JSFunction::setMethodAtom(JSAtom *atom)
 {
     JS_ASSERT(joinable());
     setSlot(METHOD_ATOM_SLOT, js::StringValue(atom));
 }
 
 inline JSObject *
 CloneFunctionObject(JSContext *cx, JSFunction *fun, JSObject *parent,
--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -4737,87 +4737,16 @@ BEGIN_CASE(JSOP_LAMBDA)
     LOAD_FUNCTION(0);
     JSObject *obj = fun;
 
     /* do-while(0) so we can break instead of using a goto. */
     do {
         JSObject *parent;
         if (fun->isNullClosure()) {
             parent = &regs.fp()->scopeChain();
-
-            if (fun->joinable()) {
-                jsbytecode *pc2 = AdvanceOverBlockchainOp(regs.pc + JSOP_LAMBDA_LENGTH);
-                JSOp op2 = JSOp(*pc2);
-
-                /*
-                 * Optimize var obj = {method: function () { ... }, ...},
-                 * this.method = function () { ... }; and other significant
-                 * single-use-of-null-closure bytecode sequences.
-                 *
-                 * WARNING: code in TraceRecorder::record_JSOP_LAMBDA must
-                 * match the optimization cases in the following code that
-                 * break from the outer do-while(0).
-                 */
-                if (op2 == JSOP_INITMETHOD) {
-#ifdef DEBUG
-                    const Value &lref = regs.sp[-1];
-                    JS_ASSERT(lref.isObject());
-                    JSObject *obj2 = &lref.toObject();
-                    JS_ASSERT(obj2->isObject());
-#endif
-
-                    fun->setMethodAtom(script->getAtom(GET_FULL_INDEX(pc2 - regs.pc)));
-                    break;
-                }
-
-                if (op2 == JSOP_SETMETHOD) {
-#ifdef DEBUG
-                    op2 = js_GetOpcode(cx, script, pc2 + JSOP_SETMETHOD_LENGTH);
-                    JS_ASSERT(op2 == JSOP_POP || op2 == JSOP_POPV);
-#endif
-                    const Value &lref = regs.sp[-1];
-                    if (lref.isObject() && lref.toObject().canHaveMethodBarrier()) {
-                        fun->setMethodAtom(script->getAtom(GET_FULL_INDEX(pc2 - regs.pc)));
-                        break;
-                    }
-                } else if (op2 == JSOP_CALL) {
-                    /*
-                     * Array.prototype.sort and String.prototype.replace are
-                     * optimized as if they are special form. We know that they
-                     * won't leak the joined function object in obj, therefore
-                     * we don't need to clone that compiler-created function
-                     * object for identity/mutation reasons.
-                     */
-                    int iargc = GET_ARGC(pc2);
-
-                    /*
-                     * Note that we have not yet pushed obj as the final argument,
-                     * so regs.sp[1 - (iargc + 2)], and not regs.sp[-(iargc + 2)],
-                     * is the callee for this JSOP_CALL.
-                     */
-                    const Value &cref = regs.sp[1 - (iargc + 2)];
-                    JSObject *callee;
-
-                    if (IsFunctionObject(cref, &callee)) {
-                        JSFunction *calleeFun = callee->getFunctionPrivate();
-                        if (Native native = calleeFun->maybeNative()) {
-                            if ((iargc == 1 && native == array_sort) ||
-                                (iargc == 2 && native == str_replace)) {
-                                break;
-                            }
-                        }
-                    }
-                } else if (op2 == JSOP_NULL) {
-                    pc2 += JSOP_NULL_LENGTH;
-                    op2 = JSOp(*pc2);
-
-                    if (op2 == JSOP_CALL && GET_ARGC(pc2) == 0)
-                        break;
-                }
-            }
         } else {
             parent = GetScopeChainFast(cx, regs.fp(), JSOP_LAMBDA, JSOP_LAMBDA_LENGTH);
             if (!parent)
                 goto error;
         }
 
         obj = CloneFunctionObject(cx, fun, parent, true);
         if (!obj)
--- a/js/src/methodjit/Compiler.cpp
+++ b/js/src/methodjit/Compiler.cpp
@@ -2557,37 +2557,16 @@ mjit::Compiler::generateMethod()
           BEGIN_CASE(JSOP_LAMBDA)
           {
             JSFunction *fun = script->getFunction(fullAtomIndex(PC));
 
             JSObjStubFun stub = stubs::Lambda;
             uint32 uses = 0;
 
             jsbytecode *pc2 = NULL;
-            if (fun->joinable()) {
-                pc2 = AdvanceOverBlockchainOp(PC + JSOP_LAMBDA_LENGTH);
-                JSOp next = JSOp(*pc2);
-
-                if (next == JSOP_INITMETHOD) {
-                    stub = stubs::LambdaJoinableForInit;
-                } else if (next == JSOP_SETMETHOD) {
-                    stub = stubs::LambdaJoinableForSet;
-                    uses = 1;
-                } else if (next == JSOP_CALL) {
-                    int iargc = GET_ARGC(pc2);
-                    if (iargc == 1 || iargc == 2) {
-                        stub = stubs::LambdaJoinableForCall;
-                        uses = frame.frameSlots();
-                    }
-                } else if (next == JSOP_NULL) {
-                    pc2 += JSOP_NULL_LENGTH;
-                    if (JSOp(*pc2) == JSOP_CALL && GET_ARGC(pc2) == 0)
-                        stub = stubs::LambdaJoinableForNull;
-                }
-            }
 
             prepareStubCall(Uses(uses));
             masm.move(ImmPtr(fun), Registers::ArgReg1);
 
             if (stub != stubs::Lambda)
                 masm.storePtr(ImmPtr(pc2), FrameAddress(offsetof(VMFrame, scratch)));
 
             INLINE_STUBCALL(stub, REJOIN_PUSH_OBJECT);