gh-152063: Cache the offset of the inline values 'valid' byte in LOAD_ATTR#153122
gh-152063: Cache the offset of the inline values 'valid' byte in LOAD_ATTR#153122Aniketsy wants to merge 1 commit into
Conversation
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
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
| typedef struct { | ||
| _Py_BackoffCounter counter; | ||
| uint16_t type_version[2]; | ||
| uint16_t value_offset; |
There was a problem hiding this comment.
This shouldn't need changing
| macro(LOAD_ATTR) = | ||
| _SPECIALIZE_LOAD_ATTR + | ||
| unused/8 + | ||
| unused/9 + |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
| 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.
|
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 |
2 participants
Fixes #152063
LOAD_ATTR_INSTANCE_VALUEinstruction can be sped up by storing the offset of thevalidfield in the cache. #152063