bpo-40137: Move state lookups out of the critical path by rhettinger · Pull Request #25492 · python/cpython

bpo-40137: Move state lookups out of the critical path by rhettinger · Pull Request #25492 · python/cpython

🤖 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cc @corona10 @shihai1991

		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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

🤖 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.

		state = get_functools_state_by_type(Py_TYPE(obj));
		if (state == NULL) {
		Py_DECREF(cachedict);
		Py_DECREF(obj);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

🤖 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, it looks safe with strong references.