From a6ca1c3b75c9ef42d65b713036a92a849e967ac2 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 1/6] [core] Reduce symbol search only to when autoloading is enabled. The llvm9 JIT issued callbacks when a symbol was missing and we reacted on it by loading the relevant library. In root-project/root@9b2041e3 we have kept the logic but now the JIT started querying more often even for symbols which are okay to be missing. In turn that leads to scanning all libraries causing performance issues. This patch tries to limit this functionality only in contexts where automatic loading is allowed. --- core/metacling/src/TCling.cxx | 5 +-- core/metacling/src/TClingCallbacks.cxx | 30 ++++++++++------- core/metacling/src/TClingCallbacks.h | 2 +- .../test/TClingDataMemberInfoTests.cxx | 32 +++++++++++++++++++ 4 files changed, 55 insertions(+), 14 deletions(-) diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx index baba126b70567..d44c317e1f4f7 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -4717,6 +4717,9 @@ TInterpreter::DeclId_t TCling::GetDataMember(ClassInfo_t *opaque_cl, const char DeclId_t d; TClingClassInfo *cl = (TClingClassInfo*)opaque_cl; + // Could trigger deserialization of decls. + cling::Interpreter::PushTransactionRAII RAII(GetInterpreterImpl()); + if (cl) { d = cl->GetDataMember(name); // We check if the decl of the data member has an annotation which indicates @@ -4747,8 +4750,6 @@ TInterpreter::DeclId_t TCling::GetDataMember(ClassInfo_t *opaque_cl, const char LookupResult R(SemaR, DName, SourceLocation(), Sema::LookupOrdinaryName, Sema::ForExternalRedeclaration); - // Could trigger deserialization of decls. - cling::Interpreter::PushTransactionRAII RAII(GetInterpreterImpl()); cling::utils::Lookup::Named(&SemaR, R); LookupResult::Filter F = R.makeFilter(); diff --git a/core/metacling/src/TClingCallbacks.cxx b/core/metacling/src/TClingCallbacks.cxx index 21436c06cc1e5..770bd861f9594 100644 --- a/core/metacling/src/TClingCallbacks.cxx +++ b/core/metacling/src/TClingCallbacks.cxx @@ -82,9 +82,12 @@ extern "C" { } class AutoloadLibraryMU : public llvm::orc::MaterializationUnit { + const TClingCallbacks &fCallbacks; + std::string fLibrary; + llvm::orc::SymbolNameVector fSymbols; public: - AutoloadLibraryMU(const std::string &Library, const llvm::orc::SymbolNameVector &Symbols) - : MaterializationUnit(getSymbolFlagsMap(Symbols), nullptr), fLibrary(Library), fSymbols(Symbols) + AutoloadLibraryMU(const TClingCallbacks &cb, const std::string &Library, const llvm::orc::SymbolNameVector &Symbols) + : MaterializationUnit(getSymbolFlagsMap(Symbols), nullptr), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols) { } @@ -92,6 +95,11 @@ class AutoloadLibraryMU : public llvm::orc::MaterializationUnit { void materialize(std::unique_ptr R) override { + if (!fCallbacks.IsAutoLoadingEnabled()) { + R->failMaterialization(); + return; + } + llvm::orc::SymbolMap loadedSymbols; llvm::orc::SymbolNameSet failedSymbols; bool loadedLibrary = false; @@ -150,19 +158,22 @@ class AutoloadLibraryMU : public llvm::orc::MaterializationUnit { map[symbolName] = llvm::JITSymbolFlags::Exported; return map; } - - std::string fLibrary; - llvm::orc::SymbolNameVector fSymbols; }; class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator { + const TClingCallbacks &fCallbacks; + cling::Interpreter *fInterpreter; public: - AutoloadLibraryGenerator(cling::Interpreter *interp) : fInterpreter(interp) {} + AutoloadLibraryGenerator(cling::Interpreter *interp, const TClingCallbacks& cb) + : fCallbacks(cb), fInterpreter(interp) {} llvm::Error tryToGenerate(llvm::orc::LookupState &LS, llvm::orc::LookupKind K, llvm::orc::JITDylib &JD, llvm::orc::JITDylibLookupFlags JDLookupFlags, const llvm::orc::SymbolLookupSet &Symbols) override { + if (!fCallbacks.IsAutoLoadingEnabled()) + llvm::Error::success(); + // If we get here, the symbols have not been found in the current process, // so no need to check that again. Instead search for the library that // provides the symbol and create one MaterializationUnit per library to @@ -197,7 +208,7 @@ class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator { } for (auto &&KV : found) { - auto MU = std::make_unique(KV.first, std::move(KV.second)); + auto MU = std::make_unique(fCallbacks, KV.first, std::move(KV.second)); if (auto Err = JD.define(MU)) return Err; } @@ -207,9 +218,6 @@ class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator { return llvm::Error::success(); } - -private: - cling::Interpreter *fInterpreter; }; TClingCallbacks::TClingCallbacks(cling::Interpreter *interp, bool hasCodeGen) : InterpreterCallbacks(interp) @@ -219,7 +227,7 @@ TClingCallbacks::TClingCallbacks(cling::Interpreter *interp, bool hasCodeGen) : m_Interpreter->declare("namespace __ROOT_SpecialObjects{}", &T); fROOTSpecialNamespace = dyn_cast(T->getFirstDecl().getSingleDecl()); - interp->addGenerator(std::make_unique(interp)); + interp->addGenerator(std::make_unique(interp, *this)); } } diff --git a/core/metacling/src/TClingCallbacks.h b/core/metacling/src/TClingCallbacks.h index 6e5c547302dd7..9259e21867998 100644 --- a/core/metacling/src/TClingCallbacks.h +++ b/core/metacling/src/TClingCallbacks.h @@ -59,7 +59,7 @@ class TClingCallbacks : public cling::InterpreterCallbacks { void Initialize(); void SetAutoLoadingEnabled(bool val = true) { fIsAutoLoading = val; } - bool IsAutoLoadingEnabled() { return fIsAutoLoading; } + bool IsAutoLoadingEnabled() const { return fIsAutoLoading; } void SetAutoParsingSuspended(bool val = true) { fIsAutoParsingSuspended = val; } bool IsAutoParsingSuspended() { return fIsAutoParsingSuspended; } diff --git a/core/metacling/test/TClingDataMemberInfoTests.cxx b/core/metacling/test/TClingDataMemberInfoTests.cxx index 2ca87d29af8ef..a59c7bf29efc6 100644 --- a/core/metacling/test/TClingDataMemberInfoTests.cxx +++ b/core/metacling/test/TClingDataMemberInfoTests.cxx @@ -220,3 +220,35 @@ class Outer { auto *dmInnerProtected = (TDataMember*)TClass::GetClass("DMLookup::Outer")->GetListOfDataMembers()->FindObject("InnerProtected"); EXPECT_EQ(dmInnerProtected->Property(), kIsProtected | kIsClass); } + +TEST(TClingDataMemberInfo, Offset) +{ + gInterpreter->Declare("int ROOT_7459 = 42; ROOT_7459++;"); + EXPECT_TRUE(43 == *(int*)gROOT->GetGlobal("ROOT_7459")->GetAddress()); + + gInterpreter->Declare("constexpr int var1 = 1;"); + EXPECT_TRUE(1 == *(int*)gROOT->GetGlobal("var1")->GetAddress()); + + gInterpreter->Declare("static constexpr int var2 = -2;"); + EXPECT_TRUE(-2 == *(int*)gROOT->GetGlobal("var2")->GetAddress()); + + gInterpreter->Declare("const float var3 = 3.1;"); + EXPECT_FLOAT_EQ(3.1, *(float*)gROOT->GetGlobal("var3")->GetAddress()); + + gInterpreter->Declare("static const double var4 = 4.2;"); + EXPECT_DOUBLE_EQ(4.2, *(double*)gROOT->GetGlobal("var4")->GetAddress()); + + // Make sure ROOT's Core constexpr constants work + EXPECT_EQ(3000, *(int*)gROOT->GetGlobal("kError")->GetAddress()); + +#ifdef R__USE_CXXMODULES + // gGeoManager is defined in the Geom libraries and we want to make sure we + // do not load it when autoloading is off. We can only test this in modules + // mode because gGeoManager is not part of the PCH and non-modular ROOT has + // header parsing and autoloading coupled leading to redundant load of + // libGeom at gROOT->GetGlobal time. + TGlobal *GeoManagerInfo = gROOT->GetGlobal("gGeoManager"); + TInterpreter::SuspendAutoLoadingRAII autoloadOff(gInterpreter); + EXPECT_TRUE(-1L == (ptrdiff_t)GeoManagerInfo->GetAddress()); +#endif // R__USE_CXXMODULES +} From ce9bbc826d26b8a5275221e31e2eef5e6c588e7a Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 2/6] [core] Evaluate initializer of constant variables Co-authored-by: Jonas Hahnfeld --- core/metacling/src/TClingDataMemberInfo.cxx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/metacling/src/TClingDataMemberInfo.cxx b/core/metacling/src/TClingDataMemberInfo.cxx index e93a7c08b13f0..5052c0b42b887 100644 --- a/core/metacling/src/TClingDataMemberInfo.cxx +++ b/core/metacling/src/TClingDataMemberInfo.cxx @@ -354,8 +354,9 @@ Longptr_t TClingDataMemberInfo::Offset() if (Longptr_t addr = reinterpret_cast(fInterp->getAddressOfGlobal(GlobalDecl(VD)))) return addr; - auto evalStmt = VD->ensureEvaluatedStmt(); - if (evalStmt && evalStmt->Value) { + // We can't reassign constexpr or const variables. We can compute the + // initializer. + if (VD->hasInit() && (VD->isConstexpr() || VD->getType().isConstQualified())) { if (const APValue* val = VD->evaluateValue()) { if (VD->getType()->isIntegralType(C)) { return reinterpret_cast(val->getInt().getRawData()); From a2be28ed4acf4587580aeaab299030d6478ee789 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 3/6] [core] Prefer variable initializer over symbol search Symbol lookup is a quite expensive operation and might result in JIT compilation and library loading. Co-authored-by: Jonas Hahnfeld --- core/metacling/src/TClingDataMemberInfo.cxx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/metacling/src/TClingDataMemberInfo.cxx b/core/metacling/src/TClingDataMemberInfo.cxx index 5052c0b42b887..f90cbe7e2e9e0 100644 --- a/core/metacling/src/TClingDataMemberInfo.cxx +++ b/core/metacling/src/TClingDataMemberInfo.cxx @@ -352,8 +352,6 @@ Longptr_t TClingDataMemberInfo::Offset() // static constexpr Long64_t something = std::numeric_limits::max(); cling::Interpreter::PushTransactionRAII RAII(fInterp); - if (Longptr_t addr = reinterpret_cast(fInterp->getAddressOfGlobal(GlobalDecl(VD)))) - return addr; // We can't reassign constexpr or const variables. We can compute the // initializer. if (VD->hasInit() && (VD->isConstexpr() || VD->getType().isConstQualified())) { @@ -389,6 +387,10 @@ Longptr_t TClingDataMemberInfo::Offset() } // not integral type } // have an APValue } // have an initializing value + + // Try the slow operation. + if (Longptr_t addr = reinterpret_cast(fInterp->getAddressOfGlobal(GlobalDecl(VD)))) + return addr; } // FIXME: We have to explicitly check for not enum constant because the // implementation of getAddressOfGlobal relies on mangling the name and in From 14cc85ca31da6a191331866ada9e132500d6bc79 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Wed, 20 Dec 2023 18:24:47 +0000 Subject: [PATCH 4/6] Revert "[core] Materialize symbols for TError variables" This reverts commit 329fb5ae0c5717aec95fe50ec276f07450ac7f11. --- core/foundation/inc/TError.h | 17 +++++++++-------- core/foundation/src/RLogger.cxx | 2 +- core/foundation/src/TError.cxx | 9 --------- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/core/foundation/inc/TError.h b/core/foundation/inc/TError.h index 2948a70e6844e..0099d8e538704 100644 --- a/core/foundation/inc/TError.h +++ b/core/foundation/inc/TError.h @@ -39,14 +39,15 @@ class TVirtualMutex; -R__EXTERN const Int_t kUnset; -R__EXTERN const Int_t kPrint; -R__EXTERN const Int_t kInfo; -R__EXTERN const Int_t kWarning; -R__EXTERN const Int_t kError; -R__EXTERN const Int_t kBreak; -R__EXTERN const Int_t kSysError; -R__EXTERN const Int_t kFatal; +constexpr Int_t kUnset = -1; +constexpr Int_t kPrint = 0; +constexpr Int_t kInfo = 1000; +constexpr Int_t kWarning = 2000; +constexpr Int_t kError = 3000; +constexpr Int_t kBreak = 4000; +constexpr Int_t kSysError = 5000; +constexpr Int_t kFatal = 6000; + // TROOT sets the error ignore level handler, the system error message handler, and the error abort handler on // construction such that the "Root.ErrorIgnoreLevel" environment variable is used for the ignore level diff --git a/core/foundation/src/RLogger.cxx b/core/foundation/src/RLogger.cxx index 18ba9f954d959..de9eb939ab8ec 100644 --- a/core/foundation/src/RLogger.cxx +++ b/core/foundation/src/RLogger.cxx @@ -52,7 +52,7 @@ inline bool RLogHandlerDefault::Emit(const RLogEntry &entry) if (!entry.fLocation.fFuncName.empty()) strm << " in " << entry.fLocation.fFuncName; - static const int errorLevelOld[] = {kFatal /*unset*/, kFatal, kError, kWarning, kInfo, kInfo /*debug*/}; + static constexpr const int errorLevelOld[] = {kFatal /*unset*/, kFatal, kError, kWarning, kInfo, kInfo /*debug*/}; (*::GetErrorHandler())(errorLevelOld[cappedLevel], entry.fLevel == ELogLevel::kFatal, strm.str().c_str(), entry.fMessage.c_str()); return true; diff --git a/core/foundation/src/TError.cxx b/core/foundation/src/TError.cxx index 969f11f48a33a..8f27b76c5d72b 100644 --- a/core/foundation/src/TError.cxx +++ b/core/foundation/src/TError.cxx @@ -28,15 +28,6 @@ to be replaced by the proper DefaultErrorHandler() #include #include -const Int_t kUnset = -1; -const Int_t kPrint = 0; -const Int_t kInfo = 1000; -const Int_t kWarning = 2000; -const Int_t kError = 3000; -const Int_t kBreak = 4000; -const Int_t kSysError = 5000; -const Int_t kFatal = 6000; - Int_t gErrorIgnoreLevel = kUnset; Int_t gErrorAbortLevel = kSysError+1; Bool_t gPrintViaErrorHandler = kFALSE; From 284c5d5e5e1db8b657b5f493a5c9da4b1f46eef0 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Mon, 8 Jan 2024 11:53:20 +0100 Subject: [PATCH 5/6] [core] Make some constants constexpr This allows us to avoid generating symbols in libCore for these constants keeping the same amount of open calls at ROOT startup time. --- core/base/inc/TString.h | 2 +- core/base/src/TString.cxx | 6 ----- core/foundation/inc/RtypesCore.h | 38 ++++++++++++++++---------------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 9444dcc4d0489..6690b2cecc453 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -277,7 +277,7 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { public: enum EStripType { kLeading = 0x1, kTrailing = 0x2, kBoth = 0x3 }; enum ECaseCompare { kExact, kIgnoreCase }; - static const Ssiz_t kNPOS = ::kNPOS; + static constexpr Ssiz_t kNPOS = ::kNPOS; TString(); // Null string explicit TString(Ssiz_t ic); // Suggested capacity diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index 4087cdbb525ed..f6efde6a0a6fb 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -54,12 +54,6 @@ as a TString, construct a TString from it, eg: #include "TVirtualMutex.h" #include "ThreadLocalStorage.h" -// Definition of the TString static data member. Declaration (even with -// initialization) in the class body *is not* definition according to C++ -// standard. The definition must be explicitly done in one TU for ODR use. See -// https://en.cppreference.com/w/cpp/language/definition -const Ssiz_t TString::kNPOS; - #if defined(R__WIN32) #define strtoull _strtoui64 #endif diff --git a/core/foundation/inc/RtypesCore.h b/core/foundation/inc/RtypesCore.h index 442d769b98647..f6736b2277687 100644 --- a/core/foundation/inc/RtypesCore.h +++ b/core/foundation/inc/RtypesCore.h @@ -97,31 +97,31 @@ typedef float Size_t; //Attribute size (float) //---- constants --------------------------------------------------------------- -const Bool_t kTRUE = true; -const Bool_t kFALSE = false; +constexpr Bool_t kTRUE = true; +constexpr Bool_t kFALSE = false; -const Int_t kMaxUChar = 256; -const Int_t kMaxChar = kMaxUChar >> 1; -const Int_t kMinChar = -kMaxChar - 1; +constexpr Int_t kMaxUChar = 256; +constexpr Int_t kMaxChar = kMaxUChar >> 1; +constexpr Int_t kMinChar = -kMaxChar - 1; -const Int_t kMaxUShort = 65534; -const Int_t kMaxShort = kMaxUShort >> 1; -const Int_t kMinShort = -kMaxShort - 1; +constexpr Int_t kMaxUShort = 65534; +constexpr Int_t kMaxShort = kMaxUShort >> 1; +constexpr Int_t kMinShort = -kMaxShort - 1; -const UInt_t kMaxUInt = UInt_t(~0); -const Int_t kMaxInt = Int_t(kMaxUInt >> 1); -const Int_t kMinInt = -kMaxInt - 1; +constexpr UInt_t kMaxUInt = UInt_t(~0); +constexpr Int_t kMaxInt = Int_t(kMaxUInt >> 1); +constexpr Int_t kMinInt = -kMaxInt - 1; -const ULong_t kMaxULong = ULong_t(~0); -const Long_t kMaxLong = Long_t(kMaxULong >> 1); -const Long_t kMinLong = -kMaxLong - 1; +constexpr ULong_t kMaxULong = ULong_t(~0); +constexpr Long_t kMaxLong = Long_t(kMaxULong >> 1); +constexpr Long_t kMinLong = -kMaxLong - 1; -const ULong64_t kMaxULong64 = ULong64_t(~0LL); -const Long64_t kMaxLong64 = Long64_t(kMaxULong64 >> 1); -const Long64_t kMinLong64 = -kMaxLong64 - 1; +constexpr ULong64_t kMaxULong64 = ULong64_t(~0LL); +constexpr Long64_t kMaxLong64 = Long64_t(kMaxULong64 >> 1); +constexpr Long64_t kMinLong64 = -kMaxLong64 - 1; -const ULong_t kBitsPerByte = 8; -const Ssiz_t kNPOS = ~(Ssiz_t)0; +constexpr ULong_t kBitsPerByte = 8; +constexpr Ssiz_t kNPOS = ~(Ssiz_t)0; //---- debug global ------------------------------------------------------------ From d4f8f6aa802bacb0dd592ae66c90f2a1eeb0cb07 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 16 Jan 2024 21:49:44 +0100 Subject: [PATCH 6/6] [core] Fix statements at global scope in TCling test `TInterpreter::Declare` does not support issuing statements on the global scope, while `TInterpreter::ProcessLine` does. This test was first introduced in a development version of ROOT (6.31), where the upgrade to LLVM16 was already in place. In that scenario, clang supports global scope statements (thanks to changes made for clang-repl). Applying the test to ROOT 6.30 uncovered the problem, since the clang of LLVM13 does not support global scope statements. Fix the test by using `ProcessLine` instead of `Declare` which does the intended thing independently from the ROOT version. Co-authored-by: Vassil Vassilev --- core/metacling/test/TClingDataMemberInfoTests.cxx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/metacling/test/TClingDataMemberInfoTests.cxx b/core/metacling/test/TClingDataMemberInfoTests.cxx index a59c7bf29efc6..f29dd242408f7 100644 --- a/core/metacling/test/TClingDataMemberInfoTests.cxx +++ b/core/metacling/test/TClingDataMemberInfoTests.cxx @@ -223,19 +223,19 @@ class Outer { TEST(TClingDataMemberInfo, Offset) { - gInterpreter->Declare("int ROOT_7459 = 42; ROOT_7459++;"); + gInterpreter->ProcessLine("int ROOT_7459 = 42; ROOT_7459++;"); EXPECT_TRUE(43 == *(int*)gROOT->GetGlobal("ROOT_7459")->GetAddress()); - gInterpreter->Declare("constexpr int var1 = 1;"); + gInterpreter->ProcessLine("constexpr int var1 = 1;"); EXPECT_TRUE(1 == *(int*)gROOT->GetGlobal("var1")->GetAddress()); - gInterpreter->Declare("static constexpr int var2 = -2;"); + gInterpreter->ProcessLine("static constexpr int var2 = -2;"); EXPECT_TRUE(-2 == *(int*)gROOT->GetGlobal("var2")->GetAddress()); - gInterpreter->Declare("const float var3 = 3.1;"); + gInterpreter->ProcessLine("const float var3 = 3.1;"); EXPECT_FLOAT_EQ(3.1, *(float*)gROOT->GetGlobal("var3")->GetAddress()); - gInterpreter->Declare("static const double var4 = 4.2;"); + gInterpreter->ProcessLine("static const double var4 = 4.2;"); EXPECT_DOUBLE_EQ(4.2, *(double*)gROOT->GetGlobal("var4")->GetAddress()); // Make sure ROOT's Core constexpr constants work