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-122697: Fix free-threading memory leaks at shutdown #122703

Merged
merged 10 commits into from
Aug 8, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Aug 5, 2024

We were not properly accounting for interpreter memory leaks at shutdown and had two sources of leaks:

  • Objects that use deferred reference counting and were reachable via static types outlive the final GC. We now disable deferred reference counting on all objects if we are calling the GC due to interpreter shutdown.

  • _PyMem_FreeDelayed did not properly check for interpreter shutdown so we had some memory blocks that were enqueued to be freed, but never actually freed.

We were not properly accounting for interpreter memory leaks at
shutdown and had two sources of leaks:

 * Objects that use deferred reference counting and were reachable via
   static types outlive the final GC. We now disable deferred reference
   counting on all objects if we are calling the GC due to interpreter
   shutdown.

 * `_PyMem_FreeDelayed` did not properly check for interpreter shutdown
   so we had some memory blocks that were enqueued to be freed, but
   never actually freed.
@colesbury
Copy link
Contributor Author

!buildbot nogil refleaks

@bedevere-bot
Copy link

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

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

The builders matched are:

  • aarch64 Fedora Rawhide NoGIL refleaks PR
  • PPC64LE Fedora Rawhide NoGIL refleaks PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Fedora Rawhide NoGIL refleaks PR

@colesbury colesbury requested a review from DinoV August 5, 2024 19:30
@colesbury colesbury marked this pull request as ready for review August 5, 2024 19:30
@colesbury colesbury requested a review from encukou August 5, 2024 19:31
@Eclips4
Copy link
Member

Eclips4 commented Aug 5, 2024

The refleak buildbots haven't started yet, but I suspect there are a lot of leaks due to #122704, so the results will not be reliable until fix (#122705) is merged.

@colesbury
Copy link
Contributor Author

!buildbot nogil refleaks

@bedevere-bot
Copy link

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

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

The builders matched are:

  • aarch64 Fedora Rawhide NoGIL refleaks PR
  • PPC64LE Fedora Rawhide NoGIL refleaks PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Fedora Rawhide NoGIL refleaks PR

// Free immediately if the world is stopped, including during
// interpreter shutdown.
if (Py_IsFinalizing() || _PyRuntime.stoptheworld.world_stopped) {
// Free immediately during interpreter shutdown or if the world is
Copy link
Contributor

Choose a reason for hiding this comment

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

runtime shutdown would be better instead of interpreter shutdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this should be interpreter shutdown. That's why the refleak buildbots had failures.

@colesbury
Copy link
Contributor Author

!buildbot nogil refleaks

@bedevere-bot
Copy link

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

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

The builders matched are:

  • aarch64 Fedora Rawhide NoGIL refleaks PR
  • PPC64LE Fedora Rawhide NoGIL refleaks PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Fedora Rawhide NoGIL refleaks PR

@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 7, 2024
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 7, 2024
@encukou
Copy link
Member

encukou commented Aug 8, 2024

+1: I don't know my way around this code, but I don't see anything wrong with the PR.

Copy link
Contributor

@DinoV DinoV left a comment

Choose a reason for hiding this comment

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

LGTM!

@colesbury colesbury merged commit 2d9d3a9 into python:main Aug 8, 2024
53 checks passed
@colesbury colesbury deleted the gh-122697-memory-leaks branch August 8, 2024 16:48
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
…122703)

We were not properly accounting for interpreter memory leaks at
shutdown and had two sources of leaks:

 * Objects that use deferred reference counting and were reachable via
   static types outlive the final GC. We now disable deferred reference
   counting on all objects if we are calling the GC due to interpreter
   shutdown.

 * `_PyMem_FreeDelayed` did not properly check for interpreter shutdown
   so we had some memory blocks that were enqueued to be freed, but
   never actually freed.

 * `_PyType_FinalizeIdPool` wasn't called at interpreter shutdown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants