[Python] Set memory policy to "strict"

[guitargeek]

The ROOT Python interfaces have many memory leaks, which is a major pain point for people using it for long-running scripts in batch jobs.

One source of memory leaks was indentified to be the "heuristic memory policy" of cppyy. This means that cppyy assumes that every non-const pointer member function argument was interpreted as the object taking ownership if the argument.

For examle, take the non-owning RooLinkedList container. It has a RooLinkedList::Add(RooAbsArg *arg) method. ROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the ROOT ownership. Nobody feels responsible for deleting the object anymore, and there is a memory leak or arg.

That particular leak was reported in this forum post:
https://root-forum.cern.ch/t/memory-leak-in-fits/56249

Function parameters of type T * are very common in ROOT, and only rarely do they imply ownership transfer. So changing the memory policy to "strict" would surely fix also many other memory leaks that are not reported so far. In fact, upstream cppyy doesn't even enable this heuristic memory policy anymore by default! So moving ROOT also to the strict memory policy closes the gap between ROOT and cppyy.

The potential drawback of this change are crashes in usercode if memory is not properly managed. But these problems should either be fixed by:

• the user

• dedicated pythonizations for these methods to manage shared ownership via Python reference counters (i.e., setting the parameter as an attribute of the object that the member function was called on)

This follows up on PR #4294, in particular it reverts guitargeek@3a12063.

Note that owning TCollection, TSeqCollection, and TList instances are Pythonized to preserve the old behavior of dropping Python ownership of added elements, as that was the case where the memory heuristic was correct.

[guitargeek]

Hi @vepadulano, thanks a lot for your review! I have updated the PR and the associated roottest PR

[dpiparo]

restarted all builds due to the issues with the server serving files for the tests.

[vepadulano]

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

[guitargeek]

Hi @vepadulano, thanks for coming back to this! Let's try to make progress then and merge this soon 🙂

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Good points. I'll address them once we see where we stand with a fresh CI run.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

The upcoming cppyy rebase on libinterop is more happening in the backend, and we have no frontend development lined up that change the behavior of these memory heuristics. But what we should consider is that it's better to not change both backend and frontend too much in one release, because it makes potential regressions harder to fix. If we assume that the backend upgrade will hit in 6.42, then this means the best time to update the memory policy is now for 6.40. Otherwise, it would be wiser to wait for 6.44, but this is quite far away.

[guitargeek]

Hi @vepadulano, I have suggested a TList Pythonization now that might address your concerns.

The proposal is that when adding to owning TCollections (checked with IsOwner()), the ownership is always relinquished on the Python side, just like the old memory heuristics implied.

So that means that the behavior change is only for non-owning TCollections, which now add borrowed references instead of letting the elements leak when added from Python.

Is that an acceptable compromise? (no need to rush an answer, I still have to fix test failures with Python 3.14 for Fedora Rawhide 🙂 )

[vepadulano]

Thanks! I like the direction, I left a few more comments/questions.

[guitargeek]

So from the latest changes I guess that the default behaviour of TList is that it owns its elements, do you agree?

That's true only for lists returned by this create_tseqcollection function in the test, where it called SetOwner(True) inside. So one has to call SetOwner(False) afterwards to get a non-owning collection.

I agree that this is confusing. I'll make some improvement suggestions.

[guitargeek]

Okay I have figured out a simpler way to manage the object ownership in these T(Seq)Collection tests, which is flexible enough so it can also be spun of into a separate PR:

• [Python] Don't rely on possible memory leaks in T(Seq)Collection tests #21449

[guitargeek]

Hi @vepadulano! This PR is greatly simplified now, because I factored the technical changes beyond the one line that changes the memory policy out into separate PRs that are merged now.

Once our own CI is green, I'll request a CMSSW test run and then we can proceed here from my side.

[guitargeek]

Hi @smuzaffar, could you please test this PR with CMSSW? It's a potentially fragile change of behavior in our Python interface. Thanks!

[vepadulano]

Thanks! Although the changes are much fewer now, I would still run the CI with a clean build and with the Python wheel workflow.

[guitargeek]

Thank you very much @smuzaffar for testing! I see that the tests passed now, so everything good from my side.

@vepadulano, I have now also addressed your review comments.

[vepadulano]

LGTM, very nice!