Skip to content

Restore _GUARD_KEYS_VERSION in LOAD_ATTR specializations - #130

Closed
ddorian wants to merge 1 commit into
facebookincubator:mainfrom
ddorian:fix-load-attr-keys-version
Closed

Restore _GUARD_KEYS_VERSION in LOAD_ATTR specializations#130
ddorian wants to merge 1 commit into
facebookincubator:mainfrom
ddorian:fix-load-attr-keys-version

Conversation

@ddorian

@ddorian ddorian commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

partial fix for #115

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 10, 2026
Comment thread cinderx/PythonLib/test_cinderx/test_load_attr_keys_version.py Outdated
@DinoV

DinoV commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Oh, this is interesting, and is going to need some changes to how we build things internally too. We end up generating the generated_cases.c.h from Meta Python 3.14 https://github.com/facebookincubator/cinder/tree/meta/3.14 where we've backported the upstream support for not needing the guards keys version. In addition to the backout here we'll need to update our internal syncing scripts too.

Or maybe what's easier would be to update cinder-bytecodes.c to have the _GUARD_KEYS_VERSION inlined into the opcode and if it ifdef'd under META_PYTHON.

@DinoV

DinoV commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Oh, this is interesting, and is going to need some changes to how we build things internally too. We end up generating the generated_cases.c.h from Meta Python 3.14 https://github.com/facebookincubator/cinder/tree/meta/3.14 where we've backported the upstream support for not needing the guards keys version. In addition to the backout here we'll need to update our internal syncing scripts too.

Or maybe what's easier would be to update cinder-bytecodes.c to have the _GUARD_KEYS_VERSION inlined into the opcode and if it ifdef'd under META_PYTHON.

@ddorian If you'd like to land the full fix for it I think we just need to add:

        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);
#endif
        }

        // Added for compatibility w/ non meta Python where we need to do _GUARD_KEYS_VERSION still
        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;

``` into cinderx/Interpreter/3.14/cinder-bytecodes.c and the equivalent version for 3.15 and then your `_GUARD_KEYS_VERSION` in the generated_cases file would get the same ifndef as well. Or we can land the fix to the bytecodes internally and just land this w/ the tests.

@ddorian

ddorian commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@DinoV

Oh, this is interesting, and is going to need some changes to how we build things internally too. We end up generating the generated_cases.c.h from Meta Python 3.14 https://github.com/facebookincubator/cinder/tree/meta/3.14 where we've backported the upstream support for not needing the guards keys version. In addition to the backout here we'll need to update our internal syncing scripts too.

Can you do the syncing and I rebase on top of it?

@ddorian

ddorian commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

I've rebased this here with all tests green: ddorian#3

Should I push the rebased version here? Are you still interested in this PR?

@DinoV

DinoV commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I've rebased this here with all tests green: ddorian#3

Should I push the rebased version here? Are you still interested in this PR?

Yes, push the rebased version here and we'll go ahead and import it!

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 facebookincubatorgh-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.
@ddorian
ddorian force-pushed the fix-load-attr-keys-version branch from 77b81d1 to 00d7e57 Compare September 1, 2026 11:11
@ddorian

ddorian commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@DinoV pushed

@meta-codesync

meta-codesync Bot commented Sep 1, 2026

Copy link
Copy Markdown

@DinoV has imported this pull request. If you are a Meta employee, you can view this in D118315962.

@meta-codesync meta-codesync Bot closed this in 089c5c2 Sep 1, 2026
@meta-codesync meta-codesync Bot added the Merged label Sep 1, 2026
@meta-codesync

meta-codesync Bot commented Sep 1, 2026

Copy link
Copy Markdown

@DinoV merged this pull request in 089c5c2.

@DinoV

DinoV commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants