[Python] Set memory policy to "strict" #13593

guitargeek · 2023-09-04T13:04:33Z

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 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 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.

Sister PR of root-project/root#13593.

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?

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

vepadulano · 2026-01-07T14:45:09Z

bindings/pyroot/pythonizations/test/tseqcollection_listmethods.py

+        # A TList is by default non-owning. To make sure that the objects live
+        # long enough, we attach then as an attribute of the output list, such
+        # that the Python reference counter doesn't hit zero.
+        sc.owning_pylist = sc_pylist


This sounds counter-intuitive for a Python user, shall we perhaps discuss making it part of a TList pythonization?

The TList Pythonization is now implemented.

README/ReleaseNotes/v636/index.md

vepadulano · 2026-01-07T14:48:06Z

README/ReleaseNotes/v636/index.md

+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!


We should make this perhaps explicit at usage by also raising a warning from the function when calling it.

We learned from the ROOT workshop that our users also prefer clear deprecation and removal schedules. Should we aim for removing ROOT.SetMemoryPolicy in 6.42? If people really want to change the policy, they can still use the Cppyy API after the removal of the ROOT wrapper though, so there is an escape hatch that makes the removal seem a bit less drastic. Of course we don't mention that in the release notes 🙂

I have added a clear deprecation and removal warning to the release notes and to ROOT.SetMemoryPolicy() in the latest iteration of this PR.

guitargeek · 2026-01-07T15:34:11Z

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 · 2026-01-09T16:45:29Z

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.

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py

vepadulano · 2026-01-12T09:53:41Z

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py


    def test_getitem_slice(self):
        sc = self.create_tseqcollection()
+        sc.SetOwner(False)


The fact you are now calling both SetOwner(True) and SetOwner(False) in different unittests confuses me as to which is the default behaviour (in C++ and Python) of the TCollection family of classes.

Especially important for documentation and to address potential future user questions

So from the latest changes I guess that the default behaviour of TList is that it owns its elements, do you agree? I would have guessed the contrary judging by the TList destructor at

root/core/cont/src/TList.cxx

Lines 85 to 92 in e3fba69

////////////////////////////////////////////////////////////////////////////////

/// Delete the list. Objects are not deleted unless the TList is the

/// owner (set via SetOwner()).

TList::~TList()

{

Clear();

}

. Is there maybe some difference between the C++ implementation and the Pythonization?

How did you infer that the default behavior is owning? Maybe I wrote something wrong somewhere? The default behavior is non-owning:

------------------------------------------------------------------ | Welcome to ROOT 6.38.00 https://root.cern | | (c) 1995-2025, The ROOT Team; conception: R. Brun, F. Rademakers | | Built for linuxx8664gcc on Nov 27 2025, 08:23:06 | | From tags/6-38-00@6-38-00 | | With g++ (GCC) 15.2.0 std201703 | | Try '.help'/'.?', '.demo', '.license', '.credits', '.quit'/'.q' | ------------------------------------------------------------------ root [0] TList l{}; root [1] l.IsOwner() (bool) false root [2]

The reason why I first do sc.SetOwner(True) is to make the list own the added elements. Then, I do SetOwner(False) after adding the elements, because some of the creates lists are implicitly transferring the ownership of the elements to another list, e.g. in these "append" unit tests.

BTW, I don't think that these T(Seq)Collection Pythonizations are used much in user code. They were silently introduced without tutorials, release notes, or documentation:

[Exp PyROOT] Add TCollection and TIter pythonisations and tests #3298

In my opinion, Pythonizing these legacy classes is a dead end and misleading, because it seems we are encouraging the use of these legacy containers, while we are actually discouraging that. To me, these Pythonizations are a candiadate for deprecation and removal themselves (at least by ROOT 7).

One usage is indirect. The T(Seq)Collection are used a 'lot' in the other interfaces.

Yes sure they are used in tons of interfaces. But I think it would be more Pythonic to Pythonize the constructor of these legacy collection so that they can be implicitly created from Python collections, instead of pythonizing the handling of the legacy collections themselves.

README/ReleaseNotes/v640/index.md

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

vepadulano · 2026-01-13T17:02:02Z

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py


    def test_getitem_slice(self):
        sc = self.create_tseqcollection()
+        sc.SetOwner(False)


So from the latest changes I guess that the default behaviour of TList is that it owns its elements, do you agree? I would have guessed the contrary judging by the TList destructor at

root/core/cont/src/TList.cxx

Lines 85 to 92 in e3fba69

////////////////////////////////////////////////////////////////////////////////

/// Delete the list. Objects are not deleted unless the TList is the

/// owner (set via SetOwner()).

TList::~TList()

{

Clear();

}

. Is there maybe some difference between the C++ implementation and the Pythonization?

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 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 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 root-project#4294, in particular it reverts 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 requested review from Axel-Naumann and vepadulano as code owners September 4, 2023 13:04

guitargeek self-assigned this Sep 4, 2023

guitargeek added the in:Python Interface label Sep 4, 2023

guitargeek mentioned this pull request Sep 4, 2023

[RF][PyROOT] Fix memory leaks with RooFit command arguments in PyROOT #13594

Closed

guitargeek changed the title ~~[PyROOT] Set memory policy to strict~~ [PyROOT] Set memory policy to "strict" Sep 4, 2023

guitargeek force-pushed the pyroot_memory_policy_1 branch from 7b2132c to a2fe3d5 Compare September 4, 2023 13:06

guitargeek added a commit to guitargeek/roottest that referenced this pull request Sep 4, 2023

[PyROOT] Adapt tests to the new default memory policy "strict"

a979fa2

Sister PR of root-project/root#13593.

guitargeek mentioned this pull request Sep 4, 2023

[PyROOT] Adapt tests to the new default memory policy "strict" root-project/roottest#1011

Closed

guitargeek force-pushed the pyroot_memory_policy_1 branch from a2fe3d5 to f3aa24f Compare September 4, 2023 16:26

root-project deleted a comment from phsft-bot Sep 4, 2023

guitargeek added a commit to guitargeek/roottest that referenced this pull request Sep 4, 2023

[PyROOT] Adapt tests to the new default memory policy "strict"

b75eb69

Sister PR of root-project/root#13593.

guitargeek force-pushed the pyroot_memory_policy_1 branch from f3aa24f to b147011 Compare September 4, 2023 22:48

guitargeek requested review from bellenot and couet as code owners September 4, 2023 22:48

guitargeek force-pushed the pyroot_memory_policy_1 branch 2 times, most recently from b7f0c8f to 4eadd14 Compare September 5, 2023 08:40

guitargeek force-pushed the pyroot_memory_policy_1 branch from 05f4dd0 to 262a8b4 Compare April 12, 2025 07:56

guitargeek changed the title ~~[PyROOT] Set memory policy to "strict"~~ [Python] Set memory policy to "strict" Apr 12, 2025

vepadulano reviewed Jan 7, 2026

View reviewed changes

guitargeek force-pushed the pyroot_memory_policy_1 branch from 262a8b4 to d0e71c7 Compare January 7, 2026 15:29

guitargeek force-pushed the pyroot_memory_policy_1 branch 13 times, most recently from 18dd852 to 7a08f72 Compare January 9, 2026 16:33

guitargeek mentioned this pull request Jan 9, 2026

[Python] Implement ruff linter suggestions #20837

Merged

guitargeek force-pushed the pyroot_memory_policy_1 branch 2 times, most recently from c176239 to 1fb8100 Compare January 10, 2026 08:39

vepadulano requested changes Jan 12, 2026

View reviewed changes

guitargeek force-pushed the pyroot_memory_policy_1 branch from 1fb8100 to ae08094 Compare January 13, 2026 12:11

guitargeek mentioned this pull request Jan 13, 2026

[hist] Delete TMultiGraph::Add(TMultiGraph*, Option_t*) overload #20869

Merged

guitargeek force-pushed the pyroot_memory_policy_1 branch from ae08094 to f6f069e Compare January 13, 2026 14:50

vepadulano reviewed Jan 13, 2026

View reviewed changes

guitargeek force-pushed the pyroot_memory_policy_1 branch from f6f069e to 4671b38 Compare January 14, 2026 12:50

guitargeek force-pushed the pyroot_memory_policy_1 branch from 4671b38 to 20765c1 Compare January 14, 2026 12:52

	////////////////////////////////////////////////////////////////////////////////
	/// Delete the list. Objects are not deleted unless the TList is the
	/// owner (set via SetOwner()).

	TList::~TList()
	{
	Clear();
	}

[Python] Set memory policy to "strict" #13593

Are you sure you want to change the base?

[Python] Set memory policy to "strict" #13593

Conversation

guitargeek commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Jan 7, 2026

Uh oh!

guitargeek commented Jan 9, 2026

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guitargeek commented Sep 4, 2023 •

edited

Loading