From 984d562aab4809643e9c7827500c858f8b2c9e51 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 1/4] [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 1870909bd0528..34da5bf039ada 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -4726,6 +4726,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 @@ -4756,8 +4759,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 c3ce75d1c23a9..d8ce47ba40196 100644 --- a/core/metacling/src/TClingCallbacks.cxx +++ b/core/metacling/src/TClingCallbacks.cxx @@ -84,9 +84,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) { } @@ -94,6 +97,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; @@ -152,19 +160,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 @@ -199,7 +210,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; } @@ -210,9 +221,6 @@ class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator { return llvm::Error::success(); } - -private: - cling::Interpreter *fInterpreter; }; TClingCallbacks::TClingCallbacks(cling::Interpreter *interp, bool hasCodeGen) : InterpreterCallbacks(interp) @@ -222,7 +230,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 6663cd9796cdc..cb8f710eea791 100644 --- a/core/metacling/src/TClingCallbacks.h +++ b/core/metacling/src/TClingCallbacks.h @@ -58,7 +58,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 c22be553e76a74a14b2b5d4f1e93b3279f61416c Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 2/4] [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 566e73c9c1f2c54be1462481e6833b4c8ba70312 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Mon, 8 Jan 2024 10:21:54 +0000 Subject: [PATCH 3/4] [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 55145f099aed8f511450cdb0f9577e5113cd663c Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Wed, 20 Dec 2023 18:24:47 +0000 Subject: [PATCH 4/4] 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;