Description

mdboom

opened on Mar 14, 2024

Issue body actions

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

• gh-116808: Fix optimized trace length histogram #116827

Activity

1. mdboom

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   performancePerformance or resource usagePerformance or resource usage

   on Mar 14, 2024
2. mdboom commented on Mar 14, 2024

   

   mdboom

   on Mar 14, 2024

   ContributorAuthor

   More actions

   Additionally, this hides data since the histogram maxes out at 8,192 (MAX_TRACE_LENGTH), and trace_length + exit_counts is frequently over that.
3. markshannon commented on Mar 14, 2024

   

   markshannon

   on Mar 14, 2024

   Member

   More actions

   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.
4. mdboom commented on Mar 14, 2024

   

   mdboom

   on Mar 14, 2024

   ContributorAuthor

   More actions

   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.
5. markshannon commented on Mar 14, 2024

   

   markshannon

   on Mar 14, 2024

   Member

   More actions

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

   added a commit that references this issue on Mar 14, 2024

   pythongh-116808: Fix optimized trace length histogram

   90c09b5
7. markshannon

   added a commit that references this issue on Mar 19, 2024

   gh-116808: Fix optimized trace length histogram (GH-116827)

   0f27801
8. vstinner

   added a commit that references this issue on Mar 20, 2024

   pythongh-116808: Fix optimized trace length histogram (pythonGH-116827)

   58904bc
9. adorilson

   added a commit that references this issue on Mar 25, 2024

   pythongh-116808: Fix optimized trace length histogram (pythonGH-116827)

   5d08e86
10. diegorusso

    added a commit that references this issue on Apr 17, 2024

    pythongh-116808: Fix optimized trace length histogram (pythonGH-116827)

    ed35667
11. hugovk commented on Jun 15, 2024

    

    hugovk

    on Jun 15, 2024

    Member

    More actions

    Triage: the linked PR is merged, can this be closed?
12. mdboom commented on Jun 17, 2024

    

    mdboom

    on Jun 17, 2024

    ContributorAuthor

    More actions

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

    Reacted by Hugo van Kemenade
13. mdboom

    closed this as completedon Jun 17, 2024

Issue actions