Skip to content
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

GH-130396: Increase trashcan overhead #130552

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Feb 25, 2025

@markshannon
Copy link
Member Author

!buildbot Windows.*2022.*NoGIL

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 1a1a46c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130552%2Fmerge

The command will test the builders whose names match following regular expression: Windows.*2022.*NoGIL

The builders matched are:

  • AMD64 Windows Server 2022 NoGIL PR

@markshannon
Copy link
Member Author

!buildbot Windows.*2022.*NoGIL.*3

@bedevere-bot
Copy link

The regex 'Windows.*2022.*NoGIL.*3' did not match any buildbot builder. Is the requested builder in the list of stable builders?

@markshannon markshannon marked this pull request as ready for review February 25, 2025 16:52
@markshannon markshannon force-pushed the increase-trashcan-overhead branch from 1a1a46c to ba9702c Compare February 25, 2025 18:20
@brandtbucher
Copy link
Member

brandtbucher commented Feb 25, 2025

So, to make sure I understand:

  • We used to deposit into the trashcan once we only had room for 1 interpreter entry. Now we start using it earlier, once we only have room for 2.
  • We used to empty the trashcan if we had room for 2 interpreter entries, but now we only do it if we have room for 4.

Can you clarify why these new numbers are chosen? It seems like it made sense before: deposit if we don't have room for a finalizer (margin of 1), and don't empty if doing so would immediately start putting stuff back in the trashcan (margin of 2, one higher than what's used in Py_TRASHCAN_BEGIN).

Are the new numbers chosen just because they're "higher"? If so, it's probably PYOS_STACK_MARGIN that needs to be fixed, not this, since the accounting of the margins in these macros seems correct to me the way it currently is.

@markshannon
Copy link
Member Author

It is possible that the increase in PYOS_STACK_MARGIN from #130554 will fix the failure.

Even so, I think it is a good idea to have a bit more headroom for the trashcan, so that raising exceptions, calls to instrumentation, sys.audit, etc don't trigger a recursion error.

@markshannon markshannon merged commit 263d56e into python:main Feb 26, 2025
45 checks passed
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