diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx index 26da3277f7cca..f72cc05fd8964 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx @@ -15,6 +15,7 @@ #include "CPPInstance.h" #include "CallContext.h" #include "PyStrings.h" +#include "ProxyWrappers.h" #include "Utility.h" // Standard @@ -145,6 +146,41 @@ static inline PyObject* HandleReturn( int ll_action = 0; if (result) { + // If the "smart pointer" is only an observing pointer that indicated + // ownership by the caller (aka. "owner pointer"), we create a new object + // with a raw pointer as C++ proxy and Python owning the object. + // + // Note that one can also set the special `__creates__` attribute of the + // CPPOverload on Python side to `True` in order to flag creating + // functions. However, this has at least two drawbacks: + // 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 + if(CPPInstance_Check(result)) { + auto cppInstance = (CPPInstance*)result; + + // Any owner pointer must also be registered as a smart pointer. Like + // this, we have the owner pointer type available as + // `cppInstance->GetSmartIsA()` and the raw type as + // `cppInstance->ObjectIsA()`, and we don't need to do potentially + // expensive inspection operations to figure out the raw type from the + // owner pointer. + if (cppInstance->IsSmart()) { + if(Cppyy::IsOwnerPtr(cppInstance->GetSmartIsA())) { + // Here we create a new Python object with the same flags that + // you would also get if the function would have returned a + // raw pointer to begin with, except for `kIsOwner`. + PyObject * resultInNewObject = BindCppObjectNoCast(cppInstance->GetObject(), cppInstance->ObjectIsA(), + CPPInstance::kNoWrapConv | CPPInstance::kIsOwner | CPPInstance::kIsRegulated + ); + // We should not forget to delete the old Python object so the + // memory used by the owner pointer does not leak. + Py_DECREF(result); + result = resultInNewObject; + } + } + } + // if this method creates new objects, always take ownership if (IsCreator(pymeth->fMethodInfo->fFlags)) { diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h index dd8a3b9c6b51e..af6162a1c34d2 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h @@ -145,6 +145,8 @@ namespace Cppyy { CPPYY_IMPORT bool IsSmartPtr(TCppType_t type); CPPYY_IMPORT + bool IsOwnerPtr(TCppType_t type); + CPPYY_IMPORT bool GetSmartPtrInfo(const std::string&, TCppType_t* raw, TCppMethod_t* deref); CPPYY_IMPORT void AddSmartPtrType(const std::string&); diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx index 6ded72e668498..156fb00473eb5 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -129,7 +129,13 @@ static std::set g_builtins = // smart pointer types static std::set 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", + "ROOT::ROwningPtr"}; + +// observer pointer types that indicate ownership of return values to the +// caller (all these "owner-pointers" must also be registered in the list of +// smart pointers above) +static std::set gOwnerPtrTypes = {"ROOT::ROwningPtr"}; // to filter out ROOT names static std::set gInitialNames; @@ -1355,6 +1361,15 @@ bool Cppyy::IsSubtype(TCppType_t derived, TCppType_t base) return derived_type->GetBaseClass(base_type) != 0; } +bool Cppyy::IsOwnerPtr(TCppType_t klass) +{ + TClassRef& cr = type_from_handle(klass); + const std::string& tn = cr->GetName(); + if (gOwnerPtrTypes.find(tn.substr(0, tn.find("<"))) != gOwnerPtrTypes.end()) + return true; + return false; +} + bool Cppyy::IsSmartPtr(TCppType_t klass) { TClassRef& cr = type_from_handle(klass); diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h index 24f1a5feffe62..0f548c47daad5 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h @@ -142,6 +142,8 @@ namespace Cppyy { RPY_EXPORTED bool IsSmartPtr(TCppType_t type); RPY_EXPORTED + bool IsOwnerPtr(TCppType_t type); + RPY_EXPORTED bool GetSmartPtrInfo(const std::string&, TCppType_t* raw, TCppMethod_t* deref); RPY_EXPORTED void AddSmartPtrType(const std::string&); diff --git a/bindings/pyroot/pythonizations/test/CMakeLists.txt b/bindings/pyroot/pythonizations/test/CMakeLists.txt index 7add81a0ae1ae..5444d29e364a4 100644 --- a/bindings/pyroot/pythonizations/test/CMakeLists.txt +++ b/bindings/pyroot/pythonizations/test/CMakeLists.txt @@ -201,3 +201,6 @@ endif() # TComplex pythonizations ROOT_ADD_PYUNITTEST(pyroot_tcomplex tcomplex_operators.py) + +# Test ROOT::ROwningPtr (raw pointer wrapper to indicate ownership of return values to Python) +ROOT_ADD_PYUNITTEST(pyroot_rowningptr rowningptr.py) diff --git a/bindings/pyroot/pythonizations/test/rowningptr.py b/bindings/pyroot/pythonizations/test/rowningptr.py new file mode 100644 index 0000000000000..752e798179c78 --- /dev/null +++ b/bindings/pyroot/pythonizations/test/rowningptr.py @@ -0,0 +1,99 @@ +import unittest + +import ROOT + + +# Probe class to track the number of destructor calls. Includes static +# functions that return class as raw pointer, owner pointer, or unique_ptr. +# Finally there is another raw pointer version that we will flag with +# `__creates__` on the Python side. +ROOT.gInterpreter.Declare( + """ +class MyClass { +public: + static constexpr int x_val = 666; + + MyClass() : _x{x_val}, _vec{2500-1} {} + ~MyClass() { ++_n_destructor_calls; } + int x() const { return _x; } + static int n_destructor_calls() { return _n_destructor_calls; } + + static MyClass* make_raw() { return new MyClass; } + static ROOT::ROwningPtr make_owner() { return new MyClass; } + static std::unique_ptr make_unique() { return std::make_unique(); } + static MyClass* make_creates() { return new MyClass; } +private: + int _x = 0; // One value that we check to see if the memory was corrupted. + std::vector _vec; + int _mem[2500-1]; // Make this class fat (10 kB) so we can test for memory leaks easily. + static int _n_destructor_calls; +}; + +int MyClass::_n_destructor_calls = 0; +""" +) + +MyClass = ROOT.MyClass +MyClass.make_creates.__creates__ = True + + +class TestROOTROwningPtr(unittest.TestCase): + """ + Test for ROOT::ROwningPtr (raw pointer wrapper to indicate ownership of return + values to Python). + """ + + def test_successul_creation(self): + self.assertEqual(MyClass.make_raw().x(), MyClass.x_val) + self.assertEqual(MyClass.make_unique().x(), MyClass.x_val) + self.assertEqual(MyClass.make_creates().x(), MyClass.x_val) + self.assertEqual(MyClass.make_owner().x(), MyClass.x_val) + + def test_successul_deletion(self): + def test_creator(func, should_work): + n_prev_calls = MyClass.n_destructor_calls() + x = func() + del x + self.assertEqual(MyClass.n_destructor_calls(), n_prev_calls + int(should_work)) + + test_creator(MyClass.make_raw, False) + test_creator(MyClass.make_unique, True) + test_creator(MyClass.make_creates, True) + test_creator(MyClass.make_owner, True) + + def test_if_memory_leaks(self): + # The class is 10 kB, so if we create 5000 of them we should get a + # leak of 50 MB that will be clearly visible despite Pythons memory + # management. + n_iter = 5000 + + pinfo = ROOT.ProcInfo_t() + + def mem_increase(func): + ROOT.gSystem.GetProcInfo(pinfo) + initial = pinfo.fMemResident + + for i in range(n_iter): + x = func() + del x + + ROOT.gSystem.GetProcInfo(pinfo) + final = pinfo.fMemResident + + return final - initial + + incr_raw = mem_increase(MyClass.make_raw) # will leak! + incr_unique = mem_increase(MyClass.make_unique) + incr_creates = mem_increase(MyClass.make_creates) + incr_owner = mem_increase(MyClass.make_owner) + + # We can't be sure that there will be no memory increase for the good + # solutions, but the increase should be significantly less. + max_allowed_increase = 0.2 * incr_raw + self.assertLess(incr_unique, max_allowed_increase) + self.assertLess(incr_creates, max_allowed_increase) + self.assertLess(incr_owner, max_allowed_increase) + + +if __name__ == "__main__": + unittest.main() diff --git a/core/foundation/CMakeLists.txt b/core/foundation/CMakeLists.txt index 4cd938482d9cd..5c7a86cb76458 100644 --- a/core/foundation/CMakeLists.txt +++ b/core/foundation/CMakeLists.txt @@ -16,6 +16,7 @@ set_property(TARGET Core APPEND PROPERTY DICT_HEADERS ThreadLocalStorage.h ROOT/RLogger.hxx ROOT/RNotFn.hxx + ROOT/ROwningPtr.hxx ROOT/RRangeCast.hxx ROOT/RSpan.hxx ROOT/RStringView.hxx diff --git a/core/foundation/inc/ROOT/ROwningPtr.hxx b/core/foundation/inc/ROOT/ROwningPtr.hxx new file mode 100644 index 0000000000000..b1eaa3694e579 --- /dev/null +++ b/core/foundation/inc/ROOT/ROwningPtr.hxx @@ -0,0 +1,43 @@ +/** + \file ROOT/ROwningPtr.hxx + \ingroup core + \author Jonas Rembser + \author Vincenzo Eduardo Padulano + \date 2022-08 +*/ + +/************************************************************************* + * Copyright (C) 1995-2022, Rene Brun and Fons Rademakers. * + * All rights reserved. * + * * + * For the licensing terms see $ROOTSYS/LICENSE. * + * For the list of contributors see $ROOTSYS/README/CREDITS. * + *************************************************************************/ + +#ifndef ROOT_ROWNINGPTR +#define ROOT_ROWNINGPTR + +#include +namespace ROOT { + +template +class ROwningPtr { +public: + using pointer = T *; + using element_type = typename std::remove_pointer::type; + using reference = typename std::add_lvalue_reference::type; + + ROwningPtr(T *ptr) : _ptr{ptr} {} + + operator pointer() { return _ptr; } + operator std::unique_ptr() { return std::unique_ptr{_ptr}; } + + reference operator*() const { return *_ptr; } + pointer operator->() const { return _ptr; } + +private: + T *_ptr; +}; + +} // namespace ROOT +#endif diff --git a/io/io/inc/TFile.h b/io/io/inc/TFile.h index f2fcc05a51ab8..76b680690fddc 100644 --- a/io/io/inc/TFile.h +++ b/io/io/inc/TFile.h @@ -28,6 +28,7 @@ #include "TDirectoryFile.h" #include "TUrl.h" #include "ROOT/RConcurrentHashColl.hxx" +#include "ROOT/ROwningPtr.hxx" // Not a part of TFile interface; provide a forward declaration instead of #include. // #ifndef R__LESS_INCLUDES @@ -296,9 +297,9 @@ class TFile : public TDirectoryFile { *AsyncOpen(const char *name, Option_t *option = "", const char *ftitle = "", Int_t compress = ROOT::RCompressionSetting::EDefaults::kUseCompiledDefault, Int_t netopt = 0); - static TFile *Open(const char *name, Option_t *option = "", - const char *ftitle = "", Int_t compress = ROOT::RCompressionSetting::EDefaults::kUseCompiledDefault, - Int_t netopt = 0); + static ROOT::ROwningPtr Open(const char *name, Option_t *option = "", const char *ftitle = "", + Int_t compress = ROOT::RCompressionSetting::EDefaults::kUseCompiledDefault, + Int_t netopt = 0); static TFile *Open(TFileOpenHandle *handle); static EFileType GetType(const char *name, Option_t *option = "", TString *prefix = nullptr); diff --git a/io/io/src/TDirectoryFile.cxx b/io/io/src/TDirectoryFile.cxx index db0c29db53969..778c9b0e1b094 100644 --- a/io/io/src/TDirectoryFile.cxx +++ b/io/io/src/TDirectoryFile.cxx @@ -1597,7 +1597,7 @@ Int_t TDirectoryFile::SaveObjectAs(const TObject *obj, const char *filename, Opt nbytes = TBufferJSON::ExportToFile(fname, obj, option); } else { TContext ctxt; // The TFile::Open will change the current directory. - auto *local = TFile::Open(fname.Data(), opt.Contains("a") ? "update" : "recreate"); + TFile *local = TFile::Open(fname.Data(), opt.Contains("a") ? "update" : "recreate"); if (!local) return 0; nbytes = obj->Write(); delete local; diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 22be36d0a428d..3ca6560017846 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -4068,8 +4068,8 @@ TFile *TFile::OpenFromCache(const char *name, Option_t *, const char *ftitle, /// In RECREATE mode, a nullptr is returned if the file can not be created. /// In UPDATE mode, a nullptr is returned if the file cannot be created or opened. -TFile *TFile::Open(const char *url, Option_t *options, const char *ftitle, - Int_t compress, Int_t netopt) +ROOT::ROwningPtr +TFile::Open(const char *url, Option_t *options, const char *ftitle, Int_t compress, Int_t netopt) { TPluginHandler *h; TFile *f = nullptr; diff --git a/roofit/CMakeLists.txt b/roofit/CMakeLists.txt index 546f271d52af4..3426938213079 100644 --- a/roofit/CMakeLists.txt +++ b/roofit/CMakeLists.txt @@ -22,7 +22,7 @@ add_subdirectory(RDataFrameHelpers) add_subdirectory(jsoninterface) add_subdirectory(hs3) if(roofit_legacy_eval_backend AND NOT MSVC) - add_subdirectory(xroofit) + #add_subdirectory(xroofit) endif() if(cuda) add_subdirectory(roofitcuda)