[RF] New RooFit::Owner pointer wrapper to flag owning return values
#9392
Conversation
roofit/roofitcore/CMakeLists.txt
Outdated
| @@ -230,6 +230,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitCore | |||
| RooNaNPacker.h | |||
| RooBinSamplingPdf.h | |||
| RooBinWidthFunction.h | |||
| RooFit/Owner.h | |||
There was a problem hiding this comment.
Is this a conscious decision to put this in RooFit? You're adding one more directory that has to be managed.
There was a problem hiding this comment.
Yes, that was a deliberate choice. We have too many headers now with terribly generic names like Floats.h, UniqueId.h, and not possible Owner.h.
Many distributors install the ROOT headers right into /usr/include, so it's bad to have these header file names I'd say. Fortunately it's not too late to do something about it, so I wanted to establish a convention to avoid name collisions:
- Installed RooFit headers must start with
Roo*or must be located in a subdirectory starting withRoo*(e.g. RooFit or RooStats). - Similarly, if the class name doesn't start with Roo, it has to go in a
Roo*namespace (usuallyRooFit) - Free functions always need to go in this namespace
- For implmentation details that we can't avoid installing, we can use a
Roo*::Detailnamespace like we have withROOT::Detail(same withExperimental)
I'd still have time to move Floats.h and and UniqueId.h intoRooFit.
What are your thoughts on this?
464cba8 to
8f8b162
Compare
Yes that works! You can also do these checks here: RooFit::Owner<int*>(nullptr) == nullptr
RooFit::Owner<int*>(new int) == nullptrAlso maybe a question also to @etejedor and @Axel-Naumann: you think this is general enough to have the |
| // owner pointer. | ||
| if (cppInstance->IsSmart()) { | ||
| if(Cppyy::IsOwnerPtr(cppInstance->GetSmartIsA())) { | ||
| // Here we create a new Python object with the same flags that |
There was a problem hiding this comment.
I was wondering why you need to create this Python object yourself and configure that Python owns the C++ object.
With your new mechanism, if instead of returning a RooDataSet* (whose Python proxy would not own) now you return a RooFit::Owner<RooDataSet*>, doesn't cppyy already return a Python proxy that owns what it has in its belly and therefore would delete the C++ object when no more references are left for the Python proxy?
There was a problem hiding this comment.
Hi @etejedor, thanks for your comment! I don't quite understand your question, I'm not so fluent with the cppyy terminology...
If I understand correctly we have separate objects:
- The Python Proxy of type
PyObject * - The C++ proxy that is owned by the Python proxy which points to the actual object, can be of type raw pointer
RooDataSet *or any smart pointer known to cppyy (e.g.unique_ptr<RooDataSet>or nowRooFit::Owner<RooDataSet>) - The actual C++ object owned by the C++ proxy, or by Python if
kIsOwneris true
What I want to achieve here is to
- Tell the
PyObjectthat it owns the C++ object, not only the C++ proxy - Replace the
RooFit::Owner<RooDataSet>proxy with a raw pointer proxyRooDataSet *such that there is no confusingowned by RooFit::Owner<RooDataSet>in the message when youprintthe Python object. But this is not the main point, only cosmetics.
If I understand your question...
doesn't cppyy already return a Python proxy that owns what it has in its belly and therefore would delete the C++ object when no more references are left for the Python proxy?
...correctly, you are wondering why the C++ object doesn't get deleted when I delete the original PyObject, right? Well, only the C++ proxy object (in this case the RooFit::Owner<RooDataSet*>) gets deleted. But the RooFit::Owner doesn't actually own, it only indicates ownership by the caller. That's why the C++ object survives.
There was a problem hiding this comment.
Yes that clarifies my doubt thanks!
8f8b162 to
0d05d7a
Compare
1802287 to
c3fb4b7
Compare
| @@ -144,6 +145,41 @@ static inline PyObject* HandleReturn( | |||
| int ll_action = 0; | |||
| if (result) { | |||
|
|
|||
| // If the "smart pointer" is only an observing pointer that indicated | |||
There was a problem hiding this comment.
Can you add to the commit a patch file that contains the changes done to cppyy? Especifically here: https://github.com/root-project/root/tree/master/bindings/pyroot/cppyy/patches
| @@ -128,7 +128,13 @@ static std::set<std::string> g_builtins = | |||
| // smart pointer types | |||
| static std::set<std::string> gSmartPtrTypes = | |||
| {"auto_ptr", "std::auto_ptr", "shared_ptr", "std::shared_ptr", | |||
| "unique_ptr", "std::unique_ptr", "weak_ptr", "std::weak_ptr"}; | |||
| "unique_ptr", "std::unique_ptr", "weak_ptr", "std::weak_ptr", | |||
| "RooFit::Owner"}; | |||
There was a problem hiding this comment.
Do you strictly need that RooFit::Owner is treated as a smart ptr too?
967d4a6 to
f3d15a3
Compare
f783af0 to
8bdbd77
Compare
In RooFit, there are many functions that return pointers that are owned
by the caller. We can't change this interface anymore, but we can wrap
the return values transparently in a raw pointer wrapper that is called
a `RooFit::Owner`.
On the C++ side, this helps to analyze your code and detect potential
memory leaks. On the Python side, we can tell cppyy to take ownership
of the object if the pointer is wrapped in a owning pointer such as the
`RooFit::Owner`. This is more flexible and convenient than the existing
cppyy way of flagging the CPPOverloads on the Python side with the
`__creates__ = True` attribute for at least two reasons:
1. This flag can't be applied at the granularity of indivirual C++
overloads
2. It's only on the Python side, so if you want to flag these functions
in C++ as well as in Python you have to do some bookkeeping
A unit test was implemented to check that the `RooFit::Owner` behaves in
Python as expected, and that there is no memory leaking when using
functions that return them.
As a first example, the `RooFit::Owner` is used in the highly used
function `RooAbsPdf::generate`, so we also get quite some test coverage
from the tutorials.
In the future after this initial effort, the remaining RooFit functions
should be migrated to fix many memory leaks in PyROOT user code.
8bdbd77 to
06f9f9e
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Build failed on windows10/default. Errors:
And 1 more |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Failing tests: |
|
Build failed on mac12arm/cxx20. Failing tests: |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Closing this PR, because this is probably not the way to solve the ownership problem. It would be better to have another approach here, with attributes on the C++ side that cppyy can understand. Either way, if we would go with a special owning pointer class, the logic suggested in this PR can better be implemented with a custom CPyCppyy executor that is declared via the public CPyCppyy API. |
In RooFit, there are many functions that return pointers that are owned
by the caller. We can't change this interface anymore, but we can wrap
the return values transparently in a raw pointer wrapper that is called
a
RooFit::Owner.On the C++ side, this helps to analyze your code and detect potential
memory leaks. On the Python side, we can tell cppyy to take ownership
of the object if the pointer is wrapped in a owning pointer such as the
RooFit::Owner. This is more flexible and convenient than the existingcppyy way of flagging the CPPOverloads on the Python side with the
__creates__ = Trueattribute for at least two reasons:overloads
in C++ as well as in Python you have to do some bookkeeping
A unit test was implemented to check that the
RooFit::Ownerbehaves inPython as expected, and that there is no memory leaking when using
functions that return them.
As a first example, the
RooFit::Owneris used in the highly usedfunction
RooAbsPdf::generate, so we also get quite some test coveragefrom the tutorials.
In the future after this initial effort, the remaining RooFit functions
should be migrated to fix many memory leaks in PyROOT user code.