From 62e2cea370614e331b3bad8710f27e0fe39a3788 Mon Sep 17 00:00:00 2001 From: Brian Hackett Date: Fri, 16 May 2014 18:00:44 -0700 Subject: [PATCH] Bug 1008753 - Don't require shape checks when calling getters/setters on common prototypes, r=efaust. --- .../jit-test/tests/basic/redefinedGetter.js | 24 +++++++++ js/src/jit/BaselineInspector.cpp | 6 +-- js/src/jit/BaselineInspector.h | 4 +- js/src/jit/IonBuilder.cpp | 44 +++++++-------- js/src/jit/IonBuilder.h | 4 +- js/src/jit/MIR.h | 25 ++++----- js/src/jsinfer.cpp | 54 +++++++++++++++++++ js/src/jsinfer.h | 2 + js/src/vm/Shape.cpp | 12 +++++ 9 files changed, 130 insertions(+), 45 deletions(-) create mode 100644 js/src/jit-test/tests/basic/redefinedGetter.js diff --git a/js/src/jit-test/tests/basic/redefinedGetter.js b/js/src/jit-test/tests/basic/redefinedGetter.js new file mode 100644 index 00000000000..ecda67db27e --- /dev/null +++ b/js/src/jit-test/tests/basic/redefinedGetter.js @@ -0,0 +1,24 @@ + +function Thing() {} + +function foo(x, expected) { + for (var i = 0; i < 10; i++) + assertEq(x.f, expected); +} + +Object.defineProperty(Thing.prototype, "f", {get: function() { return 2; }, configurable:true}); +foo(new Thing(), 2); +Object.defineProperty(Thing.prototype, "f", {get: function() { return 3; }}); +foo(new Thing(), 3); + +function OtherThing() {} + +function bar(x, expected) { + for (var i = 0; i < 10; i++) + assertEq(x.f, expected); +} + +Object.defineProperty(OtherThing.prototype, "f", {get: function() { return 2; }, configurable:true}); +bar(new OtherThing(), 2); +delete OtherThing.prototype.f; +bar(new OtherThing(), undefined); diff --git a/js/src/jit/BaselineInspector.cpp b/js/src/jit/BaselineInspector.cpp index c35ba78e513..402cb9d68a2 100644 --- a/js/src/jit/BaselineInspector.cpp +++ b/js/src/jit/BaselineInspector.cpp @@ -474,7 +474,7 @@ BaselineInspector::templateCallObject() } JSObject * -BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonGetter) +BaselineInspector::commonGetPropFunction(jsbytecode *pc, JSFunction **commonGetter) { if (!hasBaselineScript()) return nullptr; @@ -486,7 +486,6 @@ BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, J stub->isGetProp_CallNativePrototype()) { ICGetPropCallGetter *nstub = static_cast(stub); - *lastProperty = nstub->holderShape(); *commonGetter = nstub->getter(); return nstub->holder(); } @@ -495,7 +494,7 @@ BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, J } JSObject * -BaselineInspector::commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonSetter) +BaselineInspector::commonSetPropFunction(jsbytecode *pc, JSFunction **commonSetter) { if (!hasBaselineScript()) return nullptr; @@ -504,7 +503,6 @@ BaselineInspector::commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, J for (ICStub *stub = entry.firstStub(); stub; stub = stub->next()) { if (stub->isSetProp_CallScripted() || stub->isSetProp_CallNative()) { ICSetPropCallSetter *nstub = static_cast(stub); - *lastProperty = nstub->holderShape(); *commonSetter = nstub->setter(); return nstub->holder(); } diff --git a/js/src/jit/BaselineInspector.h b/js/src/jit/BaselineInspector.h index 083c429d950..8c29fb4e45c 100644 --- a/js/src/jit/BaselineInspector.h +++ b/js/src/jit/BaselineInspector.h @@ -117,8 +117,8 @@ class BaselineInspector DeclEnvObject *templateDeclEnvObject(); CallObject *templateCallObject(); - JSObject *commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonGetter); - JSObject *commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonSetter); + JSObject *commonGetPropFunction(jsbytecode *pc, JSFunction **commonGetter); + JSObject *commonSetPropFunction(jsbytecode *pc, JSFunction **commonSetter); }; } // namespace jit diff --git a/js/src/jit/IonBuilder.cpp b/js/src/jit/IonBuilder.cpp index 4f723ec0e74..d92361c0ef7 100644 --- a/js/src/jit/IonBuilder.cpp +++ b/js/src/jit/IonBuilder.cpp @@ -8366,25 +8366,33 @@ IonBuilder::freezePropertiesForCommonPrototype(types::TemporaryTypeSet *types, P } } -inline MDefinition * +inline bool IonBuilder::testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name, - bool isGetter, JSObject *foundProto, Shape *lastProperty) + bool isGetter, JSObject *foundProto, JSFunction *function) { + types::TypeObjectKey *protoType = types::TypeObjectKey::get(foundProto); + + // The state change done below needs to be done on an object with singleton + // type, as otherwise we won't be able to tell whether it already has the + // correct getter/setter. + if (!protoType->isSingleObject() || protoType->unknownProperties()) + return false; + // Check if all objects being accessed will lookup the name through foundProto. if (!objectsHaveCommonPrototype(types, name, isGetter, foundProto)) - return nullptr; + return false; // We can optimize the getter/setter, so freeze all involved properties to // ensure there isn't a lower shadowing getter or setter installed in the // future. freezePropertiesForCommonPrototype(types, name, foundProto); - // Add a shape guard on the prototype we found the property on. The rest of - // the prototype chain is guarded by TI freezes. Note that a shape guard is - // good enough here, even in the proxy case, because we have ensured there - // are no lookup hooks for this property. - MInstruction *wrapper = constant(ObjectValue(*foundProto)); - return addShapeGuard(wrapper, lastProperty, Bailout_ShapeGuard); + // TI doesn't have accurate type information for properties with getters or + // setters, but we can use the state change machinery to watch out for the + // property being redefined on the prototype. + protoType->watchStateChangeForRedefinedProperty(constraints(), name, isGetter, function); + + return true; } bool @@ -8804,16 +8812,13 @@ IonBuilder::getPropTryCommonGetter(bool *emitted, MDefinition *obj, PropertyName { JS_ASSERT(*emitted == false); - Shape *lastProperty = nullptr; JSFunction *commonGetter = nullptr; - JSObject *foundProto = inspector->commonGetPropFunction(pc, &lastProperty, &commonGetter); + JSObject *foundProto = inspector->commonGetPropFunction(pc, &commonGetter); if (!foundProto) return true; types::TemporaryTypeSet *objTypes = obj->resultTypeSet(); - MDefinition *guard = testCommonGetterSetter(objTypes, name, /* isGetter = */ true, - foundProto, lastProperty); - if (!guard) + if (!testCommonGetterSetter(objTypes, name, /* isGetter = */ true, foundProto, commonGetter)) return true; bool isDOM = objTypes->isDOMClass(); @@ -8824,9 +8829,9 @@ IonBuilder::getPropTryCommonGetter(bool *emitted, MDefinition *obj, PropertyName if (jitinfo->isInSlot) { // We can't use MLoadFixedSlot here because it might not have the // right aliasing behavior; we want to alias DOM setters. - get = MGetDOMMember::New(alloc(), jitinfo, obj, guard); + get = MGetDOMMember::New(alloc(), jitinfo, obj); } else { - get = MGetDOMProperty::New(alloc(), jitinfo, obj, guard); + get = MGetDOMProperty::New(alloc(), jitinfo, obj); } current->add(get); current->push(get); @@ -9225,16 +9230,13 @@ IonBuilder::setPropTryCommonSetter(bool *emitted, MDefinition *obj, { JS_ASSERT(*emitted == false); - Shape *lastProperty = nullptr; JSFunction *commonSetter = nullptr; - JSObject *foundProto = inspector->commonSetPropFunction(pc, &lastProperty, &commonSetter); + JSObject *foundProto = inspector->commonSetPropFunction(pc, &commonSetter); if (!foundProto) return true; types::TemporaryTypeSet *objTypes = obj->resultTypeSet(); - MDefinition *guard = testCommonGetterSetter(objTypes, name, /* isGetter = */ false, - foundProto, lastProperty); - if (!guard) + if (!testCommonGetterSetter(objTypes, name, /* isGetter = */ false, foundProto, commonSetter)) return true; bool isDOM = objTypes->isDOMClass(); diff --git a/js/src/jit/IonBuilder.h b/js/src/jit/IonBuilder.h index 885d03be310..c14cc85e29c 100644 --- a/js/src/jit/IonBuilder.h +++ b/js/src/jit/IonBuilder.h @@ -763,8 +763,8 @@ class IonBuilder : public MIRGenerator bool isGetter, JSObject *foundProto); void freezePropertiesForCommonPrototype(types::TemporaryTypeSet *types, PropertyName *name, JSObject *foundProto); - MDefinition *testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name, - bool isGetter, JSObject *foundProto, Shape *lastProperty); + bool testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name, + bool isGetter, JSObject *foundProto, JSFunction *function); bool testShouldDOMCall(types::TypeSet *inTypes, JSFunction *func, JSJitInfo::OpType opType); diff --git a/js/src/jit/MIR.h b/js/src/jit/MIR.h index b32ab6093dd..f7ad28072ae 100644 --- a/js/src/jit/MIR.h +++ b/js/src/jit/MIR.h @@ -8489,23 +8489,18 @@ class MSetDOMProperty }; class MGetDOMProperty - : public MAryInstruction<2>, + : public MUnaryInstruction, public ObjectPolicy<0> { const JSJitInfo *info_; protected: - MGetDOMProperty(const JSJitInfo *jitinfo, MDefinition *obj, MDefinition *guard) - : info_(jitinfo) + MGetDOMProperty(const JSJitInfo *jitinfo, MDefinition *obj) + : MUnaryInstruction(obj), info_(jitinfo) { JS_ASSERT(jitinfo); JS_ASSERT(jitinfo->type() == JSJitInfo::Getter); - setOperand(0, obj); - - // Pin the guard as an operand if we want to hoist later - setOperand(1, guard); - // We are movable iff the jitinfo says we can be. if (isDomMovable()) { JS_ASSERT(jitinfo->aliasSet() != JSJitInfo::AliasEverything); @@ -8527,10 +8522,9 @@ class MGetDOMProperty public: INSTRUCTION_HEADER(GetDOMProperty) - static MGetDOMProperty *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj, - MDefinition *guard) + static MGetDOMProperty *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj) { - return new(alloc) MGetDOMProperty(info, obj, guard); + return new(alloc) MGetDOMProperty(info, obj); } const JSJitGetterOp fun() { @@ -8589,18 +8583,17 @@ class MGetDOMProperty class MGetDOMMember : public MGetDOMProperty { // We inherit everything from MGetDOMProperty except our possiblyCalls value - MGetDOMMember(const JSJitInfo *jitinfo, MDefinition *obj, MDefinition *guard) - : MGetDOMProperty(jitinfo, obj, guard) + MGetDOMMember(const JSJitInfo *jitinfo, MDefinition *obj) + : MGetDOMProperty(jitinfo, obj) { } public: INSTRUCTION_HEADER(GetDOMMember) - static MGetDOMMember *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj, - MDefinition *guard) + static MGetDOMMember *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj) { - return new(alloc) MGetDOMMember(info, obj, guard); + return new(alloc) MGetDOMMember(info, obj); } bool possiblyCalls() const { diff --git a/js/src/jsinfer.cpp b/js/src/jsinfer.cpp index 808fdbcda2f..323aab6c2fa 100644 --- a/js/src/jsinfer.cpp +++ b/js/src/jsinfer.cpp @@ -1627,6 +1627,47 @@ class ConstraintDataFreezeObjectForTypedArrayBuffer } }; +// Constraint which triggers recompilation when a getter or setter property on +// an object is redefined. +class ConstraintDataFreezeObjectForGetterSetter +{ + PropertyName *name; + bool isGetter; + JSFunction *function; + + public: + ConstraintDataFreezeObjectForGetterSetter(PropertyName *name, bool isGetter, JSFunction *function) + : name(name), isGetter(isGetter), function(function) + {} + + const char *kind() { return "freezeObjectForGetterSetter"; } + + bool invalidateOnNewType(Type type) { return false; } + bool invalidateOnNewPropertyState(TypeSet *property) { return false; } + bool invalidateOnNewObjectState(TypeObject *object) { + Shape *shape = object->singleton()->nativeLookupPure(name); + if (!shape) + return true; + + if (isGetter) + return !shape->hasGetterValue() || shape->getterObject() != function; + return !shape->hasSetterValue() || shape->setterObject() != function; + } + + bool constraintHolds(JSContext *cx, + const HeapTypeSetKey &property, TemporaryTypeSet *expected) + { + return !invalidateOnNewObjectState(property.object()->maybeType()); + } + + bool shouldSweep() { + // Note: |name| may be used for looking up properties on the object, + // but if it becomes dead then the property will have been deleted and + // the associated jitcode invalidated. + return false; + } +}; + } /* anonymous namespace */ void @@ -1663,6 +1704,19 @@ TypeObjectKey::watchStateChangeForTypedArrayBuffer(CompilerConstraintList *const ConstraintDataFreezeObjectForTypedArrayBuffer(viewData))); } +void +TypeObjectKey::watchStateChangeForRedefinedProperty(CompilerConstraintList *constraints, + PropertyName *name, bool isGetter, + JSFunction *function) +{ + HeapTypeSetKey objectProperty = property(JSID_EMPTY); + LifoAlloc *alloc = constraints->alloc(); + + typedef CompilerConstraintInstance T; + constraints->add(alloc->new_(alloc, objectProperty, + ConstraintDataFreezeObjectForGetterSetter(name, isGetter, function))); +} + static void ObjectStateChange(ExclusiveContext *cxArg, TypeObject *object, bool markingUnknown) { diff --git a/js/src/jsinfer.h b/js/src/jsinfer.h index fcdbe4ae2a7..6002df2861a 100644 --- a/js/src/jsinfer.h +++ b/js/src/jsinfer.h @@ -1424,6 +1424,8 @@ struct TypeObjectKey void watchStateChangeForInlinedCall(CompilerConstraintList *constraints); void watchStateChangeForNewScriptTemplate(CompilerConstraintList *constraints); void watchStateChangeForTypedArrayBuffer(CompilerConstraintList *constraints); + void watchStateChangeForRedefinedProperty(CompilerConstraintList *constraints, + PropertyName *name, bool isGetter, JSFunction *function); HeapTypeSetKey property(jsid id); void ensureTrackedProperty(JSContext *cx, jsid id); diff --git a/js/src/vm/Shape.cpp b/js/src/vm/Shape.cpp index 9a03cfb894f..99ae07b9c7e 100644 --- a/js/src/vm/Shape.cpp +++ b/js/src/vm/Shape.cpp @@ -866,6 +866,10 @@ JSObject::putProperty(typename ExecutionModeTraits::ExclusiveContextType c JS_ASSERT_IF(shape->hasSlot() && !(attrs & JSPROP_SHARED), shape->slot() == slot); + bool wasGetterOrSetter = shape->hasGetterValue() || shape->hasSetterValue(); + if (mode == ParallelExecution && wasGetterOrSetter) + return nullptr; + if (obj->inDictionaryMode()) { /* * Updating some property in a dictionary-mode object. Create a new @@ -935,6 +939,10 @@ JSObject::putProperty(typename ExecutionModeTraits::ExclusiveContextType c ++cx->asJSContext()->runtime()->propertyRemovals; } + // Inform type inference about any getter or setter property being changed. + if (wasGetterOrSetter) + types::MarkObjectStateChange(cx->asExclusiveContext(), obj); + obj->checkShapeConsistency(); return shape; @@ -1125,6 +1133,10 @@ JSObject::removeProperty(ExclusiveContext *cx, jsid id_) self->removeLastProperty(cx); } + // Inform type inference about any getter or setter property being deleted. + if (shape->hasGetterValue() || shape->hasSetterValue()) + types::MarkObjectStateChange(cx, self); + self->checkShapeConsistency(); return true; }