gh-152063: Cache the offset of the inline values 'valid' byte in LOAD_ATTR by Aniketsy · Pull Request #153122 · python/cpython

Aniketsy · 2026-07-05T12:32:55Z

Fixes #152063

Issue: The LOAD_ATTR_INSTANCE_VALUE instruction can be sped up by storing the offset of the valid field in the cache. #152063

markshannon

The changes to _LOAD_ATTR_INSTANCE_VALUE and the specializer look good.
There should be no reason to increase the cache size for LOAD_ATTR, there is already spare space in LOAD_ATTR_INSTANCE_VALUE

markshannon · 2026-07-06T09:50:57Z

 typedef struct {
    _Py_BackoffCounter counter;
    uint16_t type_version[2];
+    uint16_t value_offset;


This shouldn't need changing

markshannon · 2026-07-06T09:55:05Z

        macro(LOAD_ATTR) =
            _SPECIALIZE_LOAD_ATTR +
-            unused/8 +
+            unused/9 +


The cache is already large enough.
If you reduce the size of the unused cache in LOAD_ATTR_INSTANCE_VALUE by one, the other specializations don't need to change.

markshannon · 2026-07-06T10:01:19Z

-    cache->index = (uint16_t)offset;
-    write_u32(cache->version, tp_version);
+    if (base_op == LOAD_ATTR) {
+        Py_ssize_t valid_offset = (char *)&values->valid - (char *)owner;


Suggested change

Py_ssize_t valid_offset = (char *)&values->valid - (char *)owner;

Py_ssize_t validity_offset = (char *)&values->valid - (char *)owner;

valid_offset sounds like a bool for whether the offset is valid.

bedevere-app · 2026-07-06T10:05:32Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Aniketsy · 2026-07-07T07:06:50Z

@markshannon thanks for the review, could you please help me with this patch, after applying these changes

aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ git diff Include/internal/pycore_code.h
diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h
index b4170340b2..fd8ca313e3 100644
--- a/Include/internal/pycore_code.h
+++ b/Include/internal/pycore_code.h
@@ -108,7 +108,6 @@ typedef struct {
 typedef struct {
     _Py_BackoffCounter counter;
     uint16_t type_version[2];
-    uint16_t value_offset;
     union {
         uint16_t keys_version[2];
         uint16_t dict_offset;
@@ -125,7 +124,7 @@ typedef struct {
 typedef struct {
     _Py_BackoffCounter counter;
     uint16_t version[2];
-    uint16_t value_offset;
+    uint16_t validity_offset;
     uint16_t index;
 } _PyLoadAttrInstanceValueCache;

aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ git diff Python/bytecodes.c
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index e9d3d5f161..fe86c92d51 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -2866,7 +2866,7 @@ dummy_func(

         macro(LOAD_ATTR) =
             _SPECIALIZE_LOAD_ATTR +
-            unused/9 +
+            unused/8 +
             _LOAD_ATTR;

         op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) {
@@ -2890,12 +2890,13 @@ dummy_func(
             EXIT_IF(tp != (PyTypeObject *)type);
         }

-        op(_CHECK_MANAGED_OBJECT_HAS_VALUES, (value_offset/1, owner -- owner)) {
+        op(_CHECK_MANAGED_OBJECT_HAS_VALUES, (validity_offset/1, owner -- owner)) {
             PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner);
             assert(Py_TYPE(owner_o)->tp_dictoffset < 0);
             assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES);
-            uint8_t *valid_ptr = (uint8_t *)owner_o + value_offset;
-            EXIT_IF(!FT_ATOMIC_LOAD_UINT8(*valid_ptr));
+            assert((char *)&_PyObject_InlineValues(owner_o)->valid ==
+                   (char *)owner_o + validity_offset);
+            EXIT_IF(!FT_ATOMIC_LOAD_UINT8(*((uint8_t *)owner_o + validity_offset)));
         }

         op(_LOAD_ATTR_INSTANCE_VALUE, (offset/1, owner -- attr, o)) {
@@ -2955,7 +2956,7 @@ dummy_func(
             unused/1 +
             _LOAD_ATTR_MODULE +
             POP_TOP +
-            unused/6 +
+            unused/5 +
             _PUSH_NULL_CONDITIONAL;

         op(_LOAD_ATTR_WITH_HINT, (hint/1, owner -- attr, o)) {
@@ -3004,7 +3005,7 @@ dummy_func(
             _GUARD_TYPE_VERSION +
             _LOAD_ATTR_WITH_HINT +
             POP_TOP +
-            unused/6 +
+            unused/5 +
             _PUSH_NULL_CONDITIONAL;

         op(_LOAD_ATTR_SLOT, (index/1, owner -- attr, o)) {
@@ -3030,7 +3031,7 @@ dummy_func(
             _GUARD_TYPE_VERSION +
             _LOAD_ATTR_SLOT +  // NOTE: This action may also deopt
             POP_TOP +
-            unused/6 +
+            unused/5 +
             _PUSH_NULL_CONDITIONAL;

         op(_CHECK_ATTR_CLASS, (type_version/2, owner -- owner)) {
@@ -3052,7 +3053,7 @@ dummy_func(
             unused/1 +
             _RECORD_TOS +
             _CHECK_ATTR_CLASS +
-            unused/3 +
+            unused/2 +
             _LOAD_ATTR_CLASS +
             _PUSH_NULL_CONDITIONAL;

@@ -3060,7 +3061,6 @@ dummy_func(
             unused/1 +
             _RECORD_TOS +
             _GUARD_TYPE_VERSION +
-            unused/1 +
             _CHECK_ATTR_CLASS +
             _LOAD_ATTR_CLASS +
             _PUSH_NULL_CONDITIONAL;
@@ -3084,7 +3084,6 @@ dummy_func(
             _RECORD_TOS_TYPE +
             _GUARD_TYPE_VERSION +
             _CHECK_PEP_523 +
-            unused/1 +
             _LOAD_ATTR_PROPERTY_FRAME +
             _SAVE_RETURN_OFFSET +
             _PUSH_FRAME;
@@ -3113,7 +3112,6 @@ dummy_func(
             _RECORD_TOS_TYPE +
             _GUARD_TYPE_VERSION +
             _CHECK_PEP_523 +
-            unused/1 +
             _LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN_FRAME +
             _SAVE_RETURN_OFFSET +
             _PUSH_FRAME;
@@ -4280,7 +4278,7 @@ dummy_func(
             unused/1 +
             _RECORD_TOS_TYPE +
             _GUARD_TYPE_VERSION +
-            unused/3 +
+            unused/2 +
             _LOAD_ATTR_METHOD_NO_DICT;

         op(_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES, (descr/4, owner -- attr)) {
@@ -4312,7 +4310,7 @@ dummy_func(
             unused/1 +
             _RECORD_TOS_TYPE +
             _GUARD_TYPE_VERSION +
-            unused/3 +
+            unused/2 +
             _LOAD_ATTR_NONDESCRIPTOR_NO_DICT;

         op(_CHECK_ATTR_METHOD_LAZY_DICT, (dictoffset/1, owner -- owner)) {
@@ -4336,7 +4334,6 @@ dummy_func(
             unused/1 +
             _RECORD_TOS_TYPE +
             _GUARD_TYPE_VERSION +
-            unused/1 +
             _CHECK_ATTR_METHOD_LAZY_DICT +
             unused/1 +
             _LOAD_ATTR_METHOD_LAZY_DICT;
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ git diff Python/specialize.c
diff --git a/Python/specialize.c b/Python/specialize.c
index 7593de78af..9873d032a6 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -655,14 +655,14 @@ specialize_dict_access_inline(
         return 0;
     }
     if (base_op == LOAD_ATTR) {
-        Py_ssize_t valid_offset = (char *)&values->valid - (char *)owner;
-        if (valid_offset != (uint16_t)valid_offset) {
+        Py_ssize_t validity_offset = (char *)&values->valid - (char *)owner;
+        if (validity_offset != (uint16_t)validity_offset) {
             SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OUT_OF_RANGE);
             return 0;
         }
         _PyLoadAttrInstanceValueCache *iv_cache =
             (_PyLoadAttrInstanceValueCache *)cache;
-        iv_cache->value_offset = (uint16_t)valid_offset;
+        iv_cache->validity_offset = (uint16_t)validity_offset;
         iv_cache->index = (uint16_t)offset;
         write_u32(iv_cache->version, tp_version);
     }
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$

getting this error while building..

ation -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -DPy_BUILD_CORE -o Python/context.o Python/context.c
In file included from ./Include/internal/pycore_backoff.h:12,
                 from Python/ceval.h:6,
                 from Python/ceval.c:3:
Python/generated_cases.c.h: In function ‘_PyEval_EvalFrameDefault’:
Python/generated_cases.c.h:8977:13: error: static assertion failed: "incorrect cache size"
 8977 |             static_assert(INLINE_CACHE_ENTRIES_LOAD_ATTR == 10, "incorrect cache size");
      |             ^~~~~~~~~~~~~
Python/generated_cases.c.h:9170:13: error: static assertion failed: "incorrect cache size"
 9170 |             static_assert(INLINE_CACHE_ENTRIES_LOAD_ATTR == 10, "incorrect cache size");
      |             ^~~~~~~~~~~~~
Python/generated_cases.c.h:9370:13: error: static assertion failed: "incorrect cache size"
 9370 |             static_assert(INLINE_CACHE_ENTRIES_LOAD_ATTR == 10, "incorrect cache size");
      |             ^~~~~~~~~~~~~
make: *** [Makefile:3363: Python/ceval.o] Error 1
make: *** Waiting for unfinished jobs....

i was kind of stuck from yesterday, so thought to take help from you 😊 . Thank you !

markshannon · 2026-07-07T08:26:35Z

It means that you've changed either the size of some of the LOAD_ATTR specializations or the size of the _PyLoadMethodCache struct.
Neither should change.

markshannon · 2026-07-07T08:28:27Z

If you increase the size of the _CHECK_MANAGED_OBJECT_HAS_VALUES uop, you'll need to reduce the number of unused cache entries by the same amount to keep the overall size the same

Aniketsy · 2026-07-13T12:11:56Z

@markshannon gentle ping, please have a look in this when you get chance. Thanks :)

markshannon

I think you may be missing the offset in the cache for LOAD_ATTR_METHOD_WITH_VALUES when specializing

markshannon · 2026-07-13T14:27:07Z

            EXIT_IF(!FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(owner_o)->valid));
        }

+        op(_CHECK_MANAGED_OBJECT_HAS_VALUES_OFFSET, (validity_offset/1, owner -- owner)) {


Can you rename this to _CHECK_MANAGED_OBJECT_HAS_VALUES it isn't checking an offset.

After renaming it creates the duplicate op, so i've removed

markshannon · 2026-07-13T14:37:34Z

            SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS);
            return 0;
        }
+        write_u32(cache->keys_version, shared_keys_version);


This seems odd.
Why are you writing the keys version? What about the offset?

You may need to change _PyLoadMethodCache to reflect how it is now used

…alizations

Aniketsy · 2026-07-14T10:19:13Z

@markshannon thanks for the review and improvement, i've updated with changes as per your suggestions, please let me know if this needs further improvement.

Cache the offset of the inline values 'valid' byte in LOAD_ATTR

82516ed

Aniketsy requested a review from markshannon as a code owner July 5, 2026 12:32

bedevere-app Bot mentioned this pull request Jul 5, 2026

The LOAD_ATTR_INSTANCE_VALUE instruction can be sped up by storing the offset of the valid field in the cache. #152063

Open

bedevere-app Bot added the awaiting review label Jul 5, 2026

Aniketsy marked this pull request as draft July 5, 2026 12:58

bedevere-app Bot removed the awaiting review label Jul 5, 2026

markshannon requested changes Jul 6, 2026

View reviewed changes

bedevere-app Bot added the awaiting changes label Jul 6, 2026

markshannon added the skip news label Jul 6, 2026

Aniketsy added 2 commits July 7, 2026 11:34

resolve conflicts

0badb77

Address review: keep _PyLoadMethodCache and cache sizes unchanged.

728d242

Aniketsy marked this pull request as ready for review July 7, 2026 18:42

bedevere-app Bot added awaiting review and removed awaiting changes labels Jul 7, 2026

markshannon reviewed Jul 13, 2026

View reviewed changes

Aniketsy added 2 commits July 13, 2026 16:51

resolve conflicts

c28f94e

Store validity_offset in LOAD_ATTR inline cache for WITH_VALUES speci…

ee09166

…alizations

	Py_ssize_t valid_offset = (char )&values->valid - (char )owner;
	Py_ssize_t validity_offset = (char )&values->valid - (char )owner;

Uh oh!

Uh oh!

Conversation

Aniketsy commented Jul 5, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

markshannon Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

markshannon Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Jul 6, 2026

Uh oh!

Aniketsy commented Jul 7, 2026

Uh oh!

markshannon commented Jul 7, 2026

Uh oh!

markshannon commented Jul 7, 2026

Uh oh!

Aniketsy commented Jul 13, 2026

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Aniketsy Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

markshannon Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Aniketsy commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Aniketsy commented Jul 5, 2026 •

edited by bedevere-app Bot

Loading