gh-116750: Add clear_tool_id function to unregister events and callbacks - #124568
gaogaotiantian merged 9 commits into
Conversation
Sorry, something went wrong.
associated with tool_id
markshannon
left a comment
There was a problem hiding this comment.
There is one comment/misunderstanding that needs resolving, but the overall design looks good.
Is there any way to reduce the size of the per-tool version table? It doubles the size of the _PyCoMonitoringData struct.
Sorry, something went wrong.
| _PyEval_StartTheWorld(interp); | ||
| return -1; | ||
| } | ||
| interp->monitoring_tool_versions[tool_id] = version; |
There was a problem hiding this comment.
This looks like the version for tool_id will be the only one that matches the global version, which means that tool_id will be the only tool with an up-to-date version number.
Isn't that the opposite of what we want?
Maybe interp->monitoring_tool_versions[tool_id] = 0 and leave the global version untouched?
Sorry, something went wrong.
There was a problem hiding this comment.
We need to force all code object, executing or not, to "refresh" because the local events will be changed, so we need to update global version for the code objects to know.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I understood you here. You think it would make sense to do
interp->monitoring_tool_versions[tool_id] = 0;here so all the code objects checking tool_id will know they are out of date. However, setting it to the latest global version will have the same effect - none of the code object has this version either.
If we set this to 0, we need to set it to the latest global version some time in the future, before we use this tool to instrument code again.
So we can either set it to 0 here and set it to global_version in use_tool_id, or we can do it together here(I can add some. comments here to explain).
Sorry, something went wrong.
There was a problem hiding this comment.
Ah I was wrong in the previous comment. The update to global version is critical here because the user can set local events immediately after clear_tool_id. clear_tool_id does not surrender the tool_id. So we have to set the tool id version to the latest global version here for the following local events.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
We can ignore sys.settrace and sys.setprofile so that's two less. Would it help? |
Sorry, something went wrong.
|
So I added the test and some comments. I attempted to throw away sys.setprofile and sys.settrace from versioning, it's definitely doable, with a few issues:
None of above is unsolvable, we can do it and it's not rocket science, but is it worth it? We eliminates 2 versions out of 8 and maybe get a very small performance optimization when we loop, but that seems small to me. I reverted the change, but let me know if you think it's better to have 6 instead of 8. |
Sorry, something went wrong.
|
I think leaving the size as 8 is fine. It is more consistent with the rest of the code. |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
Looks good.
Sorry, something went wrong.
Provide a mechanism to unregister everything of a tool, including local events. The life would be so much easier for debuggers.
📚 Documentation preview 📚: https://cpython-previews--124568.org.readthedocs.build/