root icon indicating copy to clipboard operation
root copied to clipboard

[PyROOT] Set memory policy to "strict"

Open guitargeek opened this issue 2 years ago • 2 comments

PyROOT has 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 PyROOT. This means that PyROOT 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. PyROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the PyROOT overship. 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 have this heuristic memory policy anymore! So moving PyROOT also to the strict memory policy closes the gap between PyROOT 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 3a12063116.

guitargeek avatar Sep 04 '23 13:09 guitargeek

Test Results

    19 files      19 suites   4d 8h 36m 16s ⏱️  2 723 tests  2 719 ✅ 0 💤 4 ❌ 49 994 runs  49 987 ✅ 0 💤 7 ❌

For more details on these failures, see this check.

Results for commit 262a8b41.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Sep 05 '23 10:09 github-actions[bot]

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

guitargeek avatar Feb 13 '24 17:02 guitargeek

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

dpiparo avatar Jan 02 '25 09:01 dpiparo