-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
GH-124742: Add the PYTHON_GC_THRESHOLD
environment variable.
#124743
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
+54
−0
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
Misc/NEWS.d/next/Core_and_Builtins/2024-09-28-20-01-40.gh-issue-124742.7kJx88.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Add the :envvar:`PYTHON_GC_THRESHOLD` environment variable. This can be used | ||
to set the ``threshold0`` value for the garbage collector, similar to calling | ||
``gc.set_threshold()``. If the value is empty or ``default``, the default | ||
threshold value is used. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why ignore the error? If a user accidentally sets the variable to something that doesn't work, then they might end up very confused as to why Python is ignoring their request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping this change can be included in the 3.13 RC and so I wanted it as simple and robust as possible. In the 'main' branch, I plan to re-work it so it becomes part of the
config
structure and I would add an error raise there.Maybe it is safe enough to raise an error here too. That would be consistent with the principle that "errors should not pass silently".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that making it return an error rather than nothing should still remain fairly simple,
return
will just becomereturn _PyStatus_ERR("...")
and whatnot.I haven't paid too much attention to the incremental GC problem (#124567), but I'm assuming that this is a possible solution for it? That should help it go into the 3.13 release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think this would be enough for us to feel safe about including incremental GC into 3.13. In retrospect, the incremental GC should have been opt-in for 3.13 and would become default in 3.14 if people had good experience with it.
The point of this env var is to increase the chances that people will test their programs with a higher threshold with 3.13 and then when 3.14 release nears, we would have a better idea about how "aggressive" the default GC tuning should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning an error
_PyGC_Init
results in this kind of error:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. We do the same for other env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have already reached the
feature-freeze
stage and this is a new feature, it needs confirmation from @Yhg1s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. It's Thomas's call and I completely understand if he doesn't want it. There has already been a bit too much excitement with the RCs.