diff --git a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/_batchgenerator.py b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/_batchgenerator.py index b620ed724a9f4..eb71c7581b17b 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/_batchgenerator.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/_batchgenerator.py @@ -197,7 +197,13 @@ def __init__( self.train_columns = [c for c in self.all_columns if c not in [target, weights]] - from ROOT import TMVA + from ROOT import TMVA, EnableThreadSafety + + # The RBatchGenerator will create a separate C++ thread for I/O. + # Enable thread safety in ROOT from here, to make sure there is no + # interference between the main Python thread (which might call into + # cling via cppyy) and the I/O thread. + EnableThreadSafety() self.generator = TMVA.Experimental.Internal.RBatchGenerator(template)( tree_name, diff --git a/core/meta/src/TFunction.cxx b/core/meta/src/TFunction.cxx index d687fc8a050eb..2809a8e638bc5 100644 --- a/core/meta/src/TFunction.cxx +++ b/core/meta/src/TFunction.cxx @@ -37,6 +37,9 @@ TFunction::TFunction(MethodInfo_t *info) : TDictionary() fInfo = info; fMethodArgs = nullptr; if (fInfo) { + // The next calls into TClingMethodInfo methods lock the interpreter. Lock + // once here instead of locking/unlocking every time. + R__LOCKGUARD(gInterpreterMutex); SetName(gCling->MethodInfo_Name(fInfo)); SetTitle(gCling->MethodInfo_Title(fInfo)); fMangledName = gCling->MethodInfo_GetMangledName(fInfo); @@ -49,7 +52,7 @@ TFunction::TFunction(MethodInfo_t *info) : TDictionary() TFunction::TFunction(const TFunction &orig) : TDictionary(orig) { if (orig.fInfo) { - R__LOCKGUARD(gInterpreterMutex); + // The next call locks the interpreter mutex. fInfo = gCling->MethodInfo_FactoryCopy(orig.fInfo); fMangledName = orig.fMangledName; } else @@ -63,6 +66,8 @@ TFunction::TFunction(const TFunction &orig) : TDictionary(orig) TFunction& TFunction::operator=(const TFunction &rhs) { if (this != &rhs) { + // The next calls lock the interpreter mutex. Lock once here instead of + // locking/unlocking every time. R__LOCKGUARD(gInterpreterMutex); gCling->MethodInfo_Delete(fInfo); if (fMethodArgs) fMethodArgs->Delete(); @@ -84,7 +89,6 @@ TFunction& TFunction::operator=(const TFunction &rhs) TFunction::~TFunction() { - R__LOCKGUARD(gInterpreterMutex); gCling->MethodInfo_Delete(fInfo); if (fMethodArgs) fMethodArgs->Delete(); @@ -96,6 +100,7 @@ TFunction::~TFunction() TObject *TFunction::Clone(const char *newname) const { + // The constructor locks the interpreter mutex. TNamed *newobj = new TFunction(*this); if (newname && strlen(newname)) newobj->SetName(newname); return newobj; @@ -106,7 +111,8 @@ TObject *TFunction::Clone(const char *newname) const void TFunction::CreateSignature() { - R__LOCKGUARD(gInterpreterMutex); + // The next call locks the interpreter mutex. The result is cached in the + // fSignature data member. gCling->MethodInfo_CreateSignature(fInfo, fSignature); } @@ -116,6 +122,8 @@ void TFunction::CreateSignature() const char *TFunction::GetSignature() { if (fInfo && fSignature.IsNull()) + // The next call locks the interpreter mutex. + // The result is cached in the fSignature data member. CreateSignature(); return fSignature.Data(); @@ -130,6 +138,7 @@ TList *TFunction::GetListOfMethodArgs() if (!gInterpreter) Fatal("GetListOfMethodArgs", "gInterpreter not initialized"); + // The next call locks the interpreter mutex. gInterpreter->CreateListOfMethodArgs(this); } return fMethodArgs; @@ -140,7 +149,7 @@ TList *TFunction::GetListOfMethodArgs() const char *TFunction::GetReturnTypeName() const { - R__LOCKGUARD(gInterpreterMutex); + // The next calls lock the interpreter mutex. if (fInfo == nullptr || gCling->MethodInfo_Type(fInfo) == nullptr) return "Unknown"; return gCling->MethodInfo_TypeName(fInfo); } @@ -154,7 +163,7 @@ const char *TFunction::GetReturnTypeName() const std::string TFunction::GetReturnTypeNormalizedName() const { - R__LOCKGUARD(gInterpreterMutex); + // The next calls lock the interpreter mutex. if (fInfo == nullptr || gCling->MethodInfo_Type(fInfo) == nullptr) return "Unknown"; return gCling->MethodInfo_TypeNormalizedName(fInfo); } @@ -183,6 +192,7 @@ Int_t TFunction::GetNargsOpt() const Long_t TFunction::Property() const { + // The next call locks the interpreter mutex. return fInfo ? gCling->MethodInfo_Property(fInfo) : 0; } @@ -191,6 +201,7 @@ Long_t TFunction::Property() const Long_t TFunction::ExtraProperty() const { + // The next call locks the interpreter mutex. return fInfo ? gCling->MethodInfo_ExtraProperty(fInfo) : 0; } @@ -198,6 +209,7 @@ Long_t TFunction::ExtraProperty() const TDictionary::DeclId_t TFunction::GetDeclId() const { + // The next call locks the interpreter mutex. return gInterpreter->GetDeclId(fInfo); } @@ -208,6 +220,7 @@ TDictionary::DeclId_t TFunction::GetDeclId() const void *TFunction::InterfaceMethod() const { + // The next call locks the interpreter mutex. return fInfo ? gCling->MethodInfo_InterfaceMethod(fInfo) : nullptr; } @@ -221,8 +234,10 @@ Bool_t TFunction::IsValid() // Register the transaction when checking the validity of the object. if (!fInfo && UpdateInterpreterStateMarker()) { // Only for global functions. For data member functions TMethod does it. + // The next calls lock the interpreter mutex. DeclId_t newId = gInterpreter->GetFunction(nullptr, fName); if (newId) { + // The next call locks the interpreter mutex. (TODO: why?) MethodInfo_t *info = gInterpreter->MethodInfo_Factory(newId); Update(info); } @@ -246,7 +261,7 @@ const char *TFunction::GetMangledName() const const char *TFunction::GetPrototype() const { if (fInfo) { - R__LOCKGUARD(gInterpreterMutex); + // The next call locks the interpreter mutex. return gCling->MethodInfo_GetPrototype(fInfo); } else return nullptr; @@ -278,10 +293,14 @@ void TFunction::Print(Option_t *options /* ="" */) const Bool_t TFunction::Update(MethodInfo_t *info) { + // This function needs to lock access to the interpreter multiple times. + // Take the lock at the beginning of the function so that we don't incur + // in too much locking/unlocking. + R__LOCKGUARD(gInterpreterMutex); if (info == nullptr) { if (fInfo) { - R__LOCKGUARD(gInterpreterMutex); + // The next call locks the interpreter mutex. gCling->MethodInfo_Delete(fInfo); } fInfo = nullptr; @@ -294,10 +313,11 @@ Bool_t TFunction::Update(MethodInfo_t *info) return kTRUE; } else { if (fInfo) { - R__LOCKGUARD(gInterpreterMutex); + // The next call locks the interpreter mutex. gCling->MethodInfo_Delete(fInfo); } fInfo = info; + // The next call locks the interpreter mutex. TString newMangledName = gCling->MethodInfo_GetMangledName(fInfo); if (newMangledName != fMangledName) { Error("Update","TFunction object updated with the 'wrong' MethodInfo (%s vs %s).", @@ -305,11 +325,12 @@ Bool_t TFunction::Update(MethodInfo_t *info) fInfo = nullptr; return false; } + // The next call locks the interpreter mutex. SetTitle(gCling->MethodInfo_Title(fInfo)); if (fMethodArgs) { + // TODO: Check for MethodArgInfo thread-safety MethodArgInfo_t *arg = gCling->MethodArgInfo_Factory(fInfo); Int_t i = 0; - R__LOCKGUARD(gInterpreterMutex); while (gCling->MethodArgInfo_Next(arg)) { if (gCling->MethodArgInfo_IsValid(arg)) { MethodArgInfo_t *new_arg = gCling->MethodArgInfo_FactoryCopy(arg); diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx index 4b0de29673a27..c57add0b04abe 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -7029,6 +7029,11 @@ static std::string GetClassSharedLibsForModule(const char *cls, cling::LookupHel const char* TCling::GetClassSharedLibs(const char* cls) { if (fCxxModulesEnabled) { + // Lock the interpreter mutex before interacting with cling. + // TODO: Can we move this further deep? In principle the lock should be in + // GetClassSharedLibsForModule, but it might be needed also for + // getLookupHelper? + R__LOCKGUARD(gInterpreterMutex); llvm::StringRef className = cls; // If we get a class name containing lambda, we cannot parse it and we // can exit early. @@ -8936,6 +8941,7 @@ void TCling::MethodInfo_Delete(MethodInfo_t* minfo) const void TCling::MethodInfo_CreateSignature(MethodInfo_t* minfo, TString& signature) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. info->CreateSignature(signature); } @@ -8976,6 +8982,7 @@ MethodInfo_t* TCling::MethodInfo_FactoryCopy(MethodInfo_t* minfo) const void* TCling::MethodInfo_InterfaceMethod(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->InterfaceMethod(); } @@ -9016,6 +9023,7 @@ int TCling::MethodInfo_Next(MethodInfo_t* minfo) const Long_t TCling::MethodInfo_Property(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->Property(); } @@ -9024,6 +9032,7 @@ Long_t TCling::MethodInfo_Property(MethodInfo_t* minfo) const Long_t TCling::MethodInfo_ExtraProperty(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->ExtraProperty(); } @@ -9032,6 +9041,7 @@ Long_t TCling::MethodInfo_ExtraProperty(MethodInfo_t* minfo) const TypeInfo_t* TCling::MethodInfo_Type(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return (TypeInfo_t*)info->Type(); } @@ -9041,6 +9051,7 @@ const char* TCling::MethodInfo_GetMangledName(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; TTHREAD_TLS_DECL(TString, mangled_name); + // The next call locks the interpreter mutex. mangled_name = info->GetMangledName(); return mangled_name; } @@ -9050,6 +9061,7 @@ const char* TCling::MethodInfo_GetMangledName(MethodInfo_t* minfo) const const char* TCling::MethodInfo_GetPrototype(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->GetPrototype(); } @@ -9058,6 +9070,7 @@ const char* TCling::MethodInfo_GetPrototype(MethodInfo_t* minfo) const const char* TCling::MethodInfo_Name(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->Name(); } @@ -9066,6 +9079,7 @@ const char* TCling::MethodInfo_Name(MethodInfo_t* minfo) const const char* TCling::MethodInfo_TypeName(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->TypeName(); } @@ -9074,6 +9088,7 @@ const char* TCling::MethodInfo_TypeName(MethodInfo_t* minfo) const std::string TCling::MethodInfo_TypeNormalizedName(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next part locks the interpreter mutex. if (info && info->IsValid()) return info->Type()->NormalizedName(*fNormalizedCtxt); else @@ -9085,6 +9100,7 @@ std::string TCling::MethodInfo_TypeNormalizedName(MethodInfo_t* minfo) const const char* TCling::MethodInfo_Title(MethodInfo_t* minfo) const { TClingMethodInfo* info = (TClingMethodInfo*) minfo; + // The next call locks the interpreter mutex. return info->Title(); } diff --git a/core/metacling/src/TClingMethodInfo.cxx b/core/metacling/src/TClingMethodInfo.cxx index 06ef2e6219050..125f5d111bcf8 100644 --- a/core/metacling/src/TClingMethodInfo.cxx +++ b/core/metacling/src/TClingMethodInfo.cxx @@ -236,6 +236,7 @@ TClingMethodInfo::TClingMethodInfo(cling::Interpreter *interp, TClingClassInfo *ci) : TClingDeclInfo(nullptr), fInterp(interp), fFirstTime(true), fTitle("") { + // Creating an interpreter transaction, needs locking. R__LOCKGUARD(gInterpreterMutex); if (!ci || !ci->IsValid()) { @@ -285,6 +286,8 @@ TDictionary::DeclId_t TClingMethodInfo::GetDeclId() const if (!IsValid()) { return TDictionary::DeclId_t(); } + // Next part interacts with clang, needs locking + R__LOCKGUARD(gInterpreterMutex); if (auto *FD = GetAsFunctionDecl()) return (const clang::Decl*)(FD->getCanonicalDecl()); return (const clang::Decl*)(GetAsUsingShadowDecl()->getCanonicalDecl()); @@ -302,6 +305,8 @@ const clang::UsingShadowDecl *TClingMethodInfo::GetAsUsingShadowDecl() const const clang::FunctionDecl *TClingMethodInfo::GetTargetFunctionDecl() const { + // May need to resolve the declaration interacting with clang, needs locking. + R__LOCKGUARD(gInterpreterMutex); const Decl *D = GetDecl(); do { if (auto FD = dyn_cast(D)) @@ -356,6 +361,7 @@ void *TClingMethodInfo::InterfaceMethod() const if (!IsValid()) { return nullptr; } + // TODO: can this lock be moved further deep? R__LOCKGUARD(gInterpreterMutex); TClingCallFunc cf(fInterp); cf.SetFunc(this); @@ -372,6 +378,7 @@ int TClingMethodInfo::NArg() const if (!IsValid()) { return -1; } + // The next call locks the interpreter mutex. const clang::FunctionDecl *fd = GetTargetFunctionDecl(); unsigned num_params = fd->getNumParams(); // Truncate cast to fit cint interface. @@ -383,6 +390,7 @@ int TClingMethodInfo::NDefaultArg() const if (!IsValid()) { return -1; } + // The next call locks the interpreter mutex. const clang::FunctionDecl *fd = GetTargetFunctionDecl(); unsigned num_params = fd->getNumParams(); unsigned min_args = fd->getMinRequiredArguments(); @@ -440,11 +448,14 @@ long TClingMethodInfo::Property() const if (llvm::isa(declAccess)) property |= kIsUsing; + // The next call locks the interpreter mutex. const clang::FunctionDecl *fd = GetTargetFunctionDecl(); clang::AccessSpecifier Access = clang::AS_public; if (!declAccess->getDeclContext()->isNamespace()) Access = declAccess->getAccess(); + // From here on the method interacts with clang directly, needs locking. + R__LOCKGUARD(gInterpreterMutex); if ((property & kIsUsing) && llvm::isa(fd)) { Access = clang::AS_public; clang::CXXRecordDecl *typeCXXRD = llvm::cast(Type()->GetQualType())->getAsCXXRecordDecl(); @@ -528,6 +539,7 @@ long TClingMethodInfo::ExtraProperty() const return 0L; } long property = 0; + // The next call locks the interpreter mutex. const clang::FunctionDecl *fd = GetTargetFunctionDecl(); if (fd->isOverloadedOperator()) property |= kIsOperator; @@ -551,6 +563,9 @@ TClingTypeInfo *TClingMethodInfo::Type() const ti.Init(clang::QualType()); return &ti; } + + // The next part interacts with clang, thus needs locking. + R__LOCKGUARD(gInterpreterMutex); if (llvm::isa(GetTargetFunctionDecl())) { // CINT claims that constructors return the class object. // For using-ctors of a base, claim that it "returns" the derived class. @@ -578,6 +593,7 @@ std::string TClingMethodInfo::GetMangledName() const mangled_name.clear(); const FunctionDecl* D = GetTargetFunctionDecl(); + // Creating an interpreter transaction, needs locking. R__LOCKGUARD(gInterpreterMutex); cling::Interpreter::PushTransactionRAII RAII(fInterp); GlobalDecl GD; @@ -601,10 +617,13 @@ const char *TClingMethodInfo::GetPrototype() buf.clear(); buf += Type()->Name(); buf += ' '; + // The next call locks the interpreter mutex. const FunctionDecl *FD = GetTargetFunctionDecl(); // Use the DeclContext of the decl, not of the target decl: // Used base functions should show as if they are part of the derived class, // e.g. `Derived Derived::Derived(int)`, not `Derived Base::Derived(int)`. + // Interacting with clang in the next part, needs locking + R__LOCKGUARD(gInterpreterMutex); if (const clang::TypeDecl *td = llvm::dyn_cast(GetDecl()->getDeclContext())) { std::string name; clang::QualType qualType(td->getTypeForDecl(),0); @@ -643,7 +662,13 @@ const char *TClingMethodInfo::Name() const if (!fNameCache.empty()) return fNameCache.c_str(); - ((TCling*)gCling)->GetFunctionName(GetDecl(), fNameCache); + { + // The data member needs to be filled. This calls into the interpreter, + // needs locking. + // TODO: Check if the lock can be moved further deep. + R__LOCKGUARD(gInterpreterMutex); + ((TCling *)gCling)->GetFunctionName(GetDecl(), fNameCache); + } return fNameCache.c_str(); } @@ -653,6 +678,9 @@ const char *TClingMethodInfo::TypeName() const // FIXME: Cint does not check! return nullptr; } + // The next *two* calls lock the interpreter mutex. Lock here first instead + // of locking/unlocking twice. + R__LOCKGUARD(gInterpreterMutex); return Type()->Name(); } @@ -672,6 +700,7 @@ const char *TClingMethodInfo::Title() // redecl chain (came from merging of pcms). const FunctionDecl *FD = GetTargetFunctionDecl(); + // Creating an interpreter transaction, needs locking. R__LOCKGUARD(gInterpreterMutex); // Could trigger deserialization of decls. diff --git a/core/metacling/src/TClingTypeInfo.cxx b/core/metacling/src/TClingTypeInfo.cxx index f5cf1c321ef64..a8363534c73e6 100644 --- a/core/metacling/src/TClingTypeInfo.cxx +++ b/core/metacling/src/TClingTypeInfo.cxx @@ -108,6 +108,8 @@ const char *TClingTypeInfo::Name() const TTHREAD_TLS_DECL( std::string, buf); buf.clear(); + // TODO: This needs to be locked, but the lock cannot be placed in TClingUtils.cxx as it cannot depend from + // TInterpreter.h for the declaration of gInterpreterMutex. Or can it? R__LOCKGUARD(gInterpreterMutex); ROOT::TMetaUtils::GetFullyQualifiedTypeName(buf,fQualType,*fInterp); return buf.c_str(); // NOLINT @@ -242,6 +244,9 @@ const char *TClingTypeInfo::TrueName(const ROOT::TMetaUtils::TNormalizedCtxt &no TTHREAD_TLS_DECL( std::string, buf); buf.clear(); + // TODO: This needs to be locked, but the lock cannot be placed in TClingUtils.cxx as it cannot depend from + // TInterpreter.h for the declaration of gInterpreterMutex. Or can it? + R__LOCKGUARD(gInterpreterMutex); ROOT::TMetaUtils::GetNormalizedName(buf,fQualType, *fInterp, normCtxt); return buf.c_str(); // NOLINT @@ -257,6 +262,10 @@ std::string TClingTypeInfo::NormalizedName(const ROOT::TMetaUtils::TNormalizedCt return ""; } std::string buf; + + // TODO: This needs to be locked, but the lock cannot be placed in TClingUtils.cxx as it cannot depend from + // TInterpreter.h for the declaration of gInterpreterMutex. Or can it? + R__LOCKGUARD(gInterpreterMutex); ROOT::TMetaUtils::GetNormalizedName(buf,fQualType, *fInterp, normCtxt); // in C++11 this will be efficient thanks to the move constructor. diff --git a/tutorials/CMakeLists.txt b/tutorials/CMakeLists.txt index 1897d24b56ce1..88694a43265c2 100644 --- a/tutorials/CMakeLists.txt +++ b/tutorials/CMakeLists.txt @@ -77,7 +77,10 @@ if(NOT clad) set(clad_veto fit/minuit2GausFit.C) endif() -if (PYTHON_VERSION_STRING_Development_Main VERSION_LESS 3.8) +# RBatchGenerator tutorials need Python 3.8+. +# Also, they don't work on Windows at the moment. +if (PYTHON_VERSION_STRING_Development_Main VERSION_LESS 3.8 OR + (MSVC AND NOT win_broken_tests)) list(APPEND dataframe_veto tmva/RBatchGenerator_NumPy.py) list(APPEND dataframe_veto tmva/RBatchGenerator_TensorFlow.py) list(APPEND dataframe_veto tmva/RBatchGenerator_PyTorch.py) @@ -470,13 +473,6 @@ set(all_veto hsimple.C ${clad_veto} ) -# These tutorials seem to trigger segfaults related to TClass* and RDataFrame -# JITting on many platforms. Disable for now and revert once the issue is fixed -list(APPEND all_veto tmva/RBatchGenerator_NumPy.py) -list(APPEND all_veto tmva/RBatchGenerator_TensorFlow.py) -list(APPEND all_veto tmva/RBatchGenerator_PyTorch.py) -list(APPEND all_veto tmva/RBatchGenerator_filters_vectors.py) - file(GLOB_RECURSE tutorials RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.C) if(root7 AND webgui) file(GLOB_RECURSE tutorials_v7 RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} v7/*.cxx)