Bug 767419 - Support idempotent GetProperty ICs. r=dvander,bhackett

js/src/ion/Bailouts.cpp

@@ -501,12 +501,9 @@ ion::RecompileForInlining()
             script->lineno);
 
     // Invalidate the script to force a recompile.
-    Vector<types::RecompileInfo> scripts(cx);
-    if (!scripts.append(types::RecompileInfo(script)))
+    if (!Invalidate(cx, script, /* resetUses */ false))
         return BAILOUT_RETURN_FATAL_ERROR;
 
-    Invalidate(cx->runtime->defaultFreeOp(), scripts, /* resetUses */ false);
-
     // Invalidation should not reset the use count.
     JS_ASSERT(script->getUseCount() >= js_IonOptions.usesBeforeInlining);
 

js/src/ion/CodeGenerator.cpp

@@ -2761,9 +2761,11 @@ CodeGenerator::visitOutOfLineCacheGetProperty(OutOfLineCache *ool)
     IonCacheGetProperty cache(ool->getInlineJump(), ool->getInlineLabel(),
                               masm.labelForPatch(), liveRegs,
                               objReg, name, output);
-    JS_ASSERT(mir->resumePoint() != NULL);
 
-    cache.setScriptedLocation(mir->block()->info().script(), mir->resumePoint()->pc());
+    if (mir->resumePoint())
+        cache.setScriptedLocation(mir->block()->info().script(), mir->resumePoint()->pc());
+    else
+        cache.setIdempotent();
     size_t cacheIndex = allocateCache(cache);
 
     saveLive(ins);

js/src/ion/Ion.cpp

@@ -1343,6 +1343,19 @@ ion::Invalidate(FreeOp *fop, const Vector<types::RecompileInfo> &invalid, bool r
     }
 }
 
+bool
+ion::Invalidate(JSContext *cx, JSScript *script, bool resetUses)
+{
+    JS_ASSERT(script->hasIonScript());
+
+    Vector<types::RecompileInfo> scripts(cx);
+    if (!scripts.append(types::RecompileInfo(script)))
+        return false;
+
+    Invalidate(cx->runtime->defaultFreeOp(), scripts, resetUses);
+    return true;
+}
+
 void
 ion::FinishInvalidation(FreeOp *fop, JSScript *script)
 {

js/src/ion/Ion.h

@@ -157,11 +157,18 @@ struct IonOptions
     // Default: 800
     uint32 inlineMaxTotalBytecodeLength;
 
+    // Whether functions are compiled immediately.
+    //
+    // Default: false
+    bool eagerCompilation;
+
     void setEagerCompilation() {
+        eagerCompilation = true;
         usesBeforeCompile = usesBeforeCompileNoJaeger = 0;
 
         // Eagerly inline calls to improve test coverage.
         usesBeforeInlining = 0;
+        smallFunctionUsesBeforeInlining = 0;
     }
 
     IonOptions()
@@ -182,7 +189,8 @@ struct IonOptions
         smallFunctionMaxBytecodeLength(100),
         smallFunctionUsesBeforeInlining(usesBeforeInlining / 4),
         polyInlineMax(4),
-        inlineMaxTotalBytecodeLength(800)
+        inlineMaxTotalBytecodeLength(800),
+        eagerCompilation(false)
     { }
 };
 
@@ -238,6 +246,8 @@ IonExecStatus SideCannon(JSContext *cx, StackFrame *fp, jsbytecode *pc);
 
 // Walk the stack and invalidate active Ion frames for the invalid scripts.
 void Invalidate(FreeOp *fop, const Vector<types::RecompileInfo> &invalid, bool resetUses = true);
+bool Invalidate(JSContext *cx, JSScript *script, bool resetUses = true);
+
 void MarkFromIon(JSCompartment *comp, Value *vp);
 
 void ToggleBarriers(JSCompartment *comp, bool needs);

js/src/ion/IonBuilder.cpp

@@ -4905,6 +4905,21 @@ IonBuilder::TestCommonPropFunc(JSContext *cx, types::TypeSet *types, HandleId id
     return true;
 }
 
+// Returns true if an idempotent cache has ever invalidated this script
+// or an outer script.
+bool
+IonBuilder::invalidatedIdempotentCache()
+{
+    IonBuilder *builder = this;
+    do {
+        if (builder->script->invalidatedIdempotentCache)
+            return true;
+        builder = builder->callerBuilder_;
+    } while (builder);
+
+    return false;
+}
+
 bool
 IonBuilder::jsop_getprop(HandlePropertyName name)
 {
@@ -4991,6 +5006,17 @@ IonBuilder::jsop_getprop(HandlePropertyName name)
             if (unary.rval != MIRType_Undefined && unary.rval != MIRType_Null)
                 load->setResultType(unary.rval);
         }
+
+        // Try to mark the cache as idempotent. We only do this if JM is enabled
+        // (its ICs are used to mark property reads as likely non-idempotent) or
+        // if we are compiling eagerly (to improve test coverage).
+        if ((cx->methodJitEnabled || js_IonOptions.eagerCompilation) &&
+            !invalidatedIdempotentCache())
+        {
+            if (oracle->propertyReadIdempotent(script, pc, id))
+                load->setIdempotent();
+        }
+
         ins = load;
     } else {
         ins = MCallGetProperty::New(obj, name);
@@ -4999,7 +5025,7 @@ IonBuilder::jsop_getprop(HandlePropertyName name)
     current->add(ins);
     current->push(ins);
 
-    if (!resumeAfter(ins))
+    if (ins->isEffectful() && !resumeAfter(ins))
         return false;
 
     if (ins->isCallGetProperty())

js/src/ion/IonBuilder.h

@@ -318,6 +318,8 @@ class IonBuilder : public MIRGenerator
 
     MDefinition *walkScopeChain(unsigned hops);
 
+    bool invalidatedIdempotentCache();
+
     bool jsop_add(MDefinition *left, MDefinition *right);
     bool jsop_bitnot();
     bool jsop_bitop(JSOp op);

js/src/ion/IonCaches.cpp

@@ -257,7 +257,8 @@ IonCacheGetProperty::attachNative(JSContext *cx, JSObject *obj, JSObject *holder
     PatchJump(exitJump, cacheLabel());
     updateLastJump(exitJump);
 
-    IonSpew(IonSpew_InlineCaches, "Generated native GETPROP stub at %p", code->raw());
+    IonSpew(IonSpew_InlineCaches, "Generated native GETPROP stub at %p %s", code->raw(),
+            idempotent() ? "(idempotent)" : "(not idempotent)");
 
     return true;
 }
@@ -288,6 +289,67 @@ IsCacheableGetProp(JSObject *obj, JSObject *holder, const Shape *shape)
             shape->hasDefaultGetter());
 }
 
+static bool
+IsIdempotentProtoChain(JSObject *obj)
+{
+    // Return false if obj (or an object on its proto chain) is non-native or has
+    // a resolve or lookup hook.
+    while (true) {
+        if (!obj->isNative())
+            return false;
+
+        if (obj->getClass()->resolve != JS_ResolveStub)
+            return false;
+
+        if (obj->getOps()->lookupProperty || obj->getOps()->lookupGeneric || obj->getOps()->lookupElement)
+            return false;
+
+        JSObject *proto = obj->getProto();
+        if (!proto)
+            return true;
+        obj = proto;
+    }
+
+    JS_NOT_REACHED("Should not get here");
+    return false;
+}
+
+static bool
+TryAttachNativeStub(JSContext *cx, IonCacheGetProperty &cache, JSObject *obj,
+                    HandlePropertyName name, bool *isCacheableNative)
+{
+    JS_ASSERT(!*isCacheableNative);
+
+    if (!obj->isNative())
+        return true;
+
+    // If the cache is idempotent, watch out for resolve hooks or non-native
+    // objects on the proto chain. We check this before calling lookupProperty,
+    // to make sure no effectful lookup hooks or resolve hooks are called.
+    if (cache.idempotent() && !IsIdempotentProtoChain(obj))
+        return true;
+
+    JSObject *holder;
+    JSProperty *prop;
+    if (!obj->lookupProperty(cx, name, &holder, &prop))
+        return false;
+
+    const Shape *shape = (const Shape *)prop;
+    if (!IsCacheableGetProp(obj, holder, shape))
+        return true;
+
+    *isCacheableNative = true;
+
+    if (cache.stubCount() < MAX_STUBS) {
+        cache.incrementStubCount();
+
+        if (!cache.attachNative(cx, obj, holder, shape))
+            return false;
+    }
+
+    return true;
+}
+
 bool
 js::ion::GetPropertyCache(JSContext *cx, size_t cacheIndex, HandleObject obj, Value *vp)
 {
@@ -304,22 +366,30 @@ js::ion::GetPropertyCache(JSContext *cx, size_t cacheIndex, HandleObject obj, Va
     // Override the return value if we are invalidated (bug 728188).
     AutoDetectInvalidation adi(cx, vp, topScript);
 
+    // If the cache is idempotent, we will redo the op in the interpreter.
+    if (cache.idempotent())
+        adi.disable();
+
     // For now, just stop generating new stubs once we hit the stub count
     // limit. Once we can make calls from within generated stubs, a new call
     // stub will be generated instead and the previous stubs unlinked.
-    if (cache.stubCount() < MAX_STUBS && obj->isNative()) {
-        cache.incrementStubCount();
+    bool isCacheableNative = false;
+    if (!TryAttachNativeStub(cx, cache, obj, name, &isCacheableNative))
+        return false;
 
-        JSObject *holder;
-        JSProperty *prop;
-        if (!obj->lookupProperty(cx, name, &holder, &prop))
-            return false;
+    if (cache.idempotent() && !isCacheableNative) {
+        // Invalidate the cache if the property was not found, or was found on
+        // a non-native object. This ensures:
+        // 1) The property read has no observable side-effects.
+        // 2) There's no need to dynamically monitor the return type. This would
+        //    be complicated since (due to GVN) there can be multiple pc's
+        //    associated with a single idempotent cache.
+        IonSpew(IonSpew_InlineCaches, "Invalidating from idempotent cache %s:%d",
+                topScript->filename, topScript->lineno);
 
-        const Shape *shape = (const Shape *)prop;
-        if (IsCacheableGetProp(obj, holder, shape)) {
-            if (!cache.attachNative(cx, obj, holder, shape))
-                return false;
-        }
+        topScript->invalidatedIdempotentCache = true;
+
+        return Invalidate(cx, topScript);
     }
 
     RootedId id(cx, NameToId(name));
@@ -331,16 +401,21 @@ js::ion::GetPropertyCache(JSContext *cx, size_t cacheIndex, HandleObject obj, Va
             return false;
     }
 
+    if (!cache.idempotent()) {
+        // If the cache is idempotent, the property exists so we don't have to
+        // call __noSuchMethod__.
+
 #if JS_HAS_NO_SUCH_METHOD
-    // Handle objects with __noSuchMethod__.
-    if (JSOp(*pc) == JSOP_CALLPROP && JS_UNLIKELY(vp->isPrimitive())) {
-        if (!OnUnknownMethod(cx, obj, IdToValue(id), vp))
-            return false;
-    }
+        // Handle objects with __noSuchMethod__.
+        if (JSOp(*pc) == JSOP_CALLPROP && JS_UNLIKELY(vp->isPrimitive())) {
+            if (!OnUnknownMethod(cx, obj, IdToValue(id), vp))
+                return false;
+        }
 #endif
 
-    // Monitor changes to cache entry.
-    types::TypeScript::Monitor(cx, script, pc, *vp);
+        // Monitor changes to cache entry.
+        types::TypeScript::Monitor(cx, script, pc, *vp);
+    }
 
     return true;
 }

js/src/ion/IonCaches.h

@@ -220,6 +220,12 @@ class IonCache
     bool idempotent() {
         return idempotent_;
     }
+    void setIdempotent() {
+        JS_ASSERT(!idempotent_);
+        JS_ASSERT(!script);
+        JS_ASSERT(!pc);
+        idempotent_ = true;
+    }
 
     void updateLastJump(CodeLocationJump jump) {
         lastJump_ = jump;
@@ -256,6 +262,7 @@ class IonCache
     }
 
     void setScriptedLocation(JSScript *script, jsbytecode *pc) {
+        JS_ASSERT(!idempotent_);
         this->script = script;
         this->pc = pc;
     }

js/src/ion/MIR.h

@@ -3834,12 +3834,19 @@ class MGetPropertyCache
     public SingleObjectPolicy
 {
     CompilerRootPropertyName name_;
+    bool idempotent_;
 
     MGetPropertyCache(MDefinition *obj, HandlePropertyName name)
       : MUnaryInstruction(obj),
-        name_(name)
+        name_(name),
+        idempotent_(false)
     {
         setResultType(MIRType_Value);
+
+        // The cache will invalidate if there are objects with e.g. lookup or
+        // resolve hooks on the proto chain. setGuard ensures this check is not
+        // eliminated.
+        setGuard();
     }
 
   public:
@@ -3855,7 +3862,31 @@ class MGetPropertyCache
     PropertyName *name() const {
         return name_;
     }
+    bool idempotent() const {
+        return idempotent_;
+    }
+    void setIdempotent() {
+        idempotent_ = true;
+        setMovable();
+    }
     TypePolicy *typePolicy() { return this; }
+
+    bool congruentTo(MDefinition * const &ins) const {
+        if (!idempotent_)
+            return false;
+        if (!ins->isGetPropertyCache())
+            return false;
+        if (name() != ins->toGetPropertyCache()->name())
+            return false;
+        return congruentIfOperandsEqual(ins);
+    }
+
+    AliasSet getAliasSet() const {
+        if (idempotent_)
+            return AliasSet::Load(AliasSet::ObjectFields | AliasSet::Slot);
+        return AliasSet::Store(AliasSet::Any);
+    }
+
 };
 
 class MGetElementCache

js/src/ion/TypeOracle.cpp

@@ -252,6 +252,38 @@ TypeInferenceOracle::propertyReadBarrier(JSScript *script, jsbytecode *pc)
     return NULL;
 }
 
+bool
+TypeInferenceOracle::propertyReadIdempotent(JSScript *script, jsbytecode *pc, HandleId id)
+{
+    if (script->analysis()->getCode(pc).notIdempotent)
+        return false;
+
+    if (id.value() != MakeTypeId(cx, id.value()))
+        return false;
+
+    TypeSet *types = script->analysis()->poppedTypes(pc, 0);
+    if (!types || types->unknownObject())
+        return false;
+
+    for (unsigned i = 0; i < types->getObjectCount(); i++) {
+        if (types->getSingleObject(i))
+            return false;
+
+        if (TypeObject *obj = types->getTypeObject(i)) {
+            if (obj->unknownProperties())
+                return false;
+
+            // Check if the property has been reconfigured or is a getter.
+            TypeSet *propertyTypes = obj->getProperty(cx, id, false);
+            if (!propertyTypes || propertyTypes->isOwnProperty(cx, obj, true))
+                return false;
+        }
+    }
+
+    types->addFreeze(cx);
+    return true;
+}
+
 bool
 TypeInferenceOracle::elementReadIsDenseArray(JSScript *script, jsbytecode *pc)
 {

js/src/ion/TypeOracle.h

@@ -118,6 +118,9 @@ class TypeOracle
     virtual types::TypeSet *propertyReadBarrier(JSScript *script, jsbytecode *pc) {
         return NULL;
     }
+    virtual bool propertyReadIdempotent(JSScript *script, jsbytecode *pc, HandleId id) {
+        return false;
+    }
     virtual types::TypeSet *globalPropertyWrite(JSScript *script, jsbytecode *pc,
                                                 jsid id, bool *canSpecialize) {
         *canSpecialize = true;
@@ -257,6 +260,7 @@ class TypeInferenceOracle : public TypeOracle
     types::TypeSet *globalPropertyTypeSet(JSScript *script, jsbytecode *pc, jsid id);
     types::TypeSet *propertyRead(JSScript *script, jsbytecode *pc);
     types::TypeSet *propertyReadBarrier(JSScript *script, jsbytecode *pc);
+    bool propertyReadIdempotent(JSScript *script, jsbytecode *pc, HandleId id);
     types::TypeSet *globalPropertyWrite(JSScript *script, jsbytecode *pc, jsid id, bool *canSpecialize);
     types::TypeSet *returnTypeSet(JSScript *script, jsbytecode *pc, types::TypeSet **barrier);
     types::TypeSet *getCallTarget(JSScript *caller, uint32 argc, jsbytecode *pc);

js/src/ion/VMFunctions.h

@@ -396,17 +396,24 @@ class AutoDetectInvalidation
     JSScript *script_;
     IonScript *ion_;
     Value *rval_;
+    bool disabled_;
 
   public:
     AutoDetectInvalidation(JSContext *cx, Value *rval, JSScript *script = NULL)
       : cx_(cx),
         script_(script ? script : GetTopIonJSScript(cx)),
         ion_(script_->ionScript()),
-        rval_(rval)
+        rval_(rval),
+        disabled_(false)
     { }
 
+    void disable() {
+        JS_ASSERT(!disabled_);
+        disabled_ = true;
+    }
+
     ~AutoDetectInvalidation() {
-        if (script_->ion != ion_)
+        if (!disabled_ && script_->ion != ion_)
             cx_->runtime->setIonReturnOverride(*rval_);
     }
 };

js/src/jit-test/tests/ion/getprop-idempotent-cache-1.js

@@ -0,0 +1,17 @@
+function f(o) {
+    var res = 0;
+    for (var i=0; i<11000; i++) {
+        res += o.x;
+    }
+    return res;
+}
+
+function O(x) {
+    if (x)
+        this.x = 10;
+}
+
+f(new O(true));
+
+// "o.x" is now missing so the idempotent cache should invalidate f.
+f(new O(false));

js/src/jit-test/tests/ion/getprop-idempotent-cache-2.js

@@ -0,0 +1,21 @@
+function f(o) {
+    var res = 0;
+    for (var i=0; i<110; i++) {
+        res += o.x;
+    }
+    return res;
+}
+
+function O(x) {
+    if ([].length == 0) // Thwart definite slot analysis.
+        this.x = 10;
+}
+
+var o = new O(true);
+f(o);
+
+// Add a getter, this should invalidate the script containing the idempotent cache.
+var res = 0;
+o.__defineGetter__("x", function() { res++; });
+f(o);
+assertEq(res, 110);

js/src/jsanalyze.h

@@ -114,6 +114,7 @@ class Bytecode
     bool arrayWriteHole: 1;  /* SETELEM which has written to an array hole. */
     bool getStringElement:1; /* GETELEM which has accessed string properties. */
     bool accessGetter: 1;    /* Property read on a shape with a getter hook. */
+    bool notIdempotent: 1;   /* Don't use an idempotent cache for this property read. */
 
     /* Stack depth before this opcode. */
     uint32_t stackDepth;

js/src/jsscript.h

@@ -540,6 +540,7 @@ struct JSScript : public js::gc::Cell
     bool            debugMode:1;      /* script was compiled in debug mode */
     bool            failedBoundsCheck:1; /* script has had hoisted bounds checks fail */
 #endif
+    bool            invalidatedIdempotentCache:1; /* idempotent cache has triggered invalidation */
     bool            callDestroyHook:1;/* need to call destroy hook */
     bool            isGenerator:1;    /* is a generator */
     bool            hasScriptCounts:1;/* script has an entry in

js/src/methodjit/PolyIC.cpp

@@ -752,6 +752,17 @@ struct GetPropHelper {
 namespace js {
 namespace mjit {
 
+inline void
+MarkNotIdempotent(JSScript *script, jsbytecode *pc)
+{
+    if (!script->hasAnalysis())
+        return;
+    analyze::Bytecode *code = script->analysis()->maybeCode(pc);
+    if (!code)
+        return;
+    code->notIdempotent = true;
+}
+
 class GetPropCompiler : public PICStubCompiler
 {
     JSObject    *obj;
@@ -1229,6 +1240,8 @@ class GetPropCompiler : public PICStubCompiler
 
         bool setStubShapeOffset = true;
         if (obj->isDenseArray()) {
+            MarkNotIdempotent(script, f.regs.pc);
+
             start = masm.label();
             shapeGuardJump = masm.branchPtr(Assembler::NotEqual,
                                             Address(pic.objReg, JSObject::offsetOfShape()),
@@ -1279,6 +1292,8 @@ class GetPropCompiler : public PICStubCompiler
         }
 
         if (!shape->hasDefaultGetter()) {
+            MarkNotIdempotent(script, f.regs.pc);
+
             if (shape->hasGetterValue()) {
                 generateNativeGetterStub(masm, shape, start, shapeMismatches);
             } else {
@@ -1367,8 +1382,10 @@ class GetPropCompiler : public PICStubCompiler
 
         GetPropHelper<GetPropCompiler> getprop(cx, obj, name, *this, f);
         LookupStatus status = getprop.lookupAndTest();
-        if (status != Lookup_Cacheable)
+        if (status != Lookup_Cacheable) {
+            MarkNotIdempotent(script, f.regs.pc);
             return status;
+        }
         if (hadGC())
             return Lookup_Uncacheable;