From 2b302b1060a402f340092535ea996afafde2493c Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 22 May 2024 18:07:44 +0200 Subject: [PATCH] [PyROOT] Fix memory leak in TTree `__getattr__` pythonization As reported on the forum: https://root-forum.cern.ch/t/memory-leak-in-pyroot-when-using-user-defined-c-macros-from-python-and-ttree-friends/59432 I fixed the memory leak in the Pythonization is in the usual way how I fix problems with the PyROOT CPython extension: re-implementing the offending parts in C++ and hoping that the problem is gone. Which it is! --- .../python/ROOT/_pythonization/_ttree.py | 13 +++- .../pythonizations/src/PyROOTModule.cxx | 2 +- .../pythonizations/src/PyROOTPythonize.h | 2 +- .../pyroot/pythonizations/src/TTreePyz.cxx | 62 ++++++++++--------- 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_ttree.py b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_ttree.py index 35d71b251bf4a..e77c31d721079 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_ttree.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_ttree.py @@ -130,7 +130,7 @@ */ ''' -from libROOTPythonizations import AddBranchAttrSyntax, SetBranchAddressPyz, BranchPyz +from libROOTPythonizations import GetBranchAttr, SetBranchAddressPyz, BranchPyz from . import pythonization # TTree iterator @@ -168,6 +168,15 @@ def _Branch(self, *args): return res +def _TTree__getattr__(self, key): + + import cppyy.ll + + out, cast_type = GetBranchAttr(self, key) + if cast_type: + out = cppyy.ll.cast[cast_type](out) + return out + @pythonization('TTree') def pythonize_ttree(klass, name): # Parameters: @@ -183,7 +192,7 @@ def pythonize_ttree(klass, name): klass.__iter__ = _TTree__iter__ # tree.branch syntax - AddBranchAttrSyntax(klass) + klass.__getattr__ = _TTree__getattr__ # SetBranchAddress klass._OriginalSetBranchAddress = klass.SetBranchAddress diff --git a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx index 7d9943618130f..9eee02d647c1c 100644 --- a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx +++ b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx @@ -42,7 +42,7 @@ PyObject *gRootModule = 0; static PyMethodDef gPyROOTMethods[] = { {(char *)"AddCPPInstancePickling", (PyCFunction)PyROOT::AddCPPInstancePickling, METH_VARARGS, (char *)"Add a custom pickling mechanism for Cppyy Python proxy objects"}, - {(char *)"AddBranchAttrSyntax", (PyCFunction)PyROOT::AddBranchAttrSyntax, METH_VARARGS, + {(char *)"GetBranchAttr", (PyCFunction)PyROOT::GetBranchAttr, METH_VARARGS, (char *)"Allow to access branches as tree attributes"}, {(char *)"AddTClassDynamicCastPyz", (PyCFunction)PyROOT::AddTClassDynamicCastPyz, METH_VARARGS, (char *)"Cast the void* returned by TClass::DynamicCast to the right type"}, diff --git a/bindings/pyroot/pythonizations/src/PyROOTPythonize.h b/bindings/pyroot/pythonizations/src/PyROOTPythonize.h index 7f2a30edb751e..9f2d4506b6d9d 100644 --- a/bindings/pyroot/pythonizations/src/PyROOTPythonize.h +++ b/bindings/pyroot/pythonizations/src/PyROOTPythonize.h @@ -20,7 +20,7 @@ PyObject *AddCPPInstancePickling(PyObject *self, PyObject *args); PyObject *AddPrettyPrintingPyz(PyObject *self, PyObject *args); -PyObject *AddBranchAttrSyntax(PyObject *self, PyObject *args); +PyObject *GetBranchAttr(PyObject *self, PyObject *args); PyObject *BranchPyz(PyObject *self, PyObject *args); PyObject *SetBranchAddressPyz(PyObject *self, PyObject *args); diff --git a/bindings/pyroot/pythonizations/src/TTreePyz.cxx b/bindings/pyroot/pythonizations/src/TTreePyz.cxx index 02a37486c4247..e952514d78c21 100644 --- a/bindings/pyroot/pythonizations/src/TTreePyz.cxx +++ b/bindings/pyroot/pythonizations/src/TTreePyz.cxx @@ -75,14 +75,14 @@ static TLeaf *SearchForLeaf(TTree *tree, const char *name, TBranch *branch) return leaf; } -static PyObject *BindBranchToProxy(TTree *tree, const char *name, TBranch *branch) +static std::pair ResolveBranch(TTree *tree, const char *name, TBranch *branch) { // for partial return of a split object if (branch->InheritsFrom(TBranchElement::Class())) { TBranchElement *be = (TBranchElement *)branch; if (be->GetCurrentClass() && (be->GetCurrentClass() != be->GetTargetClass()) && (0 <= be->GetID())) { Long_t offset = ((TStreamerElement *)be->GetInfo()->GetElements()->At(be->GetID()))->GetOffset(); - return CPyCppyy::Instance_FromVoidPtr(be->GetObject() + offset, be->GetCurrentClass()->GetName()); + return {be->GetObject() + offset, be->GetCurrentClass()->GetName()}; } } @@ -90,15 +90,15 @@ static PyObject *BindBranchToProxy(TTree *tree, const char *name, TBranch *branc if (branch->IsA() == TBranchElement::Class() || branch->IsA() == TBranchObject::Class()) { TClass *klass = TClass::GetClass(branch->GetClassName()); if (klass && branch->GetAddress()) - return CPyCppyy::Instance_FromVoidPtr(*(void **)branch->GetAddress(), branch->GetClassName()); + return {*(void **)branch->GetAddress(), branch->GetClassName()}; // try leaf, otherwise indicate failure by returning a typed null-object TObjArray *leaves = branch->GetListOfLeaves(); if (klass && !tree->GetLeaf(name) && !(leaves->GetSize() && (leaves->First() == leaves->Last()))) - return CPyCppyy::Instance_FromVoidPtr(nullptr, branch->GetClassName()); + return {nullptr, branch->GetClassName()}; } - return nullptr; + return {nullptr, ""}; } static PyObject *WrapLeaf(TLeaf *leaf) @@ -142,8 +142,18 @@ static PyObject *WrapLeaf(TLeaf *leaf) } // Allow access to branches/leaves as if they were data members -PyObject *GetAttr(PyObject *self, PyObject *pyname) +// Returns a Python tuple where the first element is either the desired +// CPyCppyy proxy, or an address that still needs to be wrapped by the caller +// in a proxy using cppyy.ll.cast. In the latter laster, the second tuple +// element is the target type name. Otherwise, the second element is an empty +// string. +PyObject *PyROOT::GetBranchAttr(PyObject * /*self*/, PyObject *args) { + PyObject *self = nullptr; + PyObject *pyname = nullptr; + + PyArg_ParseTuple(args, "OU:GetBranchAttr", &self, &pyname); + const char *name_possibly_alias = PyUnicode_AsUTF8(pyname); if (!name_possibly_alias) return 0; @@ -166,19 +176,28 @@ PyObject *GetAttr(PyObject *self, PyObject *pyname) if (branch) { // found a branched object, wrap its address for the object it represents - auto proxy = BindBranchToProxy(tree, name, branch); - if (proxy != nullptr) - return proxy; + void * finalAddress = nullptr; + std::string finalType; + std::tie(finalAddress, finalType) = ResolveBranch(tree, name, branch); + if (!finalType.empty()) { + PyObject *outTuple = PyTuple_New(2); + //PyObject *proxy = CPyCppyy::Instance_FromVoidPtr(finalAddress, finalType); + PyTuple_SET_ITEM(outTuple, 0, PyLong_FromLongLong((intptr_t)finalAddress)); // does this work on 32 bit?? + PyTuple_SET_ITEM(outTuple, 1, CPyCppyy_PyText_FromString((finalType + "*").c_str())); + return outTuple; + } } // if not, try leaf - TLeaf *leaf = SearchForLeaf(tree, name, branch); - - if (leaf) { + if (TLeaf *leaf = SearchForLeaf(tree, name, branch)) { // found a leaf, extract value and wrap with a Python object according to its type auto wrapper = WrapLeaf(leaf); - if (wrapper != nullptr) - return wrapper; + if (wrapper != nullptr) { + PyObject *outTuple = PyTuple_New(2); + PyTuple_SET_ITEM(outTuple, 0, wrapper); + PyTuple_SET_ITEM(outTuple, 1, CPyCppyy_PyText_FromString("")); + return outTuple; + } } // confused @@ -186,21 +205,6 @@ PyObject *GetAttr(PyObject *self, PyObject *pyname) return 0; } -//////////////////////////////////////////////////////////////////////////// -/// \brief Allow branches to be accessed as attributes of a tree. -/// \param[in] self Always null, since this is a module function. -/// \param[in] args Pointer to a Python tuple object containing the arguments -/// received from Python. -/// -/// Allow access to branches/leaves as if they were Python data attributes of the tree -/// (e.g. mytree.branch) -PyObject *PyROOT::AddBranchAttrSyntax(PyObject * /* self */, PyObject *args) -{ - PyObject *pyclass = PyTuple_GetItem(args, 0); - Utility::AddToClass(pyclass, "__getattr__", (PyCFunction)GetAttr, METH_O); - Py_RETURN_NONE; -} - //////////////////////////////////////////////////////////////////////////// /// \brief Add pythonization for TTree::SetBranchAddress. /// \param[in] self Always null, since this is a module function.