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-115103: Delay reuse of mimalloc pages that store PyObjects by colesbury · Pull Request #115435 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Feb 13, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This implements the delayed reuse of mimalloc pages that contain Python objects in the free-threaded build.

Allocations of the same size class are grouped in data structures called pages. These are different from operating system pages. For thread-safety, we want to ensure that memory used to store PyObjects remains valid as long as there may be concurrent lock-free readers; we want to delay using it for other size classes, in other heaps, or returning it to the operating system.

When a mimalloc page becomes empty, instead of immediately freeing it, we tag it with a QSBR goal and insert it into a per-thread state linked list of pages to be freed. When mimalloc needs a fresh page, we process the queue and free any still empty pages that are now deemed safe to be freed. Pages waiting to be freed are still available for allocations of the same size class and allocating from a page prevent it from being freed. There is additional logic to handle abandoned pages when threads exit.

See https://peps.python.org/pep-0703/#mimalloc-page-reuse

  • Issue: Add delayed reclamation mechanism for free-threaded build (QSBR) #115103

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury colesbury added the topic-free-threading label Feb 13, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 13, 2024
Add delayed reclamation mechanism for free-threaded build (QSBR) #115103
Closed
@colesbury colesbury added the skip news label Feb 13, 2024
@colesbury colesbury mentioned this pull request Feb 14, 2024
gh-115103: Implement delayed memory reclamation (QSBR) #115180
Merged
@colesbury
colesbury force-pushed the gh-115103-mimalloc-qsbr branch 2 times, most recently from b811432 to 99bd660 Compare March 5, 2024 00:04
@colesbury
colesbury marked this pull request as ready for review March 5, 2024 04:02
@colesbury
colesbury requested a review from ericsnowcurrently as a code owner March 5, 2024 04:02
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 5, 2024
@colesbury
colesbury requested review from DinoV and removed request for ericsnowcurrently March 5, 2024 04:03
@colesbury

colesbury commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

!buildbot nogil
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Mar 5, 2024

Copy link
Copy Markdown

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

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

  • AMD64 Ubuntu NoGIL Refleaks PR
  • x86-64 MacOS Intel ASAN NoGIL PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • x86-64 MacOS Intel NoGIL PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Ubuntu NoGIL PR
  • ARM64 MacOS M1 NoGIL PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
pythongh-115103: Delay reuse of mimalloc pages that store PyObjects
89d8cd6
DinoV
DinoV approved these changes Mar 5, 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.

Looks good to me, just a few nits really...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/obmalloc.c Outdated
mi_page_t *page = llist_data(node, mi_page_t, qsbr_node);
if (!mi_page_all_free(page)) {
// We allocated from this page some point after the delayed free
page->qsbr_goal = 0;

@DinoV DinoV Mar 5, 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.

nit: Seems like we could consistently use _PyMem_mi_page_clear_qsbr?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/obmalloc.c Outdated
return;
}

page->qsbr_goal = 0;

@DinoV DinoV Mar 5, 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.

_PyMem_mi_page_clear_qsbr?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/obmalloc.c Outdated
if (!_Py_qbsr_goal_reached(tstate->qsbr, page->qsbr_goal)) {
return false;
}
_PyMem_mi_page_clear_qsbr(page);

@DinoV DinoV Mar 5, 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.

This side effect seems surprising given the name of the function... It seems like it'd be better to call _PyMem_mi_page_clear_qsbr explicitly in mi_segment_page_clear and _PyMem_mi_page_maybe_free

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 5, 2024
@colesbury
Fix bug with reclaiming pages and refactor code.
1645d3a
@colesbury
colesbury force-pushed the gh-115103-mimalloc-qsbr branch from 99bd660 to 1645d3a Compare March 5, 2024 23:03
colesbury
colesbury commented Mar 5, 2024
View reviewed changes

@colesbury colesbury left a comment

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.

I've rebased now that #116343 landed.

  • I fixed a bug with reclaiming segments that led to the buildbot failures (see mi_segment_reclaim). Large pages only contain a single block. Prior to this PR, they were either empty and freed immediately on reclamation or full and so didn't set right_page_reclaimed. The right_page_reclaimed logic doesn't handle large pages properly because large pages use a different allocation code path than small and medium pages. This means we have slightly worse re-use than we'd like when reclaiming large pages, but at least it's correct now.
  • I changed _PyMem_mi_page_is_safe_to_free to only handle reclaimed pages, instead of also _PyMem_mi_page_maybe_free case. The qsbr_goal == 0 has different implications in the two cases. When reclaiming an abandoned page, qsbr_goal == 0 means that we can free it immediately, while in mi_heap_page_collect it means that we haven't started the QSBR process.
  • _mi_page_retire should clear the previous QSBR goal, if one exists (since we just freed a block).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DinoV reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@colesbury

colesbury commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

!buildbot nogil
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Mar 5, 2024

Copy link
Copy Markdown

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

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

  • x86-64 MacOS Intel ASAN NoGIL PR
  • ARM64 MacOS M1 NoGIL PR
  • AMD64 Ubuntu NoGIL PR
  • x86-64 MacOS Intel NoGIL PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Windows Server 2022 NoGIL PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Remove redundant _PyMem_mi_page_clear_qsbr call
3453eb9
@colesbury

colesbury commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

!buildbot nogil
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Mar 5, 2024

Copy link
Copy Markdown

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

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

  • x86-64 MacOS Intel ASAN NoGIL PR
  • ARM64 MacOS M1 NoGIL PR
  • AMD64 Ubuntu NoGIL PR
  • x86-64 MacOS Intel NoGIL PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Windows Server 2022 NoGIL PR
All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury commented Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

The failed buildbot looks unrelated to this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury merged commit c012c8a into python:main Mar 6, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 6, 2024
@colesbury
colesbury deleted the gh-115103-mimalloc-qsbr branch March 6, 2024 14:42
@ericsnowcurrently

ericsnowcurrently commented Mar 6, 2024

Copy link
Copy Markdown
Member

CC @daanx
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury colesbury mentioned this pull request Mar 6, 2024
Mimalloc differences from upstream #113141
Open
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@colesbury @adorilson
pythongh-115103: Delay reuse of mimalloc pages that store PyObjects (p…
ef3e9f5 
…ython#115435)

This implements the delayed reuse of mimalloc pages that contain Python
objects in the free-threaded build.

Allocations of the same size class are grouped in data structures called
pages. These are different from operating system pages. For thread-safety, we
want to ensure that memory used to store PyObjects remains valid as long as
there may be concurrent lock-free readers; we want to delay using it for
other size classes, in other heaps, or returning it to the operating system.

When a mimalloc page becomes empty, instead of immediately freeing it, we tag
it with a QSBR goal and insert it into a per-thread state linked list of
pages to be freed. When mimalloc needs a fresh page, we process the queue and
free any still empty pages that are now deemed safe to be freed. Pages
waiting to be freed are still available for allocations of the same size
class and allocating from a page prevent it from being freed. There is
additional logic to handle abandoned pages when threads exit.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@colesbury @diegorusso
pythongh-115103: Delay reuse of mimalloc pages that store PyObjects (p…
8757509 
…ython#115435)

This implements the delayed reuse of mimalloc pages that contain Python
objects in the free-threaded build.

Allocations of the same size class are grouped in data structures called
pages. These are different from operating system pages. For thread-safety, we
want to ensure that memory used to store PyObjects remains valid as long as
there may be concurrent lock-free readers; we want to delay using it for
other size classes, in other heaps, or returning it to the operating system.

When a mimalloc page becomes empty, instead of immediately freeing it, we tag
it with a QSBR goal and insert it into a per-thread state linked list of
pages to be freed. When mimalloc needs a fresh page, we process the queue and
free any still empty pages that are now deemed safe to be freed. Pages
waiting to be freed are still available for allocations of the same size
class and allocating from a page prevent it from being freed. There is
additional logic to handle abandoned pages when threads exit.
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.

4 participants

@colesbury @bedevere-bot @ericsnowcurrently @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.