Bug 1703469: Guard on script instead of function for non-first getter/setters r=jandem

Caching the callee inside the CacheIRWriter seemed simpler than trying to rewrite all the callers of `emitGuardGetterSetterSlot` / `emitCallGetterResultGuards` / etc to conditionally return an operand for the callee (only when scripted).

LoadGetterSetterFunction unfortunately has to guard that the result is a function, because it is possible to create a GetterSetter containing other types. We do enforce that the accessor function must be callable, but that allows bound functions and callable proxies. We could consider adding a shape flag for objects that have a non-JSFunction getter/setter, or alternatively low-bit tagging the setter if the GetterSetter contains a non-JSFunction, to avoid the class guard. Removing the class guard is about a 15-25% improvement on the microbenchmark in bug 1962778. We would get all that improvement with a shape flag, or most of that improvement with a low-bit tag. We could also consider a fuse.

Differential Revision: https://phabricator.services.mozilla.com/D251756

Author: iainireland

js/src/jit-test/tests/cacheir/non-function-getters.js

@@ -0,0 +1,41 @@
+with ({}) {}
+
+function makeObjWithFunctionGetter(n) {
+  var o = {};
+  Object.defineProperty(o, "x", {
+    get() { return n; }
+  });
+
+  return o;
+}
+
+function makeObjWithProxyGetter() {
+  var inner = () => "proxy";
+  var proxy = new Proxy(inner, {});
+
+  var o = {};
+  Object.defineProperty(o, "x", {
+    get: proxy
+  });
+  return o;
+}
+
+function makeObjWithBoundGetter() {
+  var inner = () => "bound";;
+  var bound = inner.bind({});
+
+  var o = {};
+  Object.defineProperty(o, "x", {
+    get: bound
+  });
+  return o;
+}
+
+function foo(o) { return o.x; }
+
+for (var i = 0; i < 100; i++) {
+  foo(makeObjWithFunctionGetter(i));
+}
+
+assertEq(foo(makeObjWithProxyGetter()), "proxy");
+assertEq(foo(makeObjWithBoundGetter()), "bound");

js/src/jit/CacheIR.cpp

@@ -937,6 +937,16 @@ static void EmitMissingPropResult(CacheIRWriter& writer, NativeObject* obj,
   writer.loadUndefinedResult();
 }
 
+static ValOperandId EmitLoadSlot(CacheIRWriter& writer, NativeObject* holder,
+                                 ObjOperandId holderId, uint32_t slot) {
+  if (holder->isFixedSlot(slot)) {
+    return writer.loadFixedSlot(holderId,
+                                NativeObject::getFixedSlotOffset(slot));
+  }
+  size_t dynamicSlotIndex = holder->dynamicSlotIndex(slot);
+  return writer.loadDynamicSlot(holderId, dynamicSlotIndex);
+}
+
 void IRGenerator::emitCallGetterResultNoGuards(NativeGetPropKind kind,
                                                NativeObject* obj,
                                                NativeObject* holder,
@@ -971,6 +981,7 @@ void IRGenerator::emitCallGetterResultNoGuards(NativeGetPropKind kind,
 void IRGenerator::emitGuardGetterSetterSlot(NativeObject* holder,
                                             PropertyInfo prop,
                                             ObjOperandId holderId,
+                                            AccessorKind kind,
                                             bool holderIsConstant) {
   // If the holder is guaranteed to be the same object, and it never had a
   // slot holding a GetterSetter mutated or deleted, its Shape will change when
@@ -980,6 +991,25 @@ void IRGenerator::emitGuardGetterSetterSlot(NativeObject* holder,
   }
 
   size_t slot = prop.slot();
+
+  // For the same reasons as emitCalleeGuard, we guard on the BaseScript
+  // instead of the GetterSetter if the callee is scripted and this isn't
+  // the first IC stub.
+  if (!isFirstStub_) {
+    bool isGetter = kind == AccessorKind::Getter;
+    JSObject* accessor = isGetter ? holder->getGetter(prop)
+                                  : holder->getSetter(prop);
+    JSFunction* fun = &accessor->as<JSFunction>();
+    if (fun->hasBaseScript()) {
+      ValOperandId getterSetterId = EmitLoadSlot(writer, holder, holderId, slot);
+      ObjOperandId functionId = writer.loadGetterSetterFunction(getterSetterId,
+                                                                isGetter);
+      writer.saveScriptedGetterSetterCallee(functionId);
+      writer.guardFunctionScript(functionId, fun->baseScript());
+      return;
+    }
+  }
+
   Value slotVal = holder->getSlot(slot);
   MOZ_ASSERT(slotVal.isPrivateGCThing());
 
@@ -1014,9 +1044,10 @@ void GetPropIRGenerator::emitCallGetterResultGuards(NativeObject* obj,
       TestMatchingHolder(writer, holder, holderId);
 
       emitGuardGetterSetterSlot(holder, prop, holderId,
+                                AccessorKind::Getter,
                                 /* holderIsConstant = */ true);
     } else {
-      emitGuardGetterSetterSlot(holder, prop, objId);
+      emitGuardGetterSetterSlot(holder, prop, objId, AccessorKind::Getter);
     }
   } else {
     GetterSetter* gs = holder->getGetterSetter(prop);
@@ -1114,16 +1145,6 @@ void GetPropIRGenerator::emitCallDOMGetterResult(NativeObject* obj,
   emitCallDOMGetterResultNoGuards(holder, prop, objId);
 }
 
-static ValOperandId EmitLoadSlot(CacheIRWriter& writer, NativeObject* holder,
-                                 ObjOperandId holderId, uint32_t slot) {
-  if (holder->isFixedSlot(slot)) {
-    return writer.loadFixedSlot(holderId,
-                                NativeObject::getFixedSlotOffset(slot));
-  }
-  size_t dynamicSlotIndex = holder->dynamicSlotIndex(slot);
-  return writer.loadDynamicSlot(holderId, dynamicSlotIndex);
-}
-
 void GetPropIRGenerator::attachMegamorphicNativeSlot(ObjOperandId objId,
                                                      jsid id) {
   MOZ_ASSERT(mode_ == ICState::Mode::Megamorphic);
@@ -1822,7 +1843,8 @@ AttachDecision GetPropIRGenerator::tryAttachDOMProxyExpando(
     // and not the expando object.
     MOZ_ASSERT(kind == NativeGetPropKind::NativeGetter ||
                kind == NativeGetPropKind::ScriptedGetter);
-    emitGuardGetterSetterSlot(nativeExpandoObj, *prop, expandoObjId);
+    emitGuardGetterSetterSlot(nativeExpandoObj, *prop, expandoObjId,
+                              AccessorKind::Getter);
     emitCallGetterResultNoGuards(kind, nativeExpandoObj, nativeExpandoObj,
                                  *prop, receiverId);
   }
@@ -1972,6 +1994,7 @@ AttachDecision GetPropIRGenerator::tryAttachDOMProxyUnshadowed(
                  kind == NativeGetPropKind::ScriptedGetter);
       MOZ_ASSERT(!isSuper());
       emitGuardGetterSetterSlot(holder, *prop, holderId,
+                                AccessorKind::Getter,
                                 /* holderIsConstant = */ true);
       emitCallGetterResultNoGuards(kind, nativeProtoObj, holder, *prop,
                                    receiverId);
@@ -3625,11 +3648,13 @@ AttachDecision GetNameIRGenerator::tryAttachGlobalNameGetter(ObjOperandId objId,
     ObjOperandId holderId = writer.loadObject(holder);
     writer.guardShape(holderId, holder->shape());
     emitGuardGetterSetterSlot(holder, *prop, holderId,
+                              AccessorKind::Getter,
                               /* holderIsConstant = */ true);
   } else {
     // Note: pass true for |holderIsConstant| because the holder must be the
     // current global object.
     emitGuardGetterSetterSlot(holder, *prop, globalId,
+                              AccessorKind::Getter,
                               /* holderIsConstant = */ true);
   }
 
@@ -4876,9 +4901,10 @@ AttachDecision SetPropIRGenerator::tryAttachSetter(HandleObject obj,
       TestMatchingHolder(writer, holder, holderId);
 
       emitGuardGetterSetterSlot(holder, *prop, holderId,
+                                AccessorKind::Setter,
                                 /* holderIsConstant = */ true);
     } else {
-      emitGuardGetterSetterSlot(holder, *prop, objId);
+      emitGuardGetterSetterSlot(holder, *prop, objId, AccessorKind::Setter);
     }
   } else {
     GetterSetter* gs = holder->getGetterSetter(*prop);
@@ -5340,7 +5366,7 @@ AttachDecision SetPropIRGenerator::tryAttachDOMProxyUnshadowed(
   ObjOperandId holderId = writer.loadObject(holder);
   TestMatchingHolder(writer, holder, holderId);
 
-  emitGuardGetterSetterSlot(holder, *prop, holderId,
+  emitGuardGetterSetterSlot(holder, *prop, holderId, AccessorKind::Setter,
                             /* holderIsConstant = */ true);
 
   // EmitCallSetterNoGuards expects |obj| to be the object the property is
@@ -5398,7 +5424,8 @@ AttachDecision SetPropIRGenerator::tryAttachDOMProxyExpando(
         obj, objId, expandoVal, nativeExpandoObj);
 
     MOZ_ASSERT(holder == nativeExpandoObj);
-    emitGuardGetterSetterSlot(nativeExpandoObj, *prop, expandoObjId);
+    emitGuardGetterSetterSlot(nativeExpandoObj, *prop, expandoObjId,
+                              AccessorKind::Setter);
     emitCallSetterNoGuards(nativeExpandoObj, nativeExpandoObj, *prop, objId,
                            rhsId);
     trackAttached("SetProp.DOMProxyExpandoSetter");

js/src/jit/CacheIRCompiler.cpp

@@ -9312,6 +9312,32 @@ bool CacheIRCompiler::emitMegamorphicStoreSlot(ObjOperandId objId,
   return true;
 }
 
+bool CacheIRCompiler::emitLoadGetterSetterFunction(ValOperandId getterSetterId,
+                                                   bool isGetter,
+                                                   ObjOperandId resultId) {
+  JitSpew(JitSpew_Codegen, "%s", __FUNCTION__);
+
+  ValueOperand getterSetter = allocator.useValueRegister(masm, getterSetterId);
+  Register output = allocator.defineRegister(masm, resultId);
+  AutoScratchRegister scratch(allocator, masm);
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  masm.unboxNonDouble(getterSetter, output, JSVAL_TYPE_PRIVATE_GCTHING);
+
+  size_t offset = isGetter ? GetterSetter::offsetOfGetter() : GetterSetter::offsetOfSetter();
+  masm.loadPtr(Address(output, offset), output);
+
+  masm.branchTestPtr(Assembler::Zero, output, output, failure->label());
+  masm.branchTestObjIsFunction(Assembler::NotEqual, output, scratch, output,
+                               failure->label());
+  return true;
+}
+
+
 bool CacheIRCompiler::emitGuardHasGetterSetter(ObjOperandId objId,
                                                uint32_t idOffset,
                                                uint32_t getterSetterOffset) {

js/src/jit/CacheIRGenerator.h

@@ -107,8 +107,10 @@ class MOZ_RAII IRGenerator {
   void emitOptimisticClassGuard(ObjOperandId objId, JSObject* obj,
                                 GuardClassKind kind);
 
+  enum class AccessorKind { Getter, Setter };
   void emitGuardGetterSetterSlot(NativeObject* holder, PropertyInfo prop,
                                  ObjOperandId holderId,
+                                 AccessorKind kind,
                                  bool holderIsConstant = false);
   void emitCallGetterResultNoGuards(NativeGetPropKind kind, NativeObject* obj,
                                     NativeObject* holder, PropertyInfo prop,

js/src/jit/CacheIROps.yaml

@@ -616,6 +616,17 @@
     boolean: BooleanId
     result: NumberId
 
+# Load the getter or setter out of a GetterSetter.
+# Fails if it is null.
+- name: LoadGetterSetterFunction
+  shared: true
+  transpile: true
+  cost_estimate: 1
+  args:
+    getterSetter: ValId
+    isGetter: BoolImm
+    result: ObjId
+
 - name: GuardHasGetterSetter
   shared: true
   transpile: true

js/src/jit/CacheIRWriter.h

@@ -102,6 +102,8 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
   // instruction.
   TrialInliningState trialInliningState_ = TrialInliningState::Failure;
 
+  ObjOperandId savedScriptedGetterSetterCallee_;
+
   // Basic caching to avoid quadatic lookup behaviour in readStubField.
   mutable uint32_t lastOffset_;
   mutable uint32_t lastIndex_;
@@ -618,8 +620,20 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
     callClassHook_(calleeId, argc, flags, argcFixed, target);
   }
 
+  void saveScriptedGetterSetterCallee(ObjOperandId callee) {
+    MOZ_ASSERT(!hasSavedScriptedGetterSetterCallee());
+    savedScriptedGetterSetterCallee_ = callee;
+  }
+
  private:
+  bool hasSavedScriptedGetterSetterCallee() {
+    return savedScriptedGetterSetterCallee_.valid();
+  }
+
   ObjOperandId getterSetterCalleeOperand(JSFunction* target) {
+    if (hasSavedScriptedGetterSetterCallee()) {
+      return savedScriptedGetterSetterCallee_;
+    }
     return loadObject(target);
   }
  public:
@@ -637,6 +651,7 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
                                JSFunction* getter, ICScript* icScript,
                                bool sameRealm) {
     MOZ_ASSERT(getter->hasJitEntry());
+    MOZ_ASSERT(!hasSavedScriptedGetterSetterCallee());
     uint32_t nargsAndFlags = getter->flagsAndArgCountRaw();
     callInlinedGetterResult_(receiver, callee, icScript, sameRealm,
                              nargsAndFlags);
@@ -646,6 +661,7 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
   void callNativeGetterResult(ValOperandId receiver, JSFunction* getter,
                               bool sameRealm) {
     MOZ_ASSERT(getter->isNativeWithoutJitEntry());
+    MOZ_ASSERT(!hasSavedScriptedGetterSetterCallee());
     uint32_t nargsAndFlags = getter->flagsAndArgCountRaw();
     callNativeGetterResult_(receiver, getter, sameRealm, nargsAndFlags);
   }
@@ -663,6 +679,7 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
                          JSFunction* setter, ValOperandId rhs,
                          ICScript* icScript, bool sameRealm) {
     MOZ_ASSERT(setter->hasJitEntry());
+    MOZ_ASSERT(!hasSavedScriptedGetterSetterCallee());
     uint32_t nargsAndFlags = setter->flagsAndArgCountRaw();
     callInlinedSetter_(receiver, callee, rhs, icScript, sameRealm,
                        nargsAndFlags);
@@ -672,6 +689,7 @@ class MOZ_RAII CacheIRWriter : public JS::CustomAutoRooter {
   void callNativeSetter(ObjOperandId receiver, JSFunction* setter,
                         ValOperandId rhs, bool sameRealm) {
     MOZ_ASSERT(setter->isNativeWithoutJitEntry());
+    MOZ_ASSERT(!hasSavedScriptedGetterSetterCallee());
     uint32_t nargsAndFlags = setter->flagsAndArgCountRaw();
     callNativeSetter_(receiver, setter, rhs, sameRealm, nargsAndFlags);
   }

js/src/jit/CodeGenerator.cpp

@@ -21109,6 +21109,24 @@ void CodeGenerator::visitLoadWrapperTarget(LLoadWrapperTarget* lir) {
   }
 }
 
+void CodeGenerator::visitLoadGetterSetterFunction(LLoadGetterSetterFunction* lir) {
+  ValueOperand getterSetter = ToValue(lir->getterSetter());
+  Register output = ToRegister(lir->output());
+  Register temp = ToRegister(lir->temp0());
+
+  masm.unboxNonDouble(getterSetter, output, JSVAL_TYPE_PRIVATE_GCTHING);
+
+  size_t offset = lir->mir()->isGetter() ? GetterSetter::offsetOfGetter()
+                                         : GetterSetter::offsetOfSetter();
+  masm.loadPtr(Address(output, offset), output);
+
+  Label bail;
+  masm.branchTestPtr(Assembler::Zero, output, output, &bail);
+  masm.branchTestObjIsFunction(Assembler::NotEqual, output, temp, output,
+                               &bail);
+  bailoutFrom(&bail, lir->snapshot());
+}
+
 void CodeGenerator::visitGuardHasGetterSetter(LGuardHasGetterSetter* lir) {
   Register object = ToRegister(lir->object());
   Register temp0 = ToRegister(lir->temp0());

js/src/jit/Lowering.cpp

@@ -7466,6 +7466,15 @@ void LIRGenerator::visitLoadWrapperTarget(MLoadWrapperTarget* ins) {
   define(lir, ins);
 }
 
+void LIRGenerator::visitLoadGetterSetterFunction(MLoadGetterSetterFunction* ins) {
+  MDefinition* getterSetter = ins->getterSetter();
+
+  auto* lir = new (alloc())
+      LLoadGetterSetterFunction(useBoxAtStart(getterSetter), temp());
+  assignSnapshot(lir, ins->bailoutKind());
+  define(lir, ins);
+}
+
 void LIRGenerator::visitGuardHasGetterSetter(MGuardHasGetterSetter* ins) {
   MDefinition* object = ins->object();
   MOZ_ASSERT(object->type() == MIRType::Object);

js/src/jit/MIROps.yaml

@@ -3285,6 +3285,16 @@
   alias_set: custom
   generate_lir: true
 
+- name: LoadGetterSetterFunction
+  operands:
+    getterSetter: Value
+  arguments:
+    isGetter: bool
+  result_type: Object
+  generate_lir: true
+  lir_temps: 1
+  alias_set: none
+
 # Guard the accessor shape is present on the object or its prototype chain.
 - name: GuardHasGetterSetter
   operands: