gh-148510: restore `func_version` check in `_LOAD_ATTR_PROPERTY_FRAME` by NekoAsakura · Pull Request #148528 · python/cpython

NekoAsakura · 2026-04-13T21:16:12Z

~~Do not merge.~~
Prerequisite: #148524 (Merged)
Mean +- std: [main] 12.8 ms +- 0.2 ms -> [branch] 12.3 ms +- 0.1 ms

Issue: _LOAD_ATTR_PROPERTY_FRAME has too weak checks that are also slow #148510

…_FRAME`

Fidget-Spinner

Just one thing that should be addressed. Thanks!

…_FRAME`

NekoAsakura · 2026-04-14T10:30:47Z

Seems like probable_type gets dereferenced without a NULL check here.

cpython/Python/optimizer_bytecodes.c

Lines 143 to 145 in 21da9d7

if (probable_type->tp_version_tag==type_version&&sym_set_type_version(owner, type_version)) { 
// Promote the probable type version to a known one.
if ((probable_type->tp_flags&Py_TPFLAGS_IMMUTABLETYPE) ==0) { 

Fidget-Spinner · 2026-04-14T11:09:21Z

Seems like probable_type gets dereferenced without a NULL check here.
cpython/Python/optimizer_bytecodes.c
Lines 143 to 145 in 21da9d7
if (probable_type->tp_version_tag==type_version&&sym_set_type_version(owner, type_version)) {
// Promote the probable type version to a known one.
if ((probable_type->tp_flags&Py_TPFLAGS_IMMUTABLETYPE) ==0) {

Oops I think that was me. Can you please add a condition to that in another PR?

NekoAsakura · 2026-04-14T11:13:23Z

Sure.

Fidget-Spinner · 2026-04-14T11:14:27Z

@NekoAsakura sorry for the flip flop, but I think we can do this in this PR, I realised they're the same uop, just one is the other variant.

NekoAsakura · 2026-04-14T11:16:05Z

NP! Wait me a second.

Fidget-Spinner

Pretty close.

Fidget-Spinner · 2026-04-14T11:16:27Z

+ sym_set_type_version(owner, type_version)) {
+ PyType_Watch(TYPE_WATCHER_ID, (PyObject *)probable_type);
+ _Py_BloomFilter_Add(dependencies, probable_type);


Instead of setting the type verison, promote the type. Use sym_set_type(owner, probable_type) inside the if branch. Please update _GUARD_TYPE_VERSION_LOCKED to do that as well (and fix the NULL check).

Wait I'm wrong here, we can't do that. I forgot what sym_set_type_version does. Sorry for misleading!

I think we can do both in sequence like:
sym_set_type(owner, probable_type); sym_set_type_version(owner, type_version);
This promotes type while keeping the version.

If we do sym_set_type(owner, probable_type);, we don't need to set the type version after, as sym_get_type_version just grabs tp_version_tag from the type. It's essentially a no-op.
Also type is strictly more information than type version in our framework https://github.com/python/cpython/blob/main/Python/optimizer_symbols.c#L37

I'm afraid sym_get_type_version returns sym->cls.version for KNOWN_CLASS_TAG, not cls.type->tp_version_tag:
cpython/Python/optimizer_symbols.c
Lines 750 to 751 in 21da9d7
caseJIT_SYM_KNOWN_CLASS_TAG:
returnsym->cls.version;
And sym_set_type sets cls.version = 0 when promoting from RECORDED_TYPE_TAG:
cpython/Python/optimizer_symbols.c
Lines 351 to 355 in 21da9d7
caseJIT_SYM_RECORDED_TYPE_TAG:
if (sym->recorded_type.type==typ) {
sym->tag=JIT_SYM_KNOWN_CLASS_TAG;
sym->cls.version=0;
sym->cls.type=typ;
Without sym_set_type_version afterwards, the version stays 0 and guard elimination breaks.

Ah ok. Thanks for digging into this!

Is there a reason for it to return cls.version when class is known? It seems better to return the version from tp_version_tag.

…_FRAME`

Fidget-Spinner

Thanks, LGTM. Just one comment.

Fidget-Spinner · 2026-04-14T13:21:09Z

- PyCodeObject *co = (PyCodeObject *)((PyFunctionObject *)fget)->func_code;
+ op(_LOAD_ATTR_PROPERTY_FRAME, (func_version/2, fget/4, owner -- new_frame)) {
+ if (sym_get_type_version(owner) == 0) {
+ ctx->done = true;


Why not ctx->contradiction to true as well? That would abandon the whole trace, which is what we dant. Done allows the trace to continue executing.

Oops I accidentally deleted that. Thanks for catching it!

Fidget-Spinner · 2026-04-14T13:22:39Z

+ # Check the optimizer traced through the property call.
+ self.assertIn("_CALL_BUILTIN_CLASS", uops)


No this doesn't check the optimizer actually ran inside the call. The easiest way is to check the global was promoted, ie the absence of _LOAD_GLOBAL_BUILTIN.

Yes, was missing f->func = func on the abstract frame so globals couldn't be resolved. Sorted now, test asserts the absence of _LOAD_GLOBAL_BUILTINS.

…_FRAME`

Fidget-Spinner

Great!

…_FRAME` (pythonGH-148528)

pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
9235de6
…_FRAME`

NekoAsakura requested review from Fidget-Spinner, markshannon, savannahostrowski and tomasr8 as code owners April 13, 2026 21:16

bedevere-appBot added the awaiting review label Apr 13, 2026

bedevere-appBot mentioned this pull request Apr 13, 2026
_LOAD_ATTR_PROPERTY_FRAME has too weak checks that are also slow #148510
Closed

cocolato reviewed Apr 14, 2026
View reviewed changes

Comment threadTools/cases_generator/optimizer_generator.py

cocolato added the skip news label Apr 14, 2026

Fidget-Spinner reviewed Apr 14, 2026
View reviewed changes

Comment threadPython/bytecodes.c
Comment threadPython/optimizer_bytecodes.c Outdated

NekoAsakura added 3 commits April 14, 2026 06:15

pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
815b6ba
…_FRAME`

pythongh-148510: revert 148524
8968aa5

Merge branch 'main' intopythongh-148510/load-attr-property-frame
4bc0a97

Fidget-Spinner reviewed Apr 14, 2026
View reviewed changes

Fidget-Spinner mentioned this pull request Apr 14, 2026
gh-148380: remove all uses of _PyType_LookupByVersion in optimizer_bytecodes.c#148394
Merged

kumaraditya303 reviewed Apr 14, 2026
View reviewed changes

Comment threadLib/test/test_capi/test_opt.py Outdated

NekoAsakura added 2 commits April 14, 2026 08:57

pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
cc16f6f
…_FRAME`

pythongh-148510: call builtin in property test
9c7940c

Fidget-Spinner approved these changes Apr 14, 2026
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting review labels Apr 14, 2026

Fidget-Spinner reviewed Apr 14, 2026
View reviewed changes

NekoAsakura added 2 commits April 14, 2026 09:24

pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
6a430c8
…_FRAME`

pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
7a0aced
…_FRAME`

Fidget-Spinner approved these changes Apr 14, 2026
View reviewed changes

kumaraditya303 reviewed Apr 14, 2026
View reviewed changes

Comment threadPython/optimizer_bytecodes.c Outdated

pythongh-148510: remove duplication
c8ea319

kumaraditya303 approved these changes Apr 14, 2026
View reviewed changes

Fidget-Spinner approved these changes Apr 14, 2026
View reviewed changes

Fidget-Spinner merged commit 52a7f1b into python:mainApr 14, 2026
74 checks passed

bedevere-appBot removed the awaiting merge label Apr 14, 2026

NekoAsakura deleted the gh-148510/load-attr-property-frame branch April 14, 2026 14:49

ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
pythongh-148510: restorefunc_versioncheck in `_LOAD_ATTR_PROPERTY…
b8a9ca7
…_FRAME` (pythonGH-148528)

	caseJIT_SYM_RECORDED_TYPE_TAG:
	if (sym->recorded_type.type==typ) {
	sym->tag=JIT_SYM_KNOWN_CLASS_TAG;
	sym->cls.version=0;
	sym->cls.type=typ;

		# Check the optimizer traced through the property call.
		self.assertIn("_CALL_BUILTIN_CLASS", uops)

Uh oh!

Uh oh!

Conversation

NekoAsakura commented Apr 13, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NekoAsakura commented Apr 14, 2026

Uh oh!

Fidget-Spinner commented Apr 14, 2026

Uh oh!

NekoAsakura commented Apr 14, 2026

Uh oh!

Fidget-Spinner commented Apr 14, 2026

Uh oh!

NekoAsakura commented Apr 14, 2026

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerApr 14, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NekoAsakura commented Apr 13, 2026•
edited
Loading

Fidget-SpinnerApr 14, 2026•
edited
Loading