Skip to content

pystats: optimized_trace_length histogram includes exits #116808

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

Closed
mdboom opened this issue Mar 14, 2024 · 6 comments
Closed

pystats: optimized_trace_length histogram includes exits #116808

mdboom opened this issue Mar 14, 2024 · 6 comments
Labels
performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Comments

@mdboom
Copy link
Contributor

mdboom commented Mar 14, 2024

Bug report

Bug description:

With the addition of cold exits, the "optimized trace length" histogram now includes the exit count + the optimized trace length.

While this is a useful metric for the overall executor size, it's no longer useful to see the amount of instructions that the optimizer was able to remove from the hot path.

I would suppose we break this out into two stats: the optimized trace length and the exit count.

Does this make sense, @markshannon?

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

@mdboom mdboom added type-bug An unexpected behavior, bug, or error performance Performance or resource usage labels Mar 14, 2024
@mdboom
Copy link
Contributor Author

mdboom commented Mar 14, 2024

Additionally, this hides data since the histogram maxes out at 8,192 (MAX_TRACE_LENGTH), and trace_length + exit_counts is frequently over that.

@markshannon
Copy link
Member

The trace length historgrams should show lengths in number of uops, ignoring exits.
It should be trivial to fix once #116813 is merged, as that tracks the length of trace through optimization.

@mdboom
Copy link
Contributor Author

mdboom commented Mar 14, 2024

It should be trivial to fix once #116813 is merged, as that tracks the length of trace through optimization.

Sure, I think it's also trivial to fix now (and restore to old behavior), by tracking these two things separately -- I mainly wanted to get your take on whether tracking them as a single number was intentional:

allocate_executor() {
   ...
   int size = exit_count*sizeof(_PyExitData) + length*sizeof(_PyUOpInstruction);
   ...
}

But if it's easier merge-wise to do this following or as part of #116813, that also makes sense.

@markshannon
Copy link
Member

I don't think it was intended. Tracking the size of the executor in bytes doesn't make a lot of sense.

@hugovk
Copy link
Member

hugovk commented Jun 15, 2024

Triage: the linked PR is merged, can this be closed?

@mdboom
Copy link
Contributor Author

mdboom commented Jun 17, 2024

Yes, it can be closed. Sorry this fell through the cracks.

@mdboom mdboom closed this as completed Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants