From b8d5795be69eb810193ee0afbb72a1934b87cd4b Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Mon, 22 Aug 2022 17:52:57 +0200 Subject: [PATCH 1/3] Introduce ROOT::ROwningPtr --- core/foundation/CMakeLists.txt | 1 + core/foundation/inc/ROOT/ROwningPtr.hxx | 43 +++++++++++++++++++++++++ io/io/inc/TFile.h | 7 ++-- io/io/src/TDirectoryFile.cxx | 2 +- io/io/src/TFile.cxx | 4 +-- 5 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 core/foundation/inc/ROOT/ROwningPtr.hxx 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..67f52534534db --- /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 ::value>> +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 \ No newline at end of file diff --git a/io/io/inc/TFile.h b/io/io/inc/TFile.h index f2fcc05a51ab8..e2d761b19f873 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..5fef4f844b4c9 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; From 43c8cafcf7e1dbb1a4fdc0382c9ebad381c1d03d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 23 Aug 2022 15:19:54 +0200 Subject: [PATCH 2/3] ROwningPtr no pointer template type --- core/foundation/inc/ROOT/ROwningPtr.hxx | 10 +++++----- io/io/inc/TFile.h | 6 +++--- io/io/src/TFile.cxx | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core/foundation/inc/ROOT/ROwningPtr.hxx b/core/foundation/inc/ROOT/ROwningPtr.hxx index 67f52534534db..b1eaa3694e579 100644 --- a/core/foundation/inc/ROOT/ROwningPtr.hxx +++ b/core/foundation/inc/ROOT/ROwningPtr.hxx @@ -20,14 +20,14 @@ #include namespace ROOT { -template ::value>> +template class ROwningPtr { public: - using pointer = T; + using pointer = T *; using element_type = typename std::remove_pointer::type; using reference = typename std::add_lvalue_reference::type; - ROwningPtr(T ptr) : _ptr{ptr} {} + ROwningPtr(T *ptr) : _ptr{ptr} {} operator pointer() { return _ptr; } operator std::unique_ptr() { return std::unique_ptr{_ptr}; } @@ -36,8 +36,8 @@ public: pointer operator->() const { return _ptr; } private: - T _ptr; + T *_ptr; }; } // namespace ROOT -#endif \ No newline at end of file +#endif diff --git a/io/io/inc/TFile.h b/io/io/inc/TFile.h index e2d761b19f873..76b680690fddc 100644 --- a/io/io/inc/TFile.h +++ b/io/io/inc/TFile.h @@ -297,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 ROOT::ROwningPtr 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/TFile.cxx b/io/io/src/TFile.cxx index 5fef4f844b4c9..3ca6560017846 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -4068,7 +4068,7 @@ 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. -ROOT::ROwningPtr +ROOT::ROwningPtr TFile::Open(const char *url, Option_t *options, const char *ftitle, Int_t compress, Int_t netopt) { TPluginHandler *h; From 06f9f9e6c90da9761c624d5ef7a63b462349d598 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 5 Dec 2021 18:02:24 +0100 Subject: [PATCH 3/3] [RF] New `RooFit::Owner` pointer wrapper to flag owning return values 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. --- .../pyroot/cppyy/CPyCppyy/src/CPPOverload.cxx | 36 +++++++ bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h | 2 + .../clingwrapper/src/clingwrapper.cxx | 17 +++- .../clingwrapper/src/cpp_cppyy.h | 2 + .../pyroot/pythonizations/test/CMakeLists.txt | 3 + .../pyroot/pythonizations/test/rowningptr.py | 99 +++++++++++++++++++ roofit/CMakeLists.txt | 2 +- 7 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 bindings/pyroot/pythonizations/test/rowningptr.py 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/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)