Bug 832329 - Improve analysis of definite properties for 'new' scripts, r=jandem, a=nonlibxul.
authorBrian Hackett <bhackett1024@gmail.com>
Mon, 21 Jan 2013 17:10:21 -0700
changeset 119451 5c56910fa445d3c6a91db4d7fe5115fa66e037a9
parent 119450 830cb30c2e1efce20b89b7f801cfc2cb745d228c
child 119452 219bceaafcb1a35132639cfbfc19b5ed98920313
push id24204
push usereakhgari@mozilla.com
push dateTue, 22 Jan 2013 17:50:00 +0000
treeherdermozilla-central@8962a7fabc33 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem, nonlibxul
bugs832329
milestone21.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 832329 - Improve analysis of definite properties for 'new' scripts, r=jandem, a=nonlibxul.
js/src/jit-test/tests/basic/new-read-before-write.js
js/src/jsinfer.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/new-read-before-write.js
@@ -0,0 +1,24 @@
+
+function Foo() {
+  var x = this.property;
+  this.property = 5;
+  glob = x;
+}
+Foo.prototype.property = 10;
+for (var i = 0; i < 10; i++) {
+  new Foo();
+  assertEq(glob, 10);
+}
+
+function Bar() {
+  this.property;
+  this.other = 5;
+}
+Bar.prototype.other = 10;
+Object.defineProperty(Bar.prototype, "property", {
+  get: function() { glob = this.other; }
+});
+for (var i = 0; i < 10; i++) {
+  new Bar();
+  assertEq(glob, 10);
+}
--- a/js/src/jsinfer.cpp
+++ b/js/src/jsinfer.cpp
@@ -4672,28 +4672,28 @@ ScriptAnalysis::integerOperation(jsbytec
 
       default:
         return true;
     }
 }
 
 /*
  * Persistent constraint clearing out newScript and definite properties from
- * an object should a property on another object get a setter.
+ * an object should a property on another object get a getter or setter.
  */
-class TypeConstraintClearDefiniteSetter : public TypeConstraint
+class TypeConstraintClearDefiniteGetterSetter : public TypeConstraint
 {
   public:
     TypeObject *object;
 
-    TypeConstraintClearDefiniteSetter(TypeObject *object)
+    TypeConstraintClearDefiniteGetterSetter(TypeObject *object)
         : object(object)
     {}
 
-    const char *kind() { return "clearDefiniteSetter"; }
+    const char *kind() { return "clearDefiniteGetterSetter"; }
 
     void newPropertyState(JSContext *cx, TypeSet *source)
     {
         if (!object->newScript)
             return;
         /*
          * Clear out the newScript shape and definite property information from
          * an object if the source type set could be a setter or could be
@@ -4702,16 +4702,38 @@ class TypeConstraintClearDefiniteSetter 
          */
         if (!(object->flags & OBJECT_FLAG_NEW_SCRIPT_CLEARED) && source->ownProperty(true))
             object->clearNewScript(cx);
     }
 
     void newType(JSContext *cx, TypeSet *source, Type type) {}
 };
 
+static bool
+AddClearDefiniteGetterSetterForPrototypeChain(JSContext *cx, TypeObject *type, jsid id)
+{
+    /*
+     * Ensure that if the properties named here could have a getter, setter or
+     * a permanent property in any transitive prototype, the definite
+     * properties get cleared from the shape.
+     */
+    RootedObject parent(cx, type->proto);
+    while (parent) {
+        TypeObject *parentObject = parent->getType(cx);
+        if (parentObject->unknownProperties())
+            return false;
+        HeapTypeSet *parentTypes = parentObject->getProperty(cx, id, false);
+        if (!parentTypes || parentTypes->ownProperty(true))
+            return false;
+        parentTypes->add(cx, cx->typeLifoAlloc().new_<TypeConstraintClearDefiniteGetterSetter>(type));
+        parent = parent->getProto();
+    }
+    return true;
+}
+
 /*
  * Constraint which clears definite properties on an object should a type set
  * contain any types other than a single object.
  */
 class TypeConstraintClearDefiniteSingle : public TypeConstraint
 {
   public:
     TypeObject *object;
@@ -4726,49 +4748,59 @@ class TypeConstraintClearDefiniteSingle 
         if (object->flags & OBJECT_FLAG_NEW_SCRIPT_CLEARED)
             return;
 
         if (source->baseFlags() || source->getObjectCount() > 1)
             object->clearNewScript(cx);
     }
 };
 
+struct NewScriptPropertiesState
+{
+    RootedFunction thisFunction;
+    RootedObject baseobj;
+    Vector<TypeNewScript::Initializer> initializerList;
+    Vector<jsid> accessedProperties;
+
+    NewScriptPropertiesState(JSContext *cx)
+      : thisFunction(cx), baseobj(cx), initializerList(cx), accessedProperties(cx)
+    {}
+};
+
 static bool
 AnalyzePoppedThis(JSContext *cx, Vector<SSAUseChain *> *pendingPoppedThis,
-                  TypeObject *type, HandleFunction fun, MutableHandleObject pbaseobj,
-                  Vector<TypeNewScript::Initializer> *initializerList);
+                  TypeObject *type, JSFunction *fun, NewScriptPropertiesState &state);
 
 static bool
-AnalyzeNewScriptProperties(JSContext *cx, TypeObject *type, HandleFunction fun,
-                           MutableHandleObject pbaseobj,
-                           Vector<TypeNewScript::Initializer> *initializerList)
+AnalyzeNewScriptProperties(JSContext *cx, TypeObject *type, JSFunction *fun,
+                           NewScriptPropertiesState &state)
 {
     AssertCanGC();
 
     /*
      * When invoking 'new' on the specified script, try to find some properties
      * which will definitely be added to the created object before it has a
      * chance to escape and be accessed elsewhere.
      *
      * Returns true if the entire script was analyzed (pbaseobj has been
      * preserved), false if we had to bail out part way through (pbaseobj may
      * have been cleared).
      */
 
-    if (initializerList->length() > 50) {
+    if (state.initializerList.length() > 50) {
         /*
          * Bail out on really long initializer lists (far longer than maximum
          * number of properties we can track), we may be recursing.
          */
         return false;
     }
 
     RootedScript script(cx, fun->nonLazyScript());
     if (!JSScript::ensureRanAnalysis(cx, script) || !JSScript::ensureRanInference(cx, script)) {
-        pbaseobj.set(NULL);
+        state.baseobj = NULL;
         cx->compartment->types.setPendingNukeTypes(cx);
         return false;
     }
 
     ScriptAnalysis *analysis = script->analysis();
 
     /*
      * Offset of the last bytecode which popped 'this' and which we have
@@ -4803,29 +4835,29 @@ AnalyzeNewScriptProperties(JSContext *cx
             continue;
 
         /*
          * End analysis after the first return statement from the script,
          * returning success if the return is unconditional.
          */
         if (op == JSOP_RETURN || op == JSOP_STOP || op == JSOP_RETRVAL) {
             if (offset < lastThisPopped) {
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
                 entirelyAnalyzed = false;
                 break;
             }
 
             entirelyAnalyzed = code->unconditional;
             break;
         }
 
         /* 'this' can escape through a call to eval. */
         if (op == JSOP_EVAL) {
             if (offset < lastThisPopped)
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
             entirelyAnalyzed = false;
             break;
         }
 
         /*
          * We are only interested in places where 'this' is popped. The new
          * 'this' value cannot escape and be accessed except through such uses.
          */
@@ -4852,20 +4884,18 @@ AnalyzeNewScriptProperties(JSContext *cx
         /*
          * If offset >= the offset at the top of the pending stack, we either
          * encountered the end of a compound inline assignment or a 'this' was
          * immediately popped and used. In either case, handle the use.
          */
         if (!pendingPoppedThis.empty() &&
             offset >= pendingPoppedThis.back()->offset) {
             lastThisPopped = pendingPoppedThis[0]->offset;
-            if (!AnalyzePoppedThis(cx, &pendingPoppedThis, type, fun, pbaseobj,
-                                   initializerList)) {
+            if (!AnalyzePoppedThis(cx, &pendingPoppedThis, type, fun, state))
                 return false;
-            }
         }
 
         if (!pendingPoppedThis.append(uses)) {
             entirelyAnalyzed = false;
             break;
         }
     }
 
@@ -4880,29 +4910,28 @@ AnalyzeNewScriptProperties(JSContext *cx
      * The 'this.y = "foo"' assignment breaks the above loop since the 'this'
      * in the assignment is popped multiple times, with 'this.x' being left on
      * the pending stack. But we can't mark 'x' as a definite property, as
      * that would make it come before 'y' in the shape tree, breaking the
      * invariant.
      */
     if (entirelyAnalyzed &&
         !pendingPoppedThis.empty() &&
-        !AnalyzePoppedThis(cx, &pendingPoppedThis, type, fun, pbaseobj,
-                           initializerList)) {
+        !AnalyzePoppedThis(cx, &pendingPoppedThis, type, fun, state))
+    {
         return false;
     }
 
     /* Will have hit a STOP or similar, unless the script always throws. */
     return entirelyAnalyzed;
 }
 
 static bool
 AnalyzePoppedThis(JSContext *cx, Vector<SSAUseChain *> *pendingPoppedThis,
-                  TypeObject *type, HandleFunction fun, MutableHandleObject pbaseobj,
-                  Vector<TypeNewScript::Initializer> *initializerList)
+                  TypeObject *type, JSFunction *fun, NewScriptPropertiesState &state)
 {
     RootedScript script(cx, fun->nonLazyScript());
     ScriptAnalysis *analysis = script->analysis();
 
     while (!pendingPoppedThis->empty()) {
         SSAUseChain *uses = pendingPoppedThis->back();
         pendingPoppedThis->popBack();
 
@@ -4917,63 +4946,93 @@ AnalyzePoppedThis(JSContext *cx, Vector<
              */
             RootedId id(cx, NameToId(script->getName(GET_UINT32_INDEX(pc))));
             if (IdToTypeId(id) != id)
                 return false;
             if (id_prototype(cx) == id || id___proto__(cx) == id || id_constructor(cx) == id)
                 return false;
 
             /*
-             * Ensure that if the properties named here could have a setter or
-             * a permanent property in any transitive prototype, the definite
-             * properties get cleared from the shape.
+             * Don't add definite properties for properties that were already
+             * read in the constructor.
              */
-            RootedObject parent(cx, type->proto);
-            while (parent) {
-                TypeObject *parentObject = parent->getType(cx);
-                if (parentObject->unknownProperties())
+            for (size_t i = 0; i < state.accessedProperties.length(); i++) {
+                if (state.accessedProperties[i] == id)
                     return false;
-                HeapTypeSet *parentTypes = parentObject->getProperty(cx, id, false);
-                if (!parentTypes || parentTypes->ownProperty(true))
-                    return false;
-                parentTypes->add(cx, cx->typeLifoAlloc().new_<TypeConstraintClearDefiniteSetter>(type));
-                parent = parent->getProto();
             }
 
-            unsigned slotSpan = pbaseobj->slotSpan();
+            if (!AddClearDefiniteGetterSetterForPrototypeChain(cx, type, id))
+                return false;
+
+            unsigned slotSpan = state.baseobj->slotSpan();
             RootedValue value(cx, UndefinedValue());
-            if (!DefineNativeProperty(cx, pbaseobj, id, value, NULL, NULL,
+            if (!DefineNativeProperty(cx, state.baseobj, id, value, NULL, NULL,
                                       JSPROP_ENUMERATE, 0, 0, DNP_SKIP_TYPE)) {
                 cx->compartment->types.setPendingNukeTypes(cx);
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
                 return false;
             }
 
-            if (pbaseobj->inDictionaryMode()) {
-                pbaseobj.set(NULL);
+            if (state.baseobj->inDictionaryMode()) {
+                state.baseobj = NULL;
                 return false;
             }
 
-            if (pbaseobj->slotSpan() == slotSpan) {
+            if (state.baseobj->slotSpan() == slotSpan) {
                 /* Set a duplicate property. */
                 return false;
             }
 
             TypeNewScript::Initializer setprop(TypeNewScript::Initializer::SETPROP, uses->offset);
-            if (!initializerList->append(setprop)) {
+            if (!state.initializerList.append(setprop)) {
                 cx->compartment->types.setPendingNukeTypes(cx);
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
                 return false;
             }
 
-            if (pbaseobj->slotSpan() >= (TYPE_FLAG_DEFINITE_MASK >> TYPE_FLAG_DEFINITE_SHIFT)) {
+            if (state.baseobj->slotSpan() >= (TYPE_FLAG_DEFINITE_MASK >> TYPE_FLAG_DEFINITE_SHIFT)) {
                 /* Maximum number of definite properties added. */
                 return false;
             }
-        } else if (op == JSOP_FUNCALL && uses->u.which == GET_ARGC(pc) - 1) {
+        } else if (op == JSOP_GETPROP && uses->u.which == 0) {
+            /*
+             * Properties can be read from the 'this' object if the following hold:
+             *
+             * - The read is not on a getter along the prototype chain, which
+             *   could cause 'this' to escape.
+             *
+             * - The accessed property is either already a definite property or
+             *   is not later added as one. Since the definite properties are
+             *   added to the object at the point of its creation, reading a
+             *   definite property before it is assigned could incorrectly hit.
+             */
+            RootedId id(cx, NameToId(script->getName(GET_UINT32_INDEX(pc))));
+            if (IdToTypeId(id) != id)
+                return false;
+            if (!state.baseobj->nativeLookup(cx, id) && !state.accessedProperties.append(id.get())) {
+                cx->compartment->types.setPendingNukeTypes(cx);
+                state.baseobj = NULL;
+                return false;
+            }
+
+            if (!AddClearDefiniteGetterSetterForPrototypeChain(cx, type, id))
+                return false;
+
+            /*
+             * Populate the read with any value from the type's proto, if
+             * this is being used in a function call and we need to analyze the
+             * callee's behavior.
+             */
+            Shape *shape = type->proto ? type->proto->nativeLookup(cx, id) : NULL;
+            if (shape && shape->hasSlot()) {
+                Value protov = type->proto->getSlot(shape->slot());
+                TypeSet *types = script->analysis()->bytecodeTypes(pc);
+                types->addType(cx, GetValueType(cx, protov));
+            }
+        } else if ((op == JSOP_FUNCALL || op == JSOP_FUNAPPLY) && uses->u.which == GET_ARGC(pc) - 1) {
             /*
              * Passed as the first parameter to Function.call. Follow control
              * into the callee, and add any definite properties it assigns to
              * the object as well. :TODO: This is narrow pattern matching on
              * the inheritance patterns seen in the v8-deltablue benchmark, and
              * needs robustness against other ways initialization can cross
              * script boundaries.
              *
@@ -4994,53 +5053,56 @@ AnalyzePoppedThis(JSContext *cx, Vector<
              * in performing the call (for the greater good!).
              */
             analysis->breakTypeBarriersSSA(cx, analysis->poppedValue(calleepc, 0));
             analysis->breakTypeBarriers(cx, calleepc - script->code, true);
 
             StackTypeSet *funcallTypes = analysis->poppedTypes(pc, GET_ARGC(pc) + 1);
             StackTypeSet *scriptTypes = analysis->poppedTypes(pc, GET_ARGC(pc));
 
-            /* Need to definitely be calling Function.call on a specific script. */
-            RootedFunction function(cx);
+            /* Need to definitely be calling Function.call/apply on a specific script. */
+            JSFunction *function;
             {
                 RawObject funcallObj = funcallTypes->getSingleton();
                 RawObject scriptObj = scriptTypes->getSingleton();
-                if (!funcallObj || !scriptObj || !scriptObj->isFunction() ||
+                if (!funcallObj || !funcallObj->isFunction() ||
+                    funcallObj->toFunction()->isInterpreted() ||
+                    !scriptObj || !scriptObj->isFunction() ||
                     !scriptObj->toFunction()->isInterpreted()) {
                     return false;
                 }
+                Native native = funcallObj->toFunction()->native();
+                if (native != js_fun_call && native != js_fun_apply)
+                    return false;
                 function = scriptObj->toFunction();
             }
 
             /*
              * Generate constraints to clear definite properties from the type
              * should the Function.call or callee itself change in the future.
              */
             funcallTypes->add(cx,
                 cx->analysisLifoAlloc().new_<TypeConstraintClearDefiniteSingle>(type));
             scriptTypes->add(cx,
                 cx->analysisLifoAlloc().new_<TypeConstraintClearDefiniteSingle>(type));
 
             TypeNewScript::Initializer pushframe(TypeNewScript::Initializer::FRAME_PUSH, uses->offset);
-            if (!initializerList->append(pushframe)) {
+            if (!state.initializerList.append(pushframe)) {
                 cx->compartment->types.setPendingNukeTypes(cx);
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
                 return false;
             }
 
-            if (!AnalyzeNewScriptProperties(cx, type, function,
-                                            pbaseobj, initializerList)) {
+            if (!AnalyzeNewScriptProperties(cx, type, function, state))
                 return false;
-            }
 
             TypeNewScript::Initializer popframe(TypeNewScript::Initializer::FRAME_POP, 0);
-            if (!initializerList->append(popframe)) {
+            if (!state.initializerList.append(popframe)) {
                 cx->compartment->types.setPendingNukeTypes(cx);
-                pbaseobj.set(NULL);
+                state.baseobj = NULL;
                 return false;
             }
 
             /*
              * The callee never lets the 'this' value escape, continue looking
              * for definite properties in the remainder of this script.
              */
         } else {
@@ -5058,66 +5120,71 @@ AnalyzePoppedThis(JSContext *cx, Vector<
  * newScript on the type after they were cleared by a GC.
  */
 static void
 CheckNewScriptProperties(JSContext *cx, HandleTypeObject type, HandleFunction fun)
 {
     if (type->unknownProperties())
         return;
 
+    NewScriptPropertiesState state(cx);
+    state.thisFunction = fun;
+
     /* Strawman object to add properties to and watch for duplicates. */
-    RootedObject baseobj(cx, NewBuiltinClassInstance(cx, &ObjectClass, gc::FINALIZE_OBJECT16));
-    if (!baseobj) {
+    state.baseobj = NewBuiltinClassInstance(cx, &ObjectClass, gc::FINALIZE_OBJECT16);
+    if (!state.baseobj) {
         if (type->newScript)
             type->clearNewScript(cx);
         return;
     }
 
-    Vector<TypeNewScript::Initializer> initializerList(cx);
-    AnalyzeNewScriptProperties(cx, type, fun, &baseobj, &initializerList);
-    if (!baseobj || baseobj->slotSpan() == 0 || !!(type->flags & OBJECT_FLAG_NEW_SCRIPT_CLEARED)) {
+    AnalyzeNewScriptProperties(cx, type, fun, state);
+    if (!state.baseobj ||
+        state.baseobj->slotSpan() == 0 ||
+        !!(type->flags & OBJECT_FLAG_NEW_SCRIPT_CLEARED))
+    {
         if (type->newScript)
             type->clearNewScript(cx);
         return;
     }
 
     /*
      * If the type already has a new script, we are just regenerating the type
      * constraints and don't need to make another TypeNewScript. Make sure that
      * the properties added to baseobj match the type's definite properties.
      */
     if (type->newScript) {
-        if (!type->matchDefiniteProperties(baseobj))
+        if (!type->matchDefiniteProperties(state.baseobj))
             type->clearNewScript(cx);
         return;
     }
 
-    gc::AllocKind kind = gc::GetGCObjectKind(baseobj->slotSpan());
+    gc::AllocKind kind = gc::GetGCObjectKind(state.baseobj->slotSpan());
 
     /* We should not have overflowed the maximum number of fixed slots for an object. */
-    JS_ASSERT(gc::GetGCKindSlots(kind) >= baseobj->slotSpan());
+    JS_ASSERT(gc::GetGCKindSlots(kind) >= state.baseobj->slotSpan());
 
     TypeNewScript::Initializer done(TypeNewScript::Initializer::DONE, 0);
 
     /*
      * The base object may have been created with a different finalize kind
      * than we will use for subsequent new objects. Generate an object with the
      * appropriate final shape.
      */
-    RootedShape shape(cx, baseobj->lastProperty());
-    baseobj = NewReshapedObject(cx, type, baseobj->getParent(), kind, shape);
-    if (!baseobj ||
-        !type->addDefiniteProperties(cx, baseobj) ||
-        !initializerList.append(done)) {
+    RootedShape shape(cx, state.baseobj->lastProperty());
+    state.baseobj = NewReshapedObject(cx, type, state.baseobj->getParent(), kind, shape);
+    if (!state.baseobj ||
+        !type->addDefiniteProperties(cx, state.baseobj) ||
+        !state.initializerList.append(done)) {
         cx->compartment->types.setPendingNukeTypes(cx);
         return;
     }
 
     size_t numBytes = sizeof(TypeNewScript)
-                    + (initializerList.length() * sizeof(TypeNewScript::Initializer));
+                    + (state.initializerList.length() * sizeof(TypeNewScript::Initializer));
 #ifdef JSGC_ROOT_ANALYSIS
     // calloc can legitimately return a pointer that appears to be poisoned.
     void *p;
     do {
         p = cx->calloc_(numBytes);
     } while (IsPoisonedPtr(p));
     type->newScript = (TypeNewScript *) p;
 #else
@@ -5128,21 +5195,23 @@ CheckNewScriptProperties(JSContext *cx, 
         cx->compartment->types.setPendingNukeTypes(cx);
         return;
     }
 
     AutoAssertNoGC nogc;
 
     type->newScript->fun = fun;
     type->newScript->allocKind = kind;
-    type->newScript->shape = baseobj->lastProperty();
+    type->newScript->shape = state.baseobj->lastProperty();
 
     type->newScript->initializerList = (TypeNewScript::Initializer *)
         ((char *) type->newScript.get() + sizeof(TypeNewScript));
-    PodCopy(type->newScript->initializerList, initializerList.begin(), initializerList.length());
+    PodCopy(type->newScript->initializerList,
+            state.initializerList.begin(),
+            state.initializerList.length());
 }
 
 /////////////////////////////////////////////////////////////////////
 // Printing
 /////////////////////////////////////////////////////////////////////
 
 void
 ScriptAnalysis::printTypes(JSContext *cx)