Skip to content

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Feb 8, 2024

Counting when executors are invalidated seems like a useful statistic to have (see this discussion).

@markshannon
Copy link
Member

Does this count executors that are invalidated during interpreter shutdown, or have we turned off stats by then?

@mdboom
Copy link
Contributor Author

mdboom commented Feb 9, 2024

Does this count executors that are invalidated during interpreter shutdown, or have we turned off stats by then?

Good call out. In the "normal" case where the user has turned on pystats directly, it would include the invalidations at shutdown, which are really teardowns, not invalidations. In the context of pyperformance and how we run benchmarks in our infrastructure, which turns pystats on/off around the benchmarking code, teardowns would be excluded from the number. (So the numbers I shared here should be correct).

I suppose the better option would be to count these one layer up where we know why the executors are being invalidated.

@mdboom
Copy link
Contributor Author

mdboom commented Feb 9, 2024

I've updated this to be more precise about when something is an invalidation vs. just a deallocation.

exec->vm_data.valid = false;
exec->vm_data.linked = false;
exec = next;
OPT_STAT_INC(executors_invalidated);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want stats for this, as it can be called when the builtins are modified, or other rare events.
Maybe add a flag parameter to distinguish between shutdown and actual invalidation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My approach was to move the counters to the callers, rather than a flag (but a flag is fine too if you prefer). In either case, it looks like I missed _PyInterpreterState_SetEvalFrameFunc: This counter should also be incremented there.

@mdboom mdboom requested a review from markshannon February 9, 2024 18:00
@mdboom mdboom force-pushed the pystats-executor-invalidated branch from c5de632 to db58a2f Compare February 12, 2024 16:33
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.

@mdboom
Copy link
Contributor Author

mdboom commented Feb 26, 2024

@markshannon: This has had the merge conflicts fixed and is ready to merge.

@Fidget-Spinner Fidget-Spinner enabled auto-merge (squash) February 26, 2024 17:17
@Fidget-Spinner Fidget-Spinner merged commit b05afdd into python:main Feb 26, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants