gh-145685: per-type method cache implementation#150160
Conversation
kumaraditya303
requested review from
a team,
AA-Turner,
FFY00,
ZeroIntensity,
emmatyping,
ericsnowcurrently,
erlend-aasland and
markshannon
as code owners
kumaraditya303
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 728021c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F150160%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
| } | ||
| else { | ||
| // double the cache size when resizing | ||
| new_size = old_size * 2; |
There was a problem hiding this comment.
I think we should have some (fairly large) upper bound on the per-type cache size. Maybe 65536?
| } | ||
|
|
||
| static struct type_cache * | ||
| cache_allocate(uint32_t size) |
There was a problem hiding this comment.
Should the cache be freed in type_dealloc?
| static inline void | ||
| cache_set(PyTypeObject *type, struct type_cache *cache) | ||
| { | ||
| FT_ATOMIC_STORE_PTR(*cache_slot(type), cache); |
There was a problem hiding this comment.
I think this can be FT_ATOMIC_STORE_PTR_RELEASE
| uint32_t version_tag; // initialized from type->tp_version_tag | ||
| uint32_t available; // number of available entries in hashtable | ||
| uint32_t used; // number of used entries in hashtable | ||
| struct type_cache_entry hashtable[1]; // hashtable entries, the total size is always power of 2 and at least _Py_TYPECACHE_MINSIZE |
There was a problem hiding this comment.
I think the preference now is to use C99 flexible array members (e.g., struct type_cache_entry hashtable[]) in internal-only headers, especially if it's not include transitively by one of the commonly used headers. You may need to adjust the allocation size logic if you change this.
| assert(cache->available > 0); | ||
| } | ||
| cache_insert(cache, name, value); | ||
| FT_ATOMIC_STORE_UINT_RELAXED(cache->version_tag, FT_ATOMIC_LOAD_UINT_RELAXED(type->tp_version_tag)); |
There was a problem hiding this comment.
Some of the cache->version_tag and type->tp_version_tag handling doesn't make sense to me:
- First, I think having a version_tag on the cache make sense. We still need the version tag in some places and getting it from the cache (instead of the type) is good
cache->version_tagshould be immutable for the lifetime of the cache.tp_version_tagonly changes after it gets reset to zero, which also clears the type's cache, so I don't see any case wherecache->version_tagcan reasonably change on a givencache.
3 participants
_PyType_Lookup/_PyType_LookupStackRefAndVersion#145685