Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-132917: Check resident set size (RSS) before GC trigger.#133399

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nascheme merged 9 commits intopython:mainfromnascheme:gc_check_rss
May 5, 2025

Conversation

nascheme
Copy link
Member

@naschemenascheme commentedMay 4, 2025•
edited
Loading

This greatly reduces the time to run the script inGH-132917. For the default build, I get 0.19 seconds to run. For free-threaded build main branch, 1.9 seconds. With this PR applied, 0.2 seconds.

I've tried to implement aget_current_rss() function that works on Linux, MacOS, Windows, FreeBSD and OpenBSD. If the platform doesn't have an implementation or the function fails,gc_should_collect() reverts to the old behavior.

I did not implement this for the default build GC yet. I think we should try it in 3.14 for only the free-threaded build. If it seems to work okay, we can add a similar RSS check for the default GC collection threshold check.

As conditions, I'm using RSS increase of 10% or young object count increase of 40 x threshold. I think this is quite conservative in terms of running the GC often enough.

stonebig reacted with heart emoji
@naschemenascheme changed the titleCheck resident set size (RSS) before GC trigger.gh-132917: Check resident set size (RSS) before GC trigger.May 4, 2025
@naschemenascheme added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 4, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@nascheme for commit5e965ab 🤖

Results will be shown at:

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

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 4, 2025
@naschemenascheme requested a review frompablogsalMay 4, 2025 21:29
@naschemenascheme added topic-free-threading performancePerformance or resource usage labelsMay 4, 2025
@naschemenascheme marked this pull request as ready for reviewMay 4, 2025 23:13
@naschemenascheme requested a review fromcolesburyMay 4, 2025 23:13
Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

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

A few comments below, but looks good to me.

Copy link
Member

@pablogsalpablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

I like this approach is simple and elegant and will work most of the time. I am slightly worried about Python objects that hold other resources that are not just memory not being accounted for and maybe how SWAP can affect this numbers.

For example in my mac laptop and I can prevent a GC run by opening many chrome tabs and forcing a swap to disk causing the RSS to drop and raise again. I don't think is a problem but maybe in the future we can use heap memory instead or a combined value?

@naschemenaschemeenabled auto-merge (squash)May 5, 2025 16:55
@nascheme
Copy link
MemberAuthor

For example in my mac laptop and I can prevent a GC run by opening many chrome tabs and forcing a swap to disk causing the RSS to drop and raise again. I don't think is a problem but maybe in the future we can use heap memory instead or a combined value?

Yeah, that's probably a good idea. Instead ofget_current_rss() it could return some estimate of total memory usage by the process. Thedeferred_count > 40*threshold condition will hopefully save us in the case of swapping. The GC will eventually run.

@naschemenascheme merged commit5c245ff intopython:mainMay 5, 2025
39 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@colesburycolesburycolesbury approved these changes

@pablogsalpablogsalpablogsal approved these changes

Assignees
No one assigned
Labels
performancePerformance or resource usagetopic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@nascheme@bedevere-bot@colesbury@pablogsal
[8]ページ先頭
©2009-2025 Movatter.jp