gh-142476: fix memory leak when creating JIT executors #142492
Conversation
ashm-dev
changed the title
|
I believe a news entry is not required for this change. |
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Wow, great catch!
Could you please add a comment(review below) so that we know why this is needed?
|
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? |
|
Yes, applying the changes from #137016 fixes the Windows crash. |
|
We need this to be merged first https://github.com/python/cpython/pull/137016/files |
|
Also, I'm not sure I get why we DECREF Maybe @markshannon can shed a light to this, because of #118459 |
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. |
|
@Fidget-Spinner Thanks! |
|
The crashes were caused by a double untrack exposed by the fix.
The fix: Guard the untrack call with |
|
@ashm-dev that's a great observation. We need a news entry though, a short one is fine. Since this fixes a memory leak in CPython those usually need news entries. |
|
@Fidget-Spinner I've added the news entry. |
|
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? |
|
Got it, sounds good — I’ll do that. |
| _PyObject_GC_UNTRACK(self); | ||
|
|
||
| // Object might be already untracked if we are in a GC cycle (via tp_clear). | ||
| // Avoid double-untracking. |
There was a problem hiding this comment.
Calling uop_dealloc twice means deallocating one executor twice. It seems like we're fixing symptoms, not the root of the problem.
There was a problem hiding this comment.
I can add a TODO for now so we don’t lose track of this, but I don’t have the bandwidth at the moment to dig into the root cause and fix it properly.
picnixz
changed the title
3 participants
Uh oh!
There was an error while loading. Please reload this page.