Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increase thread safety to fix RBatchGenerator tutorials segfaults #13302

Merged
merged 6 commits into from
Sep 8, 2023
Merged

Increase thread safety to fix RBatchGenerator tutorials segfaults #13302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c6f8ffb
Re-enable RBatchGenerator tutorials
vepadulano Jul 23, 2023
1cd1ec8
[tmva] Activate thread safety in pythonizations of RBatchGenerator
vepadulano Jul 23, 2023
4155876
[core] Improve TFunction + TClingMethodInfo thread-safety
vepadulano Jul 28, 2023
71d4611
[core] Lock interpreter in TCling::GetClassSharedLibs
vepadulano Jul 23, 2023
d396c1a
[core] Lock calls to TMetaUtils in TClingTypeInfo
vepadulano Jul 28, 2023
dd3a6cc
[tutorial] Veto RBatchGenerator tutorials on windows
vepadulano Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/_batchgenerator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
39 changes: 30 additions & 9 deletions core/meta/src/TFunction.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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();
Expand All @@ -84,7 +89,6 @@ TFunction& TFunction::operator=(const TFunction &rhs)

TFunction::~TFunction()
{
R__LOCKGUARD(gInterpreterMutex);
gCling->MethodInfo_Delete(fInfo);

if (fMethodArgs) fMethodArgs->Delete();
Expand All @@ -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;
Expand All @@ -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);
}

Expand All @@ -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();
Expand All @@ -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;
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -191,13 +201,15 @@ Long_t TFunction::Property() const

Long_t TFunction::ExtraProperty() const
{
// The next call locks the interpreter mutex.
return fInfo ? gCling->MethodInfo_ExtraProperty(fInfo) : 0;
}

////////////////////////////////////////////////////////////////////////////////

TDictionary::DeclId_t TFunction::GetDeclId() const
{
// The next call locks the interpreter mutex.
return gInterpreter->GetDeclId(fInfo);
}

Expand All @@ -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;
}

Expand All @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -294,22 +313,24 @@ 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.
vepadulano marked this conversation as resolved.
Show resolved Hide resolved
TString newMangledName = gCling->MethodInfo_GetMangledName(fInfo);
if (newMangledName != fMangledName) {
Error("Update","TFunction object updated with the 'wrong' MethodInfo (%s vs %s).",
fMangledName.Data(),newMangledName.Data());
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);
Expand Down
16 changes: 16 additions & 0 deletions core/metacling/src/TCling.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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;
}
Expand All @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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
Expand All @@ -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();
}

Expand Down
Loading
Loading