root icon indicating copy to clipboard operation
root copied to clipboard

[metacling] Destroy and release llvm::ManagedStatics

Open aaronj0 opened this issue 6 months ago • 5 comments

Fixes #18933

Following up on this issue, CppInterOp now uses ManagedStatic to support a stack of multiple interpreters. Although we do not use this feature (yet?), this shutdown call is now required in TCling.

aaronj0 avatar Jun 20 '25 08:06 aaronj0

Test Results

    20 files      20 suites   3d 19h 30m 0s ⏱️  3 016 tests  3 016 ✅ 0 💤 0 ❌ 58 726 runs  58 726 ✅ 0 💤 0 ❌

Results for commit 88740e4f.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jun 20 '25 16:06 github-actions[bot]

Can you check if the clad timing system works through setting the env variable?

vgvassilev avatar Jun 20 '25 19:06 vgvassilev

It doesn't work yet. Here is how you can test:

git a/interpreter/cling/tools/plugins/clad/CMakeLists.txt b/interpreter/cling/tools/plugins/clad/CMakeLists.txt
index c0316e679e0..bbebc2ab69a 100644
--- a/interpreter/cling/tools/plugins/clad/CMakeLists.txt
+++ b/interpreter/cling/tools/plugins/clad/CMakeLists.txt
@@ -74,7 +74,8 @@ if (DEFINED CLAD_SOURCE_DIR)
   list(APPEND _clad_extra_settings SOURCE_DIR ${CLAD_SOURCE_DIR})
 else()
   list(APPEND _clad_extra_settings GIT_REPOSITORY https://github.com/vgvassilev/clad.git)
-  list(APPEND _clad_extra_settings GIT_TAG v1.10)
+  list(APPEND _clad_extra_settings GIT_TAG master)
 endif()

 #list(APPEND _clad_patches_list "patch1.patch" "patch2.patch")
diff --git a/tutorials/roofit/roofit/rf101_basics.C b/tutorials/roofit/roofit/rf101_basics.C
index 969fed87b0c..dfb0be244fd 100644
--- a/tutorials/roofit/roofit/rf101_basics.C
+++ b/tutorials/roofit/roofit/rf101_basics.C
@@ -64,7 +64,7 @@ void rf101_basics()
    // -----------------------------

    // Fit pdf to data
-   gauss.fitTo(*data, PrintLevel(-1));
+   gauss.fitTo(*data, PrintLevel(-1), EvalBackend("codegen"));

    // Print values of mean and sigma (that now reflect fitted values and errors)
    mean.Print();

Then run inside tutorials/roofit/roofit:

CLAD_ENABLE_TIMING=1 EXTRA_CLING_ARGS="-Xclang -print-stats -ftime-report" root -b -q rf101_basics.C

If you were to change the managed static to a regular static in clad, it will work:

diff --git a/lib/Differentiator/Timers.cpp b/lib/Differentiator/Timers.cpp
index 67f3ec5..74de19d 100644
--- a/lib/Differentiator/Timers.cpp
+++ b/lib/Differentiator/Timers.cpp
@@ -100,8 +100,8 @@ public:
 CladTimeInfo* CladTimeInfo::TheTimingInfo;
 void InitTimers() {
   assert(!CladTimeInfo::TheTimingInfo);
-  static llvm::ManagedStatic<CladTimeInfo> CTI;
-  CladTimeInfo::TheTimingInfo = &*CTI;
+  static CladTimeInfo CTI;
+  CladTimeInfo::TheTimingInfo = &CTI;
 }

 TimedAnalysisRegion::TimedAnalysisRegion(llvm::StringRef Name) {

guitargeek avatar Jun 30 '25 14:06 guitargeek

Can we decide what to do about this item?

dpiparo avatar Nov 29 '25 06:11 dpiparo

We need to understand why the llvm::ManagedStatics are not destroyed.

vgvassilev avatar Nov 29 '25 06:11 vgvassilev