don't double execute descriptor on specialization fail

carljm · Apr 14, 2023 · 5627d66 · 5627d66
1 parent b73cff0
commit 5627d66
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 41 deletions.
diff --git a/Include/internal/pycore_typeobject.h b/Include/internal/pycore_typeobject.h
@@ -100,6 +100,8 @@ PyObject *_Py_slot_tp_getattr_hook(PyObject *self, PyObject *name);
 
 PyObject *
 _PySuper_Lookup(PyTypeObject *su_type, PyObject *su_obj, PyObject *name, int *meth_found);
+PyObject *
+_PySuper_LookupDescr(PyTypeObject *su_type, PyObject *su_obj, PyObject *name);
 
 #ifdef __cplusplus
 }

diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py
@@ -1,6 +1,29 @@
 import unittest
 
 
+class TestLoadSuperAttrCache(unittest.TestCase):
+    def test_descriptor_not_double_executed_on_spec_fail(self):
+        calls = []
+        class Descriptor:
+            def __get__(self, instance, owner):
+                calls.append((instance, owner))
+                return lambda: 1
+
+        class C:
+            d = Descriptor()
+
+        class D(C):
+            def f(self):
+                return super().d()
+
+        d = D()
+
+        self.assertEqual(d.f(), 1)  # warmup
+        calls.clear()
+        self.assertEqual(d.f(), 1)  # try to specialize
+        self.assertEqual(calls, [(d, D)])
+
+
 class TestLoadAttrCache(unittest.TestCase):
     def test_descriptor_added_after_optimization(self):
         class Descriptor:

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
@@ -9357,29 +9357,26 @@ super_repr(PyObject *self)
             su->type ? su->type->tp_name : "NULL");
 }
 
+/* Do a super lookup without executing descriptors or falling back to getattr
+on the super object itself.
+
+May return NULL with or without an exception set, like PyDict_GetItemWithError. */
 static PyObject *
-do_super_lookup(superobject *su, PyTypeObject *su_type, PyObject *su_obj,
-                PyTypeObject *su_obj_type, PyObject *name, int *meth_found)
+_super_lookup_descr(PyTypeObject *su_type, PyTypeObject *su_obj_type, PyObject *name)
 {
-    PyTypeObject *starttype;
     PyObject *mro, *res;
     Py_ssize_t i, n;
-    int temp_su = 0;
-
-    starttype = su_obj_type;
-    if (starttype == NULL)
-        goto skip;
 
     /* We want __class__ to return the class of the super object
        (i.e. super, or a subclass), not the class of su->obj. */
     if (PyUnicode_Check(name) &&
         PyUnicode_GET_LENGTH(name) == 9 &&
         _PyUnicode_Equal(name, &_Py_ID(__class__)))
-        goto skip;
+        return NULL;
 
-    mro = starttype->tp_mro;
+    mro = su_obj_type->tp_mro;
     if (mro == NULL)
-        goto skip;
+        return NULL;
 
     assert(PyTuple_Check(mro));
     n = PyTuple_GET_SIZE(mro);
@@ -9391,9 +9388,9 @@ do_super_lookup(superobject *su, PyTypeObject *su_type, PyObject *su_obj,
     }
     i++;  /* skip su->type (if any)  */
     if (i >= n)
-        goto skip;
+        return NULL;
 
-    /* keep a strong reference to mro because starttype->tp_mro can be
+    /* keep a strong reference to mro because su_obj_type->tp_mro can be
        replaced during PyDict_GetItemWithError(dict, name)  */
     Py_INCREF(mro);
     do {
@@ -9404,22 +9401,6 @@ do_super_lookup(superobject *su, PyTypeObject *su_type, PyObject *su_obj,
         res = PyDict_GetItemWithError(dict, name);
         if (res != NULL) {
             Py_INCREF(res);
-            if (meth_found && _PyType_HasFeature(Py_TYPE(res), Py_TPFLAGS_METHOD_DESCRIPTOR)) {
-                *meth_found = 1;
-            }
-            else {
-                descrgetfunc f = Py_TYPE(res)->tp_descr_get;
-                if (f != NULL) {
-                    PyObject *res2;
-                    res2 = f(res,
-                        /* Only pass 'obj' param if this is instance-mode super
-                        (See SF ID #743627)  */
-                        (su_obj == (PyObject *)starttype) ? NULL : su_obj,
-                        (PyObject *)starttype);
-                    Py_SETREF(res, res2);
-                }
-            }
-
             Py_DECREF(mro);
             return res;
         }
@@ -9431,6 +9412,43 @@ do_super_lookup(superobject *su, PyTypeObject *su_type, PyObject *su_obj,
         i++;
     } while (i < n);
     Py_DECREF(mro);
+    return NULL;
+}
+
+static PyObject *
+do_super_lookup(superobject *su, PyTypeObject *su_type, PyObject *su_obj,
+                PyTypeObject *su_obj_type, PyObject *name, int *meth_found)
+{
+    PyObject *res;
+    int temp_su = 0;
+
+    if (su_obj_type == NULL) {
+        goto skip;
+    }
+
+    res = _super_lookup_descr(su_type, su_obj_type, name);
+    if (res != NULL) {
+        if (meth_found && _PyType_HasFeature(Py_TYPE(res), Py_TPFLAGS_METHOD_DESCRIPTOR)) {
+            *meth_found = 1;
+        }
+        else {
+            descrgetfunc f = Py_TYPE(res)->tp_descr_get;
+            if (f != NULL) {
+                PyObject *res2;
+                res2 = f(res,
+                    /* Only pass 'obj' param if this is instance-mode super
+                    (See SF ID #743627)  */
+                    (su_obj == (PyObject *)su_obj_type) ? NULL : su_obj,
+                    (PyObject *)su_obj_type);
+                Py_SETREF(res, res2);
+            }
+        }
+
+        return res;
+    }
+    else if (PyErr_Occurred()) {
+        return NULL;
+    }
 
   skip:
     if (su == NULL) {
@@ -9520,6 +9538,18 @@ _PySuper_Lookup(PyTypeObject *su_type, PyObject *su_obj, PyObject *name, int *me
     return res;
 }
 
+PyObject *
+_PySuper_LookupDescr(PyTypeObject *su_type, PyObject *su_obj, PyObject *name)
+{
+    PyTypeObject *su_obj_type = supercheck(su_type, su_obj);
+    if (su_obj_type == NULL) {
+        return NULL;
+    }
+    PyObject *res = _super_lookup_descr(su_type, su_obj_type, name);
+    Py_DECREF(su_obj_type);
+    return res;
+}
+
 static PyObject *
 super_descr_get(PyObject *self, PyObject *obj, PyObject *type)
 {

diff --git a/Python/specialize.c b/Python/specialize.c
@@ -327,7 +327,7 @@ _PyCode_Quicken(PyCodeObject *code)
 #define SPEC_FAIL_SUPER_BAD_CLASS 10
 #define SPEC_FAIL_SUPER_SHADOWED 11
 #define SPEC_FAIL_SUPER_NOT_METHOD 12
-#define SPEC_FAIL_SUPER_ERROR 13
+#define SPEC_FAIL_SUPER_ERROR_OR_NOT_FOUND 13
 
 /* Attributes */
 
@@ -533,22 +533,20 @@ _Py_Specialize_LoadSuperAttr(PyObject *global_super, PyObject *class, PyObject *
         goto fail;
     }
     PyTypeObject *tp = (PyTypeObject *)class;
-    int meth_found = 0;
-    PyObject *res = _PySuper_Lookup(tp, self, name, &meth_found);
+    PyObject *res = _PySuper_LookupDescr(tp, self, name);
     if (res == NULL) {
-        SPECIALIZATION_FAIL(LOAD_SUPER_ATTR, SPEC_FAIL_SUPER_ERROR);
+        SPECIALIZATION_FAIL(LOAD_SUPER_ATTR, SPEC_FAIL_SUPER_ERROR_OR_NOT_FOUND);
         PyErr_Clear();
         goto fail;
     }
-    if (!meth_found) {
-        SPECIALIZATION_FAIL(LOAD_SUPER_ATTR, SPEC_FAIL_SUPER_NOT_METHOD);
-        goto fail;
+    if (_PyType_HasFeature(Py_TYPE(res), Py_TPFLAGS_METHOD_DESCRIPTOR)) {
+        write_u32(cache->class_version, tp->tp_version_tag);
+        write_u32(cache->self_type_version, Py_TYPE(self)->tp_version_tag);
+        write_obj(cache->method, res);  // borrowed
+        instr->op.code = LOAD_SUPER_ATTR_METHOD;
+        goto success;
     }
-    write_u32(cache->class_version, tp->tp_version_tag);
-    write_u32(cache->self_type_version, Py_TYPE(self)->tp_version_tag);
-    write_obj(cache->method, res);  // borrowed
-    instr->op.code = LOAD_SUPER_ATTR_METHOD;
-    goto success;
+    SPECIALIZATION_FAIL(LOAD_SUPER_ATTR, SPEC_FAIL_SUPER_NOT_METHOD);
 
 fail:
     STAT_INC(LOAD_SUPER_ATTR, failure);