GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-115184: Fix refleak tracking issues in free-threaded build by colesbury · Pull Request #115188 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Feb 8, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • We did not account for abandoned segments, which could miss blocks in programs that use multiple threads.
  • The mimalloc heap traversal needed to call _mi_page_free_collect earlier in order to get an accurate count of live blocks.
  • _Py_DecRefSharedDebug was missing a _Py_IncRefTotal, but this was mostly offset by a missing accounting in _Py_ExplicitMergeRefcount.
  • get_num_global_allocated_blocks should pause other threads to ensure that traversing the mimalloc heaps is safe.
  • Issue: Fix refleak tracking in free-threaded build #115184

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
pythongh-115184: Fix refleak tracking issues in free-threaded build
1b17640 
- We did not account for abandoned segments, which could miss blocks in
  multithreaded runs.
- The mimalloc heap traversal needed to call "_mi_page_free_collect"
  earlier in order to get an accurate count of blocks in use.
- `_Py_DecRefSharedDebug` was missing a `_Py_IncRefTotal`, but this was
  mostly offset by a missing accounting in `_Py_ExplicitMergeRefcount`.
- get_num_global_allocated_blocks should pause other threads to ensure
  that traversing the mimalloc heaps is safe.
@colesbury colesbury added skip news topic-free-threading labels Feb 8, 2024
@colesbury
colesbury requested a review from DinoV February 8, 2024 21:58
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 8, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 8, 2024
Fix refleak tracking in free-threaded build #115184
Closed
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
@bedevere-bot

bedevere-bot commented Feb 8, 2024

Copy link
Copy Markdown

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
DinoV
DinoV reviewed Feb 9, 2024
View reviewed changes
Comment thread Objects/object.c
_Py_AddRefTotal(_PyInterpreterState_GET(), extra);
#endif

_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0);

@DinoV DinoV Feb 9, 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.

Do I understand this correctly that we're not racing here because we're either own the owning thread or the world is stopped for GC?

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Feb 9, 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.

The possible cases are:

  1. We are the owning thread and gave up ownership (typical)
  2. We are the queueing thread and the owning thread has exited (gh-110481: Implement inter-thread queue for biased reference counting #114824)
  3. We are the queueing thread and have stopped the world (due to OOM) (gh-110481: Implement inter-thread queue for biased reference counting #114824)

The GC could have used this code path, but it uses a different function that avoids atomics (because it's stopped the world).

The reason case (2) is thread-safe is a bit subtle:

  • Queueing is done once, so only one thread will attempt it
  • Although thread ids may be reused, the merge for (2) happens while holding a lock that would need to be acquired by any new thread that reuses the thread id.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DinoV
DinoV approved these changes Feb 9, 2024
View reviewed changes

@DinoV 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 bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 9, 2024
@colesbury
colesbury merged commit 31633f4 into python:main Feb 9, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 9, 2024
@colesbury
colesbury deleted the gh-115184-refleak branch February 9, 2024 14:23
@colesbury colesbury mentioned this pull request Feb 9, 2024
gh-110481: Implement inter-thread queue for biased reference counting #114824
Merged
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@colesbury @fsc-eriker
pythongh-115184: Fix refleak tracking issues in free-threaded build (p…
000f444 
…ython#115188)

Fixes a few issues related to refleak tracking in the free-threaded build:

- Count blocks in abandoned segments
- Call `_mi_page_free_collect` earlier during heap traversal in order to get an accurate count of blocks in use.
- Add missing refcount tracking in `_Py_DecRefSharedDebug` and `_Py_ExplicitMergeRefcount`.
- Pause threads in  `get_num_global_allocated_blocks` to ensure that traversing the mimalloc heaps is safe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DinoV DinoV DinoV approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-free-threading

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.

3 participants

@colesbury @bedevere-bot @DinoV
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 01402503 — 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.