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`

Tools/cases_generator/optimizer_generator.py

Fidget-Spinner

Just one thing that should be addressed. Thanks!

Python/bytecodes.c

Python/optimizer_bytecodes.c

…_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.

Python/optimizer_bytecodes.c

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

Python/optimizer_bytecodes.c

+                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

case JIT_SYM_KNOWN_CLASS_TAG:

return sym->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

case JIT_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.

Python/optimizer_bytecodes.c

Lib/test/test_capi/test_opt.py

…_FRAME`

Fidget-Spinner

Thanks, LGTM. Just one comment.

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

Python/optimizer_bytecodes.c

-        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

Lib/test/test_capi/test_opt.py

+        # 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!

Python/optimizer_bytecodes.c

pythongh-148510: restore func_version check 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-app bot added the awaiting review label Apr 13, 2026

bedevere-app bot 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

Tools/cases_generator/optimizer_generator.py Show resolved Hide resolved

cocolato added the skip news label Apr 14, 2026

Fidget-Spinner reviewed Apr 14, 2026

View reviewed changes

Python/bytecodes.c Show resolved Hide resolved

Python/optimizer_bytecodes.c Outdated Show resolved Hide resolved

NekoAsakura added 3 commits April 14, 2026 06:15

pythongh-148510: restore func_version check in `_LOAD_ATTR_PROPERTY…

815b6ba

…_FRAME`

pythongh-148510: revert 148524

8968aa5

Merge branch 'main' into pythongh-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

Open

kumaraditya303 reviewed Apr 14, 2026

View reviewed changes

Lib/test/test_capi/test_opt.py Outdated Show resolved Hide resolved

NekoAsakura added 2 commits April 14, 2026 08:57

pythongh-148510: restore func_version check 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-app bot 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: restore func_version check in `_LOAD_ATTR_PROPERTY…

6a430c8

…_FRAME`

pythongh-148510: restore func_version check 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

Python/optimizer_bytecodes.c Outdated Show resolved Hide resolved

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:main Apr 14, 2026
74 checks passed

bedevere-app bot removed the awaiting merge label Apr 14, 2026

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

	case JIT_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!

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-Spinner Apr 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-Spinner Apr 14, 2026 •

edited

Loading