Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-142476: fix memory leak when creating JIT executors#142492
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ashm-dev commented Dec 9, 2025
I believe a news entry is not required for this change. |
Fidget-Spinner left a comment
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.
Wow, great catch!
Could you please add a comment(review below) so that we know why this is needed?
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented Dec 9, 2025
The failing Windows test is likely because of https://github.com/python/cpython/pull/137016/files. Could you please try pulling in the changes from there? |
ashm-dev commented Dec 10, 2025
Yes, applying the changes from #137016 fixes the Windows crash. |
Fidget-Spinner commented Dec 10, 2025
We need this to be merged first https://github.com/python/cpython/pull/137016/files |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sergey-miryanov commented Dec 12, 2025
Also, I'm not sure I get why we DECREF Maybe @markshannon can shed a light to this, because of #118459 |
Fidget-Spinner commented Dec 12, 2025
Detach is for removing the executor from a code object. The code object owns a strong reference to the executor so we must decref it. |
sergey-miryanov commented Dec 12, 2025
@Fidget-Spinner Thanks! |
ashm-dev commented Dec 12, 2025
The crashes were caused by a double untrack exposed by the fix.
The fix: Guard the untrack call with |
ashm-dev commented Dec 12, 2025
@Fidget-Spinner I've added the news entry. |
Fidget-Spinner commented Dec 13, 2025
So I'm gonna leave this around for a few more days. However could you please add a code comment above the if that tp_traverse might untrack the onject once it breaks cycles, so we need to not double-untrack it please? |
ashm-dev commented Dec 13, 2025
Got it, sounds good — I’ll do that. |
Uh oh!
There was an error while loading. Please reload this page.
sergey-miryanov commented Dec 19, 2025
ashm-dev commented Dec 19, 2025
@sergey-miryanov The project doesn't even build right now, so I'm unable to verify it. I'm building Python from the Build Logs |
Fidget-Spinner commented Dec 19, 2025
@ashm-dev does the project build if you apply this patch here to main? |
ashm-dev commented Dec 19, 2025
@Fidget-Spinner No, it doesn't build even with my patch applied. It looks like the JIT is broken again. |
Fidget-Spinner left a comment
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.
It's not broken (apart from the refcount leak I think), the current patch seems to be breaking it.
Uh oh!
There was an error while loading. Please reload this page.
ashm-dev commented Dec 19, 2025
But it doesn't build on main either, even without my patch. I posted the logs above. |
sergey-miryanov left a comment
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. Thanks!
Fidget-Spinner commented Dec 19, 2025
@ashm-dev I think you're misunderstanding me. I'm asking if it builds with this patch currently on this branch? |
ashm-dev commented Dec 19, 2025
@Fidget-Spinner Yes, it builds! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e2a7db7 into python:mainUh oh!
There was an error while loading. Please reload this page.
colesbury commented Dec 19, 2025
I'm seeing GHA segfaults in x86_64-pc-windows-msvc/msvc (Debug) on this commit and the subsequent one, but not on earlier commits: |
Uh oh!
There was an error while loading. Please reload this page.