Repository navigation
_multiprocessing leaks references when initialised multiple times #94382
Description
Activity
It seems the leak is with
SEM_VALUE_MAX, a static integer?
That could be solved by a PyGetSetDef getter.Reacted by Erlend E. AaslandRepeating #94336 (comment): AFAICS, the multiprocessing types do not access global module state, so there should be no reason to convert them to heap types.
It seems the leak is with SEM_VALUE_MAX, a static integer?
Where is that declared static? I don't see how it is considered a static integer. I am happy to be wrong though.
- changed the title
[-]Convert static types of _multiprocessing module to heap types[/-][+]_multiprocessing leaks references when initialised multiple times[/+]on Jun 28, 2022 I changed the issue title. It seems to me that the issue is a ref leak. The proposed solution is converting types to heap types. IMO, the GitHub Issue title should reflect the problem, not necessary the solution.
It seems to me that the issue is a ref leak.
Do you have a fix that does not use heap types or private APIs but fixes the issue?
Ah, terminology. It's not C
static, but it's always the same, and it's currently meant to be initialized only once (and it's not decref'd, so it leaks). You can't change the value from Python.Do you have a fix that does not use heap types or private APIs but fixes the issue?
I can send one later today, if nothing comes up.
I can send one later today, if nothing comes up.
Sure, please add me as a reviewer, I'll learn something new.
OK, I see now that the leak is not caused by SEM_VALUE_MAX, but (mostly?) by the method/getter/setter wrappers set by
PyType_Ready.
It seems to me that these are harmless: they won't be created a second time when_multiprocessingis imported again, and they can be reused in other interpreters. OTOH, they are flagged by-X showrefcount, and it would be nice to get those numbers to zero so-X showrefcountis useful in flagging new issues.@vstinner, are you aware of this issue with
-X showrefcountand static types with methods/getters/setters?In any case, the module state is unnecessary here.
Reacted by Erlend E. Aasland23 remaining items
For heap type conversion, PEP-687 explicitly applies to the following modules, as they contain types that access module state:
Modules/_asynciomodule.c
Modules/_ctypes/_ctypes.c
Modules/_datetimemodule.c
Modules/_decimal/_decimal.c
Modules/_pickle.c
Modules/_zoneinfo.c
Maybe I missed a module.Thanks Erlend for the list but it would have been better if the PEP had explicitly mentioned this list.
If there is no general consensus we can ping the SC. Either by asking them for advice, or by submitting another PEP for such issues.
This is what I was trying to say if there is no general agreement, we can ask the SC for decision/advice.
This is what I was trying to say if there is no general agreement, we can ask the SC for decision/advice
Perhaps there is a misunderstanding about what "general agreement" means. If this group (those involved on this issue) reach an agreement, that is not a "general agreement". A general agreement can only be reached amongst all core devs. Thus, the next level is creating a topic on Discourse, as has been mentioned multiple times already.
@erlend-aasland: I'll create a discourse topic to discuss to this soon.
Also, I apologies incase any of my wording sounded a bit rude but it was not my intension to hurt anyone here.
Hi,
I agree that the change looks good from what I know now, but I don't think I (or any other single person) should make the decision.
As Victor wrote:Things changed multiple times, I failed to keep track of these issues. But well, there are complex issues ;-)
For complex issues, we have the PEP process. It's not just for getting agreement, but also for untangling/explaining the complexity. And while this change is small enough that a full PEP might not be needed, IMO it does need a clear explanation of what's wrong and why heap types are the best solution. And perhaps we'll not get there with just a discussion thread.
This is not fixing a bug that users are seeing. It's a trade-off between correctness/maintainability and (percieved?) performance. There might be other issues we're not seeing.(FWIW, I also fail to keep track of these issues, as you can see from the fact that this was not covered in the PEP.)
Reacted by Erlend E. AaslandDiscourse topic created here: https://discuss.python.org/t/extension-modules-with-static-types-and-python-embedding-and-subinterpreters/16971
In the topic you still say this is a problem for embedders and multiple interpreters aka subinterpreters which in embedding case will leak memory whenever Python is initialized and finalized one or more times in the same process, while I still think the extra memory gets allocated only once per process. Am I wrong? Could you show a case where there is an unlimited memory leak?
Reacted by Erlend E. AaslandAm I wrong? Could you show a case where there is an unlimited memory leak?
Note that memory leak is much less in Python 3.12 and 3.11 as the runtime finalization even clears static types.
I have seen more complicated cases where gc is involved and if the object survives after its interpreter is freed.from test import support import sys for i in range(1, 10): support.run_in_subinterp("import multiprocessing") print("refs:", sys.gettotalrefcount(), "blocks: ", sys.getallocatedblocks())
Even if we consider the case for multiple finalization is less important still python leaks memory at exit even if it is initialized only once. Let's continue the discussion on discourse.
Output of the script:
refs: 161895 blocks: 47464 refs: 161958 blocks: 47484 refs: 162016 blocks: 47504 refs: 162074 blocks: 47524 refs: 162132 blocks: 47544 refs: 162190 blocks: 47564 refs: 162248 blocks: 47584 refs: 162306 blocks: 47604 refs: 162364 blocks: 47624For me, it's a real memory leak. For the specific case of PR #94336, I don't think that we have to involve the SC or write a PEP. I reviewed the PR. IMO if you make requested changes, the PR can be merged.
IMO if you make requested changes, the PR can be merged.
Requested changes have been made and also news entry added.
Kumar, could you please update the PR title and NEWS entry to more accurately describe the change? Apart from that, I'm fine with this.
Thanks for raising the discussion! 🙂
(Sorry, this comment was intended for the PR)
Kumar, could you please update the PR title and NEWS entry to more accurately describe the change? Apart from that, I'm fine with this.
"port multiprocessing static types to heap types" seems fine but I am open to suggestions. Feel free to edit it as you wish :)
Thanks for raising the discussion! 🙂
Thanks! :)
Fixed in
mainwith #94336
By converting
_multiprocessingto heap types, the module can be used in subinterpreters and does not leak memory when Python finalizes or when it is initialized multiple times. The converted heap types are not performance critical.Current Memory leak at exit:
[170 refs, 71 blocks]See PR #94336