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
Propagate qualname from the compiler unit to code objects for finer grained profiling data #88696
Comments
When dumping profiling data out of code objects using out-of-process tools like Austin (https://github.com/p403n1x87/austin) one has access only to file name, function name, and line number. Consider the flame graph generated by running the following script, and aggregating on function names ---- class Foo:
def on_cpu(self, n):
a = []
for i in range(n):
a.append(i)
class Bar:
def on_cpu(self, n):
a = []
for i in range(n):
a.append(i)
if __name__ == "__main__":
f = Foo()
b = Bar()
f.on_cpu(1_000_000)
b.on_cpu(5_000_000) Without the extra information coming from the actual Python source, one would not be able to tell, by looking at the flame graph alone, that on_cpu has contributions from two different methods. By propagating the qualname information from the compiler to code objects, such names would be disambiguated and the resulting flame graph would be clearer. I would like to propose adding the co_qualname field to the PyCodeObject structure, which is to be set to NULL except for when the code object is created by the compiler in compile.c. |
I think this is a worthwhile improvement.
|
This also has the side effect of making marshalled code objects bigger, and we keep adding to this in 3.11 with the new exception table and soon with PEP-657. Given that a lot of people are concerned about this and that this change only affects a very small set of tools, we need to analyze very carefully the balance. |
Thanks for the feedback. I certainly appreciate all the concerns around this proposed change. @Mark.Shannon
However, I also appreciate @pablogsal's point of view of evaluating the actual benefits. The extra point in favour of this change that I can make is that it would resolve name clashes in profiling data from tools that have minimal impact on the runtime. Whether this is enough to justify the extra memory cost is certainly up for debate. From my side, I don't have the numbers to make this case more concrete. As a next step, I'll look into exposing the new field to Python to see what it actually ends up looking like. |
Commit a0252ab exposes co_qualname to Python. I've added a test case to check that the result coincides with the current implementation of __qualname__. Is there a "standard" way for me to quantify the memory impact of these kinds of changes? @Mark.Shannon I'll look into making __qualname__ use __code__.co_qualname as a next step. |
That is a pointer size per code object. The most standard way is to calculate the size of all pyc files in the stdlib after compiling them all with "-m compileall -r 1000 Lib". |
This yields the following numbers between what was main when I branched off and the tip of my branch: # main (7569c0f): 25_059_438 bytes So that seems to be about half a MB increase over 25 MB (about 2% relative increase). This is potentially just an interim result, as at first sight it looks like MAKE_FUNCTION could require one less argument (the qualname), which can now be taken from the code object. So not quite the final picture yet. |
I personally think that is acceptable, so I would be supportive of the patch but for context, many folks have indicated that they are worried about this size getting bigger during previous discussions, so we need to still think collectively :) |
With commit 7a12d31a8c22cae7a9c1a2239a1bb54adee33622 the new figures are # main (7569c0f): 25_059_438 bytes which is a 0.1% relative increase :). This is likely due to the fact that with the change MAKE_FUNCTION needs to pop one less value from the TOS, as the qualname now comes from the code object. I think the PR is now good for a first review. I think I'd need some help with the documentation sources. |
I suspect that the 0.1% increase is noise. After all, the qualname has to stored somewhere and moving it from caller to callee should make no difference. Organizing the code object so it wastes less memory is a separate issue. |
This change modified the audit event 'code.__name__', which requires a deprecation period (all events are public API, as per PEP-578). We need to revert that part of the change. I don't think we need to add a new event to report the qualname here, so just dropping the new argument seems fine. |
Correction: the event is |
Reopening this to track the removal of some dead code left behind in the compiler by this change; cleaned up by #100806. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: