GH-94382: port multiprocessing static types to heap types

[kumaraditya303]

• Issue: _multiprocessing leaks references when initialised multiple times #94382

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 100ad99 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[kumaraditya303]

Refleaks bots are failing because of #94330

[erlend-aasland]

@kumaraditya303, thanks for the PR. One of the most important aims of PEP-687 is to communicate the changes before they happen. See Part 1: Preparation. I'd prefer if you please re-read the PEP and follow its guidelines as specified. You'll notice that opening a PR comes in the second half of the process.

[kumaraditya303]

One of the most important aims of PEP-687 is to communicate the changes before they happen. See Part 1: Preparation. I'd prefer if you please re-read the PEP and follow its guidelines as specified. You'll notice that opening a PR comes in the second half of the process.

I'll create an issue to discuss it but I don't expect much as the module has no global state and the implemented types are not performance critical. I had worked on this months ago just was waiting for PEP to get accepted :)

[erlend-aasland]

[...] as the module has no global state and the implemented types are not performance critical [...]

If the types do not access global module state they should remain static; this is explicitly noted in the PEP.

[erlend-aasland]

I'll create an issue to discuss it but I don't expect much [...]

Well, the important thing is that it gets done. Lack of communication is the single one issue that has contributed to heating the discussions regarding these changes. Even if we expect few people to participate in such a topic, it is of importance to raise awareness (and then wait at least for a week or two before continuing).

[kumaraditya303]

@erlend-aasland: Let's continue the discussion on the issue #94382

[kumaraditya303]

Removing do-not-merge as no alternative PR has been proposed so far.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

[erlend-aasland]

Removing do-not-merge as no alternative PR has been proposed so far.

Well, only three hours passed. That's a little narrow time-frame, don't you agree? :)

[erlend-aasland]

As of Petr's last comment, please re-label this with do-not-merge until there is an agreement.

[vstinner]

Please add a NEWS entry explaining that the purpose of this PR is to fix memory leaks when the _multiprocessing extension module is loaded/unloaded multiple times: #94382 (comment)

[kumaraditya303]

Please add a NEWS entry explaining that the purpose of this PR is to fix memory leaks when the _multiprocessing extension module is loaded/unloaded multiple times: #94382 (comment)

This means I can remove do-not-merge, right ?

[vstinner]

This means I can remove do-not-merge, right ?

That's unrelated. @erlend-aasland wrote:

As of Petr's last comment, please re-label this with do-not-merge until there is an agreement.

Since @erlend-aasland and @encukou wrote PEP 687, I understand that you need to agree on that specific PR.

[vstinner]

Sorry, I'm no longer convinced that this PR fix an actual memory leak. The NEWS entry no looks wrong to me.

Test from #94382 (comment):

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()) 

Without this PR, Python leaks memory: refs +469, blocks +160.

refs: 162138 blocks: 47456 refs: 162201 blocks: 47476 refs: 162259 blocks: 47496 refs: 162317 blocks: 47516 refs: 162375 blocks: 47536 refs: 162433 blocks: 47556 refs: 162491 blocks: 47576 refs: 162549 blocks: 47596 refs: 162607 blocks: 47616 

With this PR (rebased to main), Python still leaks memory: refs +469, blocks +160 (no change).

refs: 162138 blocks: 47446 refs: 162201 blocks: 47466 refs: 162259 blocks: 47486 refs: 162317 blocks: 47506 refs: 162375 blocks: 47526 refs: 162433 blocks: 47546 refs: 162491 blocks: 47566 refs: 162549 blocks: 47586 refs: 162607 blocks: 47606 

Tell me if I did something wrong.

---

If I modify the test to import the _multiprocessing extension instead, I see no leak on the Python main branch (without this PR):

refs: 162674 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 refs: 162679 blocks: 49018 

Note: I added an empty line for readability.

[kumaraditya303]

I had only verified single leak #94382 (comment) which is fixed by this PR. If the multiple initialization is not fixed, there must be something in multiprocessing module which is causing it. Anyways, I'll wait for decision on the discourse now before putting anymore effort into this as there are more reason for this change other than multiple initialization.

[erlend-aasland]

This means I can remove do-not-merge, right ?

Please wait until there is an agreement for this change on Discourse.

[erlend-aasland]

FYI, I'll merge this in a day or two.

[erlend-aasland]

I'm hesitant to backport this change. Let me know if you disagree, Victor / Petr.

[erlend-aasland]

Thanks, Kumar!