bpo-40137: Move state lookups out of the critical path#25492
bpo-40137: Move state lookups out of the critical path#25492rhettinger merged 5 commits intopython:masterfrom
Conversation
rhettinger
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @rhettinger for commit 58bc078 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Since it's a cache, I think that adding two pointers to lru_cache_object structure is a reasonable CPU vs memory trade-off. Maybe we should reuse this approach in other performance critical code paths.
Modules/_functoolsmodule.c
Outdated
| obj->misses = obj->hits = 0; | ||
| obj->maxsize = maxsize; | ||
| obj->kwd_mark = state->kwd_mark; // Borrowed | ||
| obj->lru_list_elem_type = state->lru_list_elem_type; // Borrowed |
There was a problem hiding this comment.
Would it possible to use a strong reference instead? Maybe the module instance can be deleted but the object survives longer. You need to update lru_cache_tp_traverse() and lru_cache_tp_clear() in this case.
rhettinger
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @rhettinger for commit a1e622e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Modules/_functoolsmodule.c
Outdated
| state = get_functools_state_by_type(Py_TYPE(obj)); | ||
| if (state == NULL) { | ||
| Py_DECREF(cachedict); | ||
| Py_DECREF(obj); |
There was a problem hiding this comment.
Hum. I am not sure if it's safe to call lru_cache_dealloc() here, obj is not fully initialized. For example, lru_cache_unlink_list() can crash, no? Maybe call get_functools_state_by_type(type) before creating obj?
rhettinger
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @rhettinger for commit 5a03e52 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. Thanks, it looks safe with strong references.
4 participants
https://bugs.python.org/issue40137