Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
GH-114695: Addsys._clear_internal_caches#115152
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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
validflag 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?
brandtbucherFeb 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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_executorssys._clear_internal_cachesoffensive-vk commentedFeb 10, 2024
I'm Sorry, mistakenly clicked on review button. |
Uh oh!
There was an error while loading.Please reload this page.
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
linkedmember is redundant with thevalidfield, so it's removed.📚 Documentation preview 📚:https://cpython-previews--115152.org.readthedocs.build/