From 00d7e574c8b29cf401be0e4dd59c9a4527c26cee Mon Sep 17 00:00:00 2001 From: Dorian Hoxha Date: Tue, 1 Sep 2026 12:44:56 +0200 Subject: [PATCH] Restore _GUARD_KEYS_VERSION in LOAD_ATTR specializations The 3.14 and 3.15 interpreters were missing the shared-keys version check in LOAD_ATTR_METHOD_WITH_VALUES and LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES, so a warmed load site kept returning the cached class attribute after an instance stored the attribute through the type's shared keys. That is what breaks SQLAlchemy's deferred column loads in gh-115. Meta Python invalidates the type version when shared keys change, so _GUARD_TYPE_VERSION covers it there; the dk_version check is under #ifndef META_PYTHON. --- .../3.14/Includes/generated_cases.c.h | 34 +++++++++- cinderx/Interpreter/3.14/cinder-bytecodes.c | 27 ++++++++ .../3.15/Includes/generated_cases.c.h | 34 +++++++++- cinderx/Interpreter/3.15/cinder-bytecodes.c | 21 ++++++ .../test_cinderx/test_jit_specialization.py | 67 ++++++++++++++++++- 5 files changed, 178 insertions(+), 5 deletions(-) diff --git a/cinderx/Interpreter/3.14/Includes/generated_cases.c.h b/cinderx/Interpreter/3.14/Includes/generated_cases.c.h index a0c487de2..e3e3921b4 100644 --- a/cinderx/Interpreter/3.14/Includes/generated_cases.c.h +++ b/cinderx/Interpreter/3.14/Includes/generated_cases.c.h @@ -10059,7 +10059,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - /* Skip 2 cache entries */ + // _GUARD_KEYS_VERSION + { + uint32_t keys_version = read_u32(&this_instr[4].cache); + #ifndef META_PYTHON + PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + #else + (void)keys_version; + #endif + } // _LOAD_ATTR_METHOD_WITH_VALUES { PyObject *descr = read_obj(&this_instr[6].cache); @@ -10242,7 +10257,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - /* Skip 2 cache entries */ + // _GUARD_KEYS_VERSION + { + uint32_t keys_version = read_u32(&this_instr[4].cache); + #ifndef META_PYTHON + PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + #else + (void)keys_version; + #endif + } // _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES { PyObject *descr = read_obj(&this_instr[6].cache); diff --git a/cinderx/Interpreter/3.14/cinder-bytecodes.c b/cinderx/Interpreter/3.14/cinder-bytecodes.c index 885d1610d..05dcf9e0e 100644 --- a/cinderx/Interpreter/3.14/cinder-bytecodes.c +++ b/cinderx/Interpreter/3.14/cinder-bytecodes.c @@ -583,6 +583,33 @@ dummy_func( #endif } + // Meta Python bumps the type version when shared keys change, so + // _GUARD_TYPE_VERSION covers it; upstream Python still needs this check. + op(_GUARD_KEYS_VERSION, (keys_version/2, owner -- owner)) { +#ifndef META_PYTHON + PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version); +#else + (void)keys_version; +#endif + } + + macro(LOAD_ATTR_METHOD_WITH_VALUES) = + unused/1 + + _GUARD_TYPE_VERSION + + _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT + + _GUARD_KEYS_VERSION + + _LOAD_ATTR_METHOD_WITH_VALUES; + + macro(LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES) = + unused/1 + + _GUARD_TYPE_VERSION + + _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT + + _GUARD_KEYS_VERSION + + _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES; + override inst(EXTENDED_OPCODE, (args[oparg>>2] -- top[oparg&0x03])) { // Decode any extended oparg int extop = (int)next_instr->op.code; diff --git a/cinderx/Interpreter/3.15/Includes/generated_cases.c.h b/cinderx/Interpreter/3.15/Includes/generated_cases.c.h index 5be704c5c..84adbbaf1 100644 --- a/cinderx/Interpreter/3.15/Includes/generated_cases.c.h +++ b/cinderx/Interpreter/3.15/Includes/generated_cases.c.h @@ -10240,7 +10240,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - /* Skip 2 cache entries */ + // _GUARD_KEYS_VERSION + { + uint32_t keys_version = read_u32(&this_instr[4].cache); + #ifndef META_PYTHON + PyTypeObject* owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject* owner_heap_type = (PyHeapTypeObject*)owner_cls; + PyDictKeysObject* keys = owner_heap_type->ht_cached_keys; + if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + #else + (void)keys_version; + #endif + } // _LOAD_ATTR_METHOD_WITH_VALUES { PyObject *descr = read_obj(&this_instr[6].cache); @@ -10423,7 +10438,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - /* Skip 2 cache entries */ + // _GUARD_KEYS_VERSION + { + uint32_t keys_version = read_u32(&this_instr[4].cache); + #ifndef META_PYTHON + PyTypeObject* owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject* owner_heap_type = (PyHeapTypeObject*)owner_cls; + PyDictKeysObject* keys = owner_heap_type->ht_cached_keys; + if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + #else + (void)keys_version; + #endif + } // _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES { PyObject *descr = read_obj(&this_instr[6].cache); diff --git a/cinderx/Interpreter/3.15/cinder-bytecodes.c b/cinderx/Interpreter/3.15/cinder-bytecodes.c index 241bc0a33..7f63a9381 100644 --- a/cinderx/Interpreter/3.15/cinder-bytecodes.c +++ b/cinderx/Interpreter/3.15/cinder-bytecodes.c @@ -191,6 +191,27 @@ static PyObject* dummy_func( ERROR_IF(err < 0); } + // Meta Python bumps the type version when shared keys change, so + // _GUARD_TYPE_VERSION covers it; upstream Python still needs this check. + op(_GUARD_KEYS_VERSION, (keys_version / 2, owner-- owner)) { +#ifndef META_PYTHON + PyTypeObject* owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); + PyHeapTypeObject* owner_heap_type = (PyHeapTypeObject*)owner_cls; + PyDictKeysObject* keys = owner_heap_type->ht_cached_keys; + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version); +#else + (void)keys_version; +#endif + } + + macro(LOAD_ATTR_METHOD_WITH_VALUES) = unused / 1 + _GUARD_TYPE_VERSION + + _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT + _GUARD_KEYS_VERSION + + _LOAD_ATTR_METHOD_WITH_VALUES; + + macro(LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES) = unused / 1 + + _GUARD_TYPE_VERSION + _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT + + _GUARD_KEYS_VERSION + _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES; + override inst(EXTENDED_OPCODE, (args[oparg >> 2]-- top[oparg & 0x03])) { // Decode any extended oparg int extop = (int)next_instr->op.code; diff --git a/cinderx/PythonLib/test_cinderx/test_jit_specialization.py b/cinderx/PythonLib/test_cinderx/test_jit_specialization.py index 56146d6a6..f06d21e4b 100644 --- a/cinderx/PythonLib/test_cinderx/test_jit_specialization.py +++ b/cinderx/PythonLib/test_cinderx/test_jit_specialization.py @@ -2,11 +2,12 @@ # pyre-strict +import contextlib import dis import sys import unittest from types import ModuleType -from typing import Callable, TypeVar +from typing import Callable, Iterator, TypeVar import cinderx import cinderx.jit @@ -51,6 +52,31 @@ def specialize( cinderx.jit.force_compile(func) +# The specialized opcodes under test only run in CinderX's eval loop, which +# needs the frame evaluator installed. +@contextlib.contextmanager +def frame_evaluator() -> Iterator[None]: + if cinderx.is_frame_evaluator_installed(): + yield + return + + cinderx.install_frame_evaluator() + try: + yield + finally: + cinderx.remove_frame_evaluator() + + +# Like specialize(), but stays interpreted so the interpreter's guards run. +def specialize_interpreted( + func: Callable[..., TCallableRet], callable: Callable[[], TCallableRet] +) -> None: + cinderx.jit.jit_suppress(func) + + for _ in range(5): + callable() + + @passIf(not cinderx.jit.is_enabled(), "Tests functionality on the JIT") class SpecializationTests(unittest.TestCase): def setUp(self) -> None: @@ -298,6 +324,45 @@ def f() -> str: self.assertIn("LOAD_ATTR_MODULE", opnames(f)) self.assertEqual(f(), sys.argv[0]) + @passUnless(sys.version_info >= (3, 14), "3.12 only builds against Meta Python") + def test_load_attr_nondescriptor_with_values(self) -> None: + class C: + attr: object = "class-attr" + + def f(o: C) -> object: + return o.attr + + with frame_evaluator(): + specialize_interpreted(f, lambda: f(C())) + + self.assertNotIn("LOAD_ATTR", opnames(f)) + self.assertIn("LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", opnames(f)) + + # Setting the attribute on an instance bumps the shared keys, so the + # load must deopt instead of returning the cached class attribute. + o = C() + o.attr = "instance-attr" + self.assertEqual(f(o), "instance-attr") + + @passUnless(sys.version_info >= (3, 14), "3.12 only builds against Meta Python") + def test_load_attr_method_with_values(self) -> None: + class C: + def m(self) -> str: + return "class-method" + + def f(o: C) -> str: + return o.m() + + with frame_evaluator(): + specialize_interpreted(f, lambda: f(C())) + + self.assertNotIn("LOAD_ATTR", opnames(f)) + self.assertIn("LOAD_ATTR_METHOD_WITH_VALUES", opnames(f)) + + o = C() + setattr(o, "m", lambda: "instance-attr") + self.assertEqual(f(o), "instance-attr") + def test_store_subscr_dict(self) -> None: def f(a: dict[str, str], b: str, c: str) -> None: a[b] = c