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-103059: Clarify gc.freeze documentation #103058

Merged
merged 3 commits into from
Apr 10, 2023
Merged

gh-103059: Clarify gc.freeze documentation #103058

merged 3 commits into from
Apr 10, 2023

Conversation

raylu
Copy link
Contributor

@raylu raylu commented Mar 27, 2023

The current advice seems to mean you should gc.disable() and gc.freeze() right before os.fork(), which doesn't do anything.

Łukasz explained the advice in https://bugs.python.org/issue31558#msg302780.

This moves the advice around gc.disable out of the gc.freeze docs.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Mar 27, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@raylu raylu changed the title Move fork advice from gc.freeze to gc.disable gh-103059: Move fork advice from gc.freeze to gc.disable Mar 27, 2023
@raylu raylu marked this pull request as ready for review March 27, 2023 18:46
@raylu raylu requested a review from pablogsal as a code owner March 27, 2023 18:46
@arhadthedev arhadthedev added the docs Documentation in the Doc dir label Mar 28, 2023
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal and PR!

I definitely agree that the gc.freeze() docs should be cleaned up and clarified, but I don't agree with adding a bunch of fork-specific text to the gc.disable() docs. There are lots of possible reasons to disable GC; "I'm going to fork-without-exec later" is only one of them, and probably not the most common. And in that use case, gc.disable() is still useless unless you also gc.freeze(). So the only API that is specific to the fork-without-exec use case is gc.freeze(), and the explanation of how to handle fork-without-exec belongs there.

Here's my full suggested rewrite for the gc.freeze() documentation:

Freeze all objects tracked by the garbage collector: move them to a permanent generation and ignore them in all future collections.

If a process will fork() without exec(), avoiding unnecessary copy-on-write in child processes will maximize memory sharing and reduce overall memory usage. This requires both avoiding creation of freed "holes" in memory pages in the parent process, and ensuring that GC collections in child processes won't touch the memory of long-lived objects originating in the parent process. To accomplish both, call gc.disable() early in the parent process, gc.freeze() right before fork(), and gc.enable() early in child processes.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

raylu and others added 2 commits March 28, 2023 19:57
The current advice seems to mean you should gc.disable() and gc.freeze()
right before os.fork(), which doesn't do anything.

Łukasz explained the advice in
https://bugs.python.org/issue31558#msg302780.

This moves the advice around gc.disable out of the gc.freeze docs.
@raylu
Copy link
Contributor Author

raylu commented Mar 29, 2023

I left in the bit about gc_refs (I feel like this was critical to my understanding of freeze) but otherwise went with your explanation and left gc.disable() untouched

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@carljm: please review the changes made to this pull request.

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This change looks accurate and like a solid improvement to me. Thanks for the PR!

Process nit for future contributions to CPython: it's better to never force-push your PR. It can make life more difficult for reviewers (can't see changes since previously-reviewed version), and there's no benefit, since we always squash on merge anyway.

@carljm carljm changed the title gh-103059: Move fork advice from gc.freeze to gc.disable gh-103059: Clarify gc.freeze documentation Mar 29, 2023
@bedevere-bot

This comment was marked as outdated.

@carljm carljm added the needs backport to 3.11 only security fixes label Apr 10, 2023
@carljm carljm merged commit 8b1b171 into python:main Apr 10, 2023
@miss-islington
Copy link
Contributor

Thanks @raylu for the PR, and @carljm for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-103416 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 10, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 10, 2023
(cherry picked from commit 8b1b171)

Co-authored-by: raylu <lurayl@gmail.com>
carljm pushed a commit that referenced this pull request Apr 10, 2023
gh-103059: Clarify gc.freeze documentation (GH-103058)
(cherry picked from commit 8b1b171)

Co-authored-by: raylu <lurayl@gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants