-
-
Notifications
You must be signed in to change notification settings - Fork 31k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-114695: Add sys._clear_internal_caches
#115152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but I have a couple of concerns.
-
I don't like exposing low-level implementation details like executors though, so could we rename
sys._clear_executors()
?
Maybe merge it withsys._clear_type_cache()
into something likesys._clear_internal_caches()
?
That way we can add other caches, jitted code, etc. and clear them all without changing the API. -
We might want to invalidate a lot of executors in a stop-the-world event. Merely setting the
valid
flag to zero (as we do now) is fine, but freeing lots of object may not be. Can we go back to invalidating executors not doing much work, and leaving clean up to later?
Include/cpython/optimizer.h
Outdated
_PyBloomFilter bloom; | ||
_PyExecutorLinkListNode links; | ||
PyCodeObject *code; // Weak (NULL if no corresponding ENTER_EXECUTOR). | ||
int index; // Index of ENTER_EXECUTOR in the above code object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this above the bloom filter, to avoid holes?
Python/optimizer.c
Outdated
exec->vm_data.valid = false; | ||
exec->vm_data.linked = false; | ||
exec = next; | ||
_PyExecutorObject *executor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for moving the declaration out of the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a while loop now.
Would you prefer to leave it as a for loop? It's a tiny bit more repetitive, but maybe easier to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The for loop is nice as it limits the scope of the iteration variable. It doesn't matter much.
sys._clear_executors
sys._clear_internal_caches
I'm Sorry, mistakenly clicked on review button. |
This new function immediately releases all tier-two related references and memory, which is useful for refleak-hunting.
Currently, invalid executors need to be hit again in order to be cleared (which may never happen), and there is no way to free the code object's executor array. This PR adds a (weak) pointer from executors back to the code object they're installed in, which allows them to immediately remove themselves upon invalidation. Code objects can clear this pointer if the executor outlives them.
This PR also changes the linked list to contain every valid executor, and no invalid ones. That means that the old
linked
member is redundant with thevalid
field, so it's removed.📚 Documentation preview 📚: https://cpython-previews--115152.org.readthedocs.build/