GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-112529: Make the GC scheduling thread-safe by colesbury · Pull Request #114880 · python/cpython

Conversation

colesbury commented Feb 1, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

The GC keeps track of the number of allocations (less deallocations) since the last GC. This change buffers the allocation count in thread-local state and uses atomic operations to modify the per-interpreter count. The thread-local buffering avoids contention on shared state.

A consequence is that the GC scheduling is not as precise, so "test_sneaky_frame_object" is skipped because it requires that the GC be run exactly after allocating a frame object.

Issue: Make the garbage collector thread-safe in --disable-gil builds #112529

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-112529: Make the GC scheduling thread-safe

5f53ebd

The GC keeps track of the number of allocations (less deallocations)
since the last GC. This buffers the count in thread-local state and uses
atomic operations to modify the per-interpreter count. The thread-local
buffering avoids contention on shared state.

A consequence is that the GC scheduling is not as precise, so
"test_sneaky_frame_object" is skipped because it requires that the GC be
run exactly after allocating a frame object.

colesbury requested review from DinoV, nascheme and pablogsal

February 1, 2024 21:21

colesbury requested review from ericsnowcurrently and markshannon as code owners

February 1, 2024 21:21

bedevere-app Bot added the awaiting review label

Feb 1, 2024

bedevere-app Bot mentioned this pull request

Feb 1, 2024

Make the garbage collector thread-safe in --disable-gil builds #112529

Closed

3 tasks

colesbury added the skip news label

Feb 1, 2024

ericsnowcurrently reviewed

Feb 2, 2024

View reviewed changes

ericsnowcurrently left a comment

Copy link

Copy Markdown

Member

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.

The GC is one of the runtime components with which I am least familiar, so I mostly have questions for you. 😄

Otherwise, the PR mostly makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/gc_free_threading.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/gc_free_threading.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/gc_free_threading.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Fix warning

e832dd9

ericsnowcurrently commented Feb 2, 2024

Copy link

Copy Markdown

Member

The change here seems okay to me, but I'd feel better if one of the GC experts reviewed this before it's merged.

CC @markshannon @pablogsal @nascheme @DinoV @nanjekyejoannah

colesbury reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nascheme commented Feb 4, 2024

Copy link

Copy Markdown

Member

I've not looked at the code but the idea of the change sounds fine to me. I suspect there are some users who require that the GC threshold is precise, like the test_sneaky_frame_object case. However, I don't think that's a reasonable thing and I think it's okay to break them. We are pretty likely to adjust how the thresholds work anyhow. Using atomic operations to count allocations/dellocations will be too expensive.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury added 2 commits

February 6, 2024 15:38


          Merge branch 'main' into pythongh-112529-gc-schedule

483b37e


          Skip test_gc.test_get_count() in builds

456b778

colesbury mentioned this pull request

Feb 9, 2024

gh-112175: Add eval_breaker to PyThreadState #115194

Merged


          Merge branch 'main' into pythongh-112529-gc-schedule

f99d14e

DinoV reviewed

Feb 14, 2024

View reviewed changes

Comment thread

Python/gc_free_threading.c

		// We buffer the allocation count to avoid the overhead of atomic
		// operations for every allocation.
		gc->alloc_count++;
		if (gc->alloc_count >= LOCAL_ALLOC_COUNT_THRESHOLD) {

DinoV Feb 14, 2024

Copy link

Copy Markdown

Contributor

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 wonder if this could be tied to the configurable GC threshold and therefore the tests could continue to pass but maybe it doesn't matter enough and the extra read isn't worth it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

colesbury Feb 14, 2024

Copy link

Copy Markdown

Contributor Author

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.

Yeah, I considered making it a configurable runtime threshold, but decided it wasn't worth it, at least for now.

I think there's a decent chance we change how we count allocations in the future. In the nogil forks, for example, I accounted for allocations in mi_page_to_full and _mi_page_unfull, which provides some natural batching and avoids the thread-local that's done in every allocation here, but wouldn't allow for a configurable threshold. I haven't attempted that yet because I'd like some performance measurements to justify it first.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

DinoV approved these changes

Feb 14, 2024

View reviewed changes

DinoV left a comment

Copy link

Copy Markdown

Contributor

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.

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting review labels

Feb 14, 2024

colesbury added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Feb 14, 2024

bedevere-bot commented Feb 14, 2024

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @colesbury for commit f99d14e 🤖

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Feb 14, 2024

colesbury merged commit b24c916 into python:main

Feb 16, 2024

colesbury deleted the gh-112529-gc-schedule branch

February 16, 2024 16:22

bedevere-app Bot removed the awaiting merge label

Feb 16, 2024

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024


          pythongh-112529: Make the GC scheduling thread-safe (python#114880)

a2a0dd9

The GC keeps track of the number of allocations (less deallocations)
since the last GC. This buffers the count in thread-local state and uses
atomic operations to modify the per-interpreter count. The thread-local
buffering avoids contention on shared state.

A consequence is that the GC scheduling is not as precise, so
"test_sneaky_frame_object" is skipped because it requires that the GC be
run exactly after allocating a frame object.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          pythongh-112529: Make the GC scheduling thread-safe (python#114880)

52df553

The GC keeps track of the number of allocations (less deallocations)
since the last GC. This buffers the count in thread-local state and uses
atomic operations to modify the per-interpreter count. The thread-local
buffering avoids contention on shared state.

A consequence is that the GC scheduling is not as precise, so
"test_sneaky_frame_object" is skipped because it requires that the GC be
run exactly after allocating a frame object.

LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request

Jan 22, 2025


          pythongh-112529: Make the GC scheduling thread-safe (python#114880)

df74dc2

The GC keeps track of the number of allocations (less deallocations)
since the last GC. This buffers the count in thread-local state and uses
atomic operations to modify the per-interpreter count. The thread-local
buffering avoids contention on shared state.

A consequence is that the GC scheduling is not as precise, so
"test_sneaky_frame_object" is skipped because it requires that the GC be
run exactly after allocating a frame object.

Reviewers

ericsnowcurrently

ericsnowcurrently left review comments

DinoV

DinoV approved these changes

nascheme

Awaiting requested review from nascheme

pablogsal

Awaiting requested review from pablogsal

pablogsal is a code owner

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

colesbury commented Feb 1, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently commented Feb 2, 2024

Uh oh!

nascheme commented Feb 4, 2024

Uh oh!

DinoV Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

DinoV left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 14, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colesbury commented Feb 1, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.