From 6001a66ce33f02dc64e5340a2eb32b70fd6e8ab5 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 13 Jan 2014 19:22:55 +0100 Subject: [PATCH 01/38] Fast and dirty version of setting special value in TObject::new Write special values into memory after TObject::new called in order to have TObject's constructor determine if the object was made on the heap. --- core/base/inc/TStorage.h | 7 +++++++ core/base/src/TStorage.cxx | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/core/base/inc/TStorage.h b/core/base/inc/TStorage.h index f81425e99acbd..f969ba12b6f38 100644 --- a/core/base/inc/TStorage.h +++ b/core/base/inc/TStorage.h @@ -80,7 +80,14 @@ class TStorage { ClassDef(TStorage,0) //Storage manager class }; +#define TSTORAGEMEMVALUE 0x99999999 #ifndef WIN32 +inline void TStorage::AddToHeap(ULong_t begin, ULong_t end) +{ } + +inline Bool_t TStorage::IsOnHeap(void *p) + { return (ULong_t)p >= fgHeapBegin && (ULong_t)p < fgHeapEnd; } + inline size_t TStorage::GetMaxBlockSize() { return fgMaxBlockSize; } inline void TStorage::SetMaxBlockSize(size_t size) { fgMaxBlockSize = size; } diff --git a/core/base/src/TStorage.cxx b/core/base/src/TStorage.cxx index 4c5653e3ec6df..0b4794800bed6 100644 --- a/core/base/src/TStorage.cxx +++ b/core/base/src/TStorage.cxx @@ -486,7 +486,6 @@ void TStorage::SetCustomNewDelete() void TStorage::AddToHeap(ULong_t, ULong_t) { //add a range to the heap - ::Obsolete("AddToHeap(ULong_t,ULong_t)", "v5-34-00", "v6-02-00"); } //______________________________________________________________________________ From 7d65924e1d17495ae76f4aecc8f6fddeb47c959e Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 13 Jan 2014 23:11:01 +0100 Subject: [PATCH 02/38] Cleaned up TStorage changes Removed obsolete static member data and obsolete function bodies. Added ::Obsolete calls where appropriate. --- core/base/inc/TStorage.h | 7 ------- core/base/src/TStorage.cxx | 1 + 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/core/base/inc/TStorage.h b/core/base/inc/TStorage.h index f969ba12b6f38..f81425e99acbd 100644 --- a/core/base/inc/TStorage.h +++ b/core/base/inc/TStorage.h @@ -80,14 +80,7 @@ class TStorage { ClassDef(TStorage,0) //Storage manager class }; -#define TSTORAGEMEMVALUE 0x99999999 #ifndef WIN32 -inline void TStorage::AddToHeap(ULong_t begin, ULong_t end) -{ } - -inline Bool_t TStorage::IsOnHeap(void *p) - { return (ULong_t)p >= fgHeapBegin && (ULong_t)p < fgHeapEnd; } - inline size_t TStorage::GetMaxBlockSize() { return fgMaxBlockSize; } inline void TStorage::SetMaxBlockSize(size_t size) { fgMaxBlockSize = size; } diff --git a/core/base/src/TStorage.cxx b/core/base/src/TStorage.cxx index 0b4794800bed6..4c5653e3ec6df 100644 --- a/core/base/src/TStorage.cxx +++ b/core/base/src/TStorage.cxx @@ -486,6 +486,7 @@ void TStorage::SetCustomNewDelete() void TStorage::AddToHeap(ULong_t, ULong_t) { //add a range to the heap + ::Obsolete("AddToHeap(ULong_t,ULong_t)", "v5-34-00", "v6-02-00"); } //______________________________________________________________________________ From f1f9d3b02b994379c80f143df0a962975981acb8 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 15 Jan 2014 23:21:09 +0100 Subject: [PATCH 03/38] Encapsulate check on alloc in TStorage::FilledByObjectAlloc Instead of having TObject's constructors know explicitly about how TStorage::ObjectAlloc works, we no encapsulate that knowledge into TStorage::FilledByObjectAlloc. Additionally, switched from fBits to fUniqueID when checking for assignment from 'new' in order to more easily create a valgrind suppression entry. --- core/base/inc/TStorage.h | 8 ++++++-- core/base/src/TObject.cxx | 22 ++++++++++++---------- core/base/src/TStorage.cxx | 15 ++++++++++----- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/core/base/inc/TStorage.h b/core/base/inc/TStorage.h index f81425e99acbd..afef2e6e4c507 100644 --- a/core/base/inc/TStorage.h +++ b/core/base/inc/TStorage.h @@ -30,8 +30,6 @@ typedef void *(*ReAllocFun_t)(void*, size_t); typedef void *(*ReAllocCFun_t)(void*, size_t, size_t); typedef char *(*ReAllocCharFun_t)(char*, size_t, size_t); -/*Magic number written to memory after TStorage::ObjectAlloc */ -#define TSTORAGEMEMVALUE 0x99999999 class TStorage { @@ -42,6 +40,8 @@ class TStorage { static ReAllocFun_t fgReAllocHook; // custom ReAlloc static ReAllocCFun_t fgReAllocCHook; // custom ReAlloc with length check static Bool_t fgHasCustomNewDelete; // true if using ROOT's new/delete + static const UInt_t kObjectAllocMemValue = 0x99999999; + // magic number for ObjectAlloc public: virtual ~TStorage() { } @@ -77,10 +77,14 @@ class TStorage { static void AddToHeap(ULong_t begin, ULong_t end); static Bool_t IsOnHeap(void *p); + static Bool_t FilledByObjectAlloc(UInt_t* member); + ClassDef(TStorage,0) //Storage manager class }; #ifndef WIN32 +inline Bool_t TStorage::FilledByObjectAlloc(UInt_t *member) { return *member == kObjectAllocMemValue; } + inline size_t TStorage::GetMaxBlockSize() { return fgMaxBlockSize; } inline void TStorage::SetMaxBlockSize(size_t size) { fgMaxBlockSize = size; } diff --git a/core/base/src/TObject.cxx b/core/base/src/TObject.cxx index ed17afd36feff..45a4c5835518c 100644 --- a/core/base/src/TObject.cxx +++ b/core/base/src/TObject.cxx @@ -62,7 +62,7 @@ Bool_t TObject::fgObjectStat = kTRUE; ClassImp(TObject) //______________________________________________________________________________ -TObject::TObject() : fUniqueID(0) //Need to leave FBits unset +TObject::TObject() : fBits(kNotDeleted) //Need to leave FUniqueID unset { // TObject constructor. It sets the two data words of TObject to their // initial values. The unique ID is set to 0 and the status word is @@ -71,11 +71,10 @@ TObject::TObject() : fUniqueID(0) //Need to leave FBits unset // (see TEnv) the object is added to the global TObjectTable for // bookkeeping. - if(fBits == TSTORAGEMEMVALUE) { - fBits = kNotDeleted|kIsOnHeap; - } else { - fBits = kNotDeleted; - } + if (TStorage::FilledByObjectAlloc(&fUniqueID)) + fBits |= kIsOnHeap; + + fUniqueID = 0; if (fgObjectStat) TObjectTable::AddObj(this); } @@ -85,16 +84,19 @@ TObject::TObject(const TObject &obj) { // TObject copy ctor. - fUniqueID = obj.fUniqueID; // when really unique don't copy + fBits = obj.fBits; - if (fBits == TSTORAGEMEMVALUE) - fBits = obj.fBits | kIsOnHeap; + if (TStorage::FilledByObjectAlloc(&fUniqueID)) + fBits |= kIsOnHeap; else - fBits = obj.fBits & ~kIsOnHeap; + fBits &= ~kIsOnHeap; fBits &= ~kIsReferenced; fBits &= ~kCanDelete; + //Set only after used in above call + fUniqueID = obj.fUniqueID; // when really unique don't copy + if (fgObjectStat) TObjectTable::AddObj(this); } diff --git a/core/base/src/TStorage.cxx b/core/base/src/TStorage.cxx index 4c5653e3ec6df..7e482380ef811 100644 --- a/core/base/src/TStorage.cxx +++ b/core/base/src/TStorage.cxx @@ -321,14 +321,12 @@ void *TStorage::ObjectAlloc(size_t sz) { // Used to allocate a TObject on the heap (via TObject::operator new()). // Directly after this routine one can call (in the TObject ctor) - // TStorage::IsOnHeap() to find out if the just created object is on + // TStorage::FilledByObjectAlloc() to find out if the just created object is on // the heap. - // Needs to be protected by global mutex - void* space = ::operator new(sz); - memset(space,TSTORAGEMEMVALUE,sz); - return (void*) space; + memset(space, kObjectAllocMemValue, sz); + return space; } //______________________________________________________________________________ @@ -498,6 +496,13 @@ Bool_t TStorage::IsOnHeap(void *) } #ifdef WIN32 +//______________________________________________________________________________ +Bool_t TStorage::FilledByObjectAlloc(UInt_t *member) +{ + //called by TObject's constructor to determine if object was created by call to new + return *member == kObjectAllocMemValue; +} + //______________________________________________________________________________ size_t TStorage::GetMaxBlockSize() { From d7e3e1b8a8277566f95399ca0753e842ca4fb3bc Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 15 Jan 2014 23:30:41 +0100 Subject: [PATCH 04/38] Added suppression entries for determining if TObject on heap To determine if TObject is on the heap we read the value of a member data before it has been set by the constructor. If it was heap allocated the TObject::operator new will have filled special values into the memory area. However, for stack allocated it is unset. This makes valgrind report an error. Given we intend this behavior, we suppress the message. --- etc/valgrind-root.supp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/etc/valgrind-root.supp b/etc/valgrind-root.supp index 019624d5fa0ae..1cf82fc3b6f74 100644 --- a/etc/valgrind-root.supp +++ b/etc/valgrind-root.supp @@ -484,6 +484,29 @@ fun:_ZL20G__cpp_setup_global0v } +######## ROOT TObject on heap + +{ + TObject::TObject() uses uninitialized value + Memcheck:Cond + fun:_ZN7TObjectC1Ev + ... +} + +{ + TObject::TObject(const TObject&) uses uninitialized value + Memcheck:Cond + fun:_ZN7TObjectC1ERKS_ + ... +} + +{ + gcc optimizer confuses valgrind on TObject::~TObject() + Memcheck:Cond + fun:_ZN7TObjectD1Ev + ... +} + ######### Misc { From 90ab19a81b1d5c1fe0d8625bb2dd849c79030fdc Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Tue, 28 Jan 2014 20:30:33 +0100 Subject: [PATCH 05/38] Make variable used for GUID of TKeys atomic Since TKeys can be made by different threads, it is necessary to protect the global used to assign the GUID of TKeys, keyAbsNumber so that we are guaranteed to get a unique value for each request. C++11 atomics give that guarantee. --- io/io/src/TKey.cxx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/io/io/src/TKey.cxx b/io/io/src/TKey.cxx index 0a86907212d5a..14ab4faea7ea5 100644 --- a/io/io/src/TKey.cxx +++ b/io/io/src/TKey.cxx @@ -46,6 +46,8 @@ // // ////////////////////////////////////////////////////////////////////////// +#include + #include "Riostream.h" #include "TROOT.h" #include "TClass.h" @@ -78,7 +80,7 @@ const ULong64_t kPidOffsetMask = 0xffffffffffffUL; const UChar_t kPidOffsetShift = 48; static TString gTDirectoryString = "TDirectory"; -UInt_t keyAbsNumber = 0; +std::atomic keyAbsNumber{0}; ClassImp(TKey) From 3b1874a6a3f6397a06d6df8eada3eece3b46d2ea Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 30 Jan 2014 16:53:30 +0100 Subject: [PATCH 06/38] Protect Cintex structure with mutex The caching of TClasses by Cintext could be updated at anytime and therefore needs to be protected when running multi-threaded. --- cint/cintex/src/ROOTClassEnhancer.cxx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/cint/cintex/src/ROOTClassEnhancer.cxx b/cint/cintex/src/ROOTClassEnhancer.cxx index 9c123639c06df..f7ddf8ef5c098 100644 --- a/cint/cintex/src/ROOTClassEnhancer.cxx +++ b/cint/cintex/src/ROOTClassEnhancer.cxx @@ -21,6 +21,7 @@ #include "TClassStreamer.h" #include "TCollectionProxyInfo.h" #include "TVirtualCollectionProxy.h" +#include "TVirtualMutex.h" #include "TMemberInspector.h" #include "RVersion.h" #include "Reflex/Reflex.h" @@ -44,6 +45,8 @@ using namespace ROOT::Reflex; using namespace ROOT::Cintex; using namespace std; +static TVirtualMutex* gCintexMutex = 0; + namespace ROOT { namespace Cintex { class IsAProxy; @@ -174,7 +177,10 @@ namespace ROOT { namespace Cintex { // Constructor. fType = CleanType(t); fName = CintName(fType); - rootEnhancerInfos().push_back(this); + { + R__LOCKGUARD2(gCintexMutex); + rootEnhancerInfos().push_back(this); + } fMyType = &t.TypeInfo(); fIsVirtual = TypeGet().IsVirtual(); fClassInfo = 0; @@ -373,7 +379,8 @@ namespace ROOT { namespace Cintex { if ( &typ == fMyType ) { return Tclass(); } - else if ( &typ == fLastType ) { + R__LOCKGUARD2(gCintexMutex); + if ( &typ == fLastType ) { return fLastClass; } // Check if TypeNth is already in sub-class cache From 11b11791d86785e699c1a81bb1bdf02a4fedd9cb Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 30 Jan 2014 16:56:10 +0100 Subject: [PATCH 07/38] Make zip globals thread local The zip functions use globals to hold state while calling from function to function. Such use is not thread safe. Given the globals are only used during a function call chain, it is safe to change them to thread local. Given this is compiled by a C compiler, we must use the gcc specific __thread keyword. --- core/zip/inc/Bits.h | 34 ++++++++++++++++++---------------- core/zip/inc/ZIP.h | 2 +- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/core/zip/inc/Bits.h b/core/zip/inc/Bits.h index fafb888107f56..b24de20c4efba 100644 --- a/core/zip/inc/Bits.h +++ b/core/zip/inc/Bits.h @@ -89,9 +89,9 @@ local void R__flush_outbuf OF((unsigned w, unsigned size)); /* =========================================================================== * Local data used by the "bit string" routines. */ -local FILE *zfile; /* output zip file */ +local __thread FILE *zfile; /* output zip file */ -local unsigned short bi_buf; +local __thread unsigned short bi_buf; /* Output buffer. bits are inserted starting at the bottom (least significant * bits). */ @@ -101,25 +101,25 @@ local unsigned short bi_buf; * more than 16 bits on some systems.) */ -local int bi_valid; +local __thread int bi_valid; /* Number of valid bits in bi_buf. All bits above the last valid bit * are always zero. */ -local char *in_buf, *out_buf; +local __thread char *in_buf, *out_buf; /* Current input and output buffers. in_buf is used only for in-memory * compression. */ -local unsigned in_offset, out_offset; +local __thread unsigned in_offset, out_offset; /* Current offset in input and output buffers. in_offset is used only for * in-memory compression. On 16 bit machiens, the buffer is limited to 64K. */ -local unsigned in_size, out_size; +local __thread unsigned in_size, out_size; /* Size of current input and output buffers */ -int (*R__read_buf) OF((char *buf, unsigned size)) = R__mem_read; +__thread int (*R__read_buf) OF((char *buf, unsigned size)) = R__mem_read; /* Current input function. Set to R__mem_read for in-memory compression */ #ifdef DEBUG @@ -411,7 +411,7 @@ local int R__mem_read(char *b, unsigned bsize) * * ***********************************************************************/ #define HDRSIZE 9 -static int error_flag; +static __thread int error_flag; void R__zipMultipleAlgorithm(int cxlevel, int *srcsize, char *src, int *tgtsize, char *tgt, int *irep, int compressionAlgorithm) /* int cxlevel; compression level */ @@ -496,6 +496,8 @@ void R__zipMultipleAlgorithm(int cxlevel, int *srcsize, char *src, int *tgtsize, } else { z_stream stream; + //Don't use the globals but want name similar to help see similarities in code + unsigned l_in_size, l_out_size; *irep = 0; error_flag = 0; @@ -538,15 +540,15 @@ void R__zipMultipleAlgorithm(int cxlevel, int *srcsize, char *src, int *tgtsize, tgt[1] = 'L'; tgt[2] = (char) method; - in_size = (unsigned) (*srcsize); - out_size = stream.total_out; /* compressed size */ - tgt[3] = (char)(out_size & 0xff); - tgt[4] = (char)((out_size >> 8) & 0xff); - tgt[5] = (char)((out_size >> 16) & 0xff); + l_in_size = (unsigned) (*srcsize); + l_out_size = stream.total_out; /* compressed size */ + tgt[3] = (char)(l_out_size & 0xff); + tgt[4] = (char)((l_out_size >> 8) & 0xff); + tgt[5] = (char)((l_out_size >> 16) & 0xff); - tgt[6] = (char)(in_size & 0xff); /* decompressed size */ - tgt[7] = (char)((in_size >> 8) & 0xff); - tgt[8] = (char)((in_size >> 16) & 0xff); + tgt[6] = (char)(l_in_size & 0xff); /* decompressed size */ + tgt[7] = (char)((l_in_size >> 8) & 0xff); + tgt[8] = (char)((l_in_size >> 16) & 0xff); *irep = stream.total_out + HDRSIZE; return; diff --git a/core/zip/inc/ZIP.h b/core/zip/inc/ZIP.h index b644341665afb..41f46184eccf5 100644 --- a/core/zip/inc/ZIP.h +++ b/core/zip/inc/ZIP.h @@ -110,7 +110,7 @@ unsigned R__bi_reverse OF((unsigned value, int length)); void R__bi_windup OF((void)); void R__copy_block OF((char far *buf, unsigned len, int header)); int R__seekable OF((void)); -extern int (*R__read_buf) OF((char *buf, unsigned size)); +extern __thread int (*R__read_buf) OF((char *buf, unsigned size)); ulg R__memcompress OF((char *tgt, ulg tgtsize, char *src, ulg srcsize)); void R__error OF((char *h)); From 6517b5e0016f0b20e55ef447283fb257677d7853 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 1 Feb 2014 16:53:10 +0100 Subject: [PATCH 08/38] Protect access to TROOT::GetListOfFiles() When new files are opened or closed, gROOT->GetListOfFiles() is updated. If the opening/closing happens on different threads then we need to serialize all access to that list. This changes uses gROOTMutex to serialize those accesses. --- io/io/src/TDirectoryFile.cxx | 1 + io/io/src/TFile.cxx | 2 ++ tree/tree/src/TBranch.cxx | 14 ++++++++++---- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/io/io/src/TDirectoryFile.cxx b/io/io/src/TDirectoryFile.cxx index 65042c0684510..64e17a24ff96a 100644 --- a/io/io/src/TDirectoryFile.cxx +++ b/io/io/src/TDirectoryFile.cxx @@ -424,6 +424,7 @@ TObject *TDirectoryFile::FindObjectAnyFile(const char *name) const // Scan the memory lists of all files for an object with name TFile *f; + R__LOCKGUARD2(gROOTMutex); TIter next(gROOT->GetListOfFiles()); while ((f = (TFile*)next())) { TObject *obj = f->GetList()->FindObject(name); diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 9df79f0757c8d..9c93e556c5a5d 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -4534,6 +4534,7 @@ TFile::EAsyncOpenStatus TFile::GetAsyncOpenStatus(const char* name) } // Check also the list of files open + R__LOCKGUARD2(gROOTMutex); TSeqCollection *of = gROOT->GetListOfFiles(); if (of && (of->GetSize() > 0)) { TIter nxf(of); @@ -4580,6 +4581,7 @@ const TUrl *TFile::GetEndpointUrl(const char* name) } // Check also the list of files open + R__LOCKGUARD2(gROOTMutex); TSeqCollection *of = gROOT->GetListOfFiles(); if (of && (of->GetSize() > 0)) { TIter nxf(of); diff --git a/tree/tree/src/TBranch.cxx b/tree/tree/src/TBranch.cxx index d674054411959..5f805185a6625 100644 --- a/tree/tree/src/TBranch.cxx +++ b/tree/tree/src/TBranch.cxx @@ -36,6 +36,7 @@ #include "TTree.h" #include "TTreeCache.h" #include "TTreeCacheUnzip.h" +#include "TVirtualMutex.h" #include "TVirtualPad.h" #include @@ -459,6 +460,7 @@ TBranch::~TBranch() if (fDirectory && (!fTree || fDirectory != fTree->GetDirectory())) { TString bFileName( GetRealFileName() ); + R__LOCKGUARD2(gROOTMutex); TFile* file = (TFile*)gROOT->GetListOfFiles()->FindObject(bFileName); if (file){ file->Close(); @@ -1377,10 +1379,14 @@ TFile* TBranch::GetFile(Int_t mode) if (fDirectory) return fDirectory->GetFile(); // check if a file with this name is in the list of Root files - TFile *file = (TFile*)gROOT->GetListOfFiles()->FindObject(fFileName.Data()); - if (file) { - fDirectory = file; - return file; + TFile *file = 0; + { + R__LOCKGUARD2(gROOTMutex); + file = (TFile*)gROOT->GetListOfFiles()->FindObject(fFileName.Data()); + if (file) { + fDirectory = file; + return file; + } } if (fFileName.Length() == 0) return 0; From e2251f6613a43ab4573f2975ba588bd42a547422 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 1 Feb 2014 17:23:36 +0100 Subject: [PATCH 09/38] Protected threaded access to TClass::GetStreamerInfos() The list of TStreamerInfos held by a TClass can change while processing and therefore access to it must be protected when using multiple threads. The gCINTMutex is used for this purpose since the TClass is considered part of the CINT data model. --- core/meta/src/TSchemaRuleSet.cxx | 9 +++- core/meta/src/TVirtualStreamerInfo.cxx | 10 ++++- io/io/src/TBufferFile.cxx | 58 +++++++++++++++++++------- tree/tree/src/TBranchElement.cxx | 2 + 4 files changed, 61 insertions(+), 18 deletions(-) diff --git a/core/meta/src/TSchemaRuleSet.cxx b/core/meta/src/TSchemaRuleSet.cxx index ee78797e749e0..18cb1a446b4f8 100644 --- a/core/meta/src/TSchemaRuleSet.cxx +++ b/core/meta/src/TSchemaRuleSet.cxx @@ -11,6 +11,8 @@ #include "TVirtualCollectionProxy.h" #include "TVirtualStreamerInfo.h" +#include "TVirtualMutex.h" +#include "TInterpreter.h" // For gCINTMutex #include "TStreamerElement.h" #include "TClassEdit.h" @@ -103,7 +105,12 @@ Bool_t TSchemaRuleSet::AddRule( TSchemaRule* rule, EConsistencyCheck checkConsis TObject* obj; // Check only if we have some information about the class, otherwise we have // nothing to check against - if( rule->GetTarget() && !(fClass->TestBit(TClass::kIsEmulation) && (fClass->GetStreamerInfos()==0 || fClass->GetStreamerInfos()->GetEntries()==0)) ) { + bool streamerInfosTest; + { + R__LOCKGUARD2(gCINTMutex); + streamerInfosTest = (fClass->GetStreamerInfos()==0 || fClass->GetStreamerInfos()->GetEntries()==0); + } + if( rule->GetTarget() && !(fClass->TestBit(TClass::kIsEmulation) && streamerInfosTest) ) { TObjArrayIter titer( rule->GetTarget() ); while( (obj = titer.Next()) ) { TObjString* str = (TObjString*)obj; diff --git a/core/meta/src/TVirtualStreamerInfo.cxx b/core/meta/src/TVirtualStreamerInfo.cxx index 3ebfa2807b8de..4cca5277bff86 100644 --- a/core/meta/src/TVirtualStreamerInfo.cxx +++ b/core/meta/src/TVirtualStreamerInfo.cxx @@ -19,6 +19,8 @@ #include "TROOT.h" #include "TSystem.h" #include "TClass.h" +#include "TVirtualMutex.h" +#include "TInterpreter.h" #include "TVirtualStreamerInfo.h" #include "TPluginManager.h" #include "TStreamerElement.h" @@ -92,8 +94,12 @@ TStreamerBasicType *TVirtualStreamerInfo::GetElementCounter(const char *countNam // Get pointer to a TStreamerBasicType in TClass *cl //static function - TObjArray *sinfos = cl->GetStreamerInfos(); - TVirtualStreamerInfo *info = (TVirtualStreamerInfo *)sinfos->At(cl->GetClassVersion()); + TVirtualStreamerInfo *info; + { + R__LOCKGUARD(gCINTMutex); + TObjArray *sinfos = cl->GetStreamerInfos(); + info = (TVirtualStreamerInfo *)sinfos->At(cl->GetClassVersion()); + } if (!info || !info->IsBuilt()) { // Even if the streamerInfo exist, it could still need to be 'build' diff --git a/io/io/src/TBufferFile.cxx b/io/io/src/TBufferFile.cxx index 2dded853d0d91..c7fe4ec654d7e 100644 --- a/io/io/src/TBufferFile.cxx +++ b/io/io/src/TBufferFile.cxx @@ -35,6 +35,8 @@ #include "TStreamerElement.h" #include "TSchemaRuleSet.h" #include "TStreamerInfoActions.h" +#include "TInterpreter.h" +#include "TVirtualMutex.h" #include "TArrayC.h" #if (defined(__linux) || defined(__APPLE__)) && defined(__i386__) && \ @@ -69,6 +71,18 @@ static inline ULong_t Void_Hash(const void *ptr) return TString::Hash(&ptr, sizeof(void*)); } +//______________________________________________________________________________ +static inline bool Class_Has_StreamerInfo(const TClass* cl) +{ + // Thread-safe check on StreamerInfos of a TClass + + // NOTE: we do not need a R__LOCKGUARD2 since we know the + // mutex is available since the TClass constructor will make + // it + R__LOCKGUARD(gCINTMutex); + return cl->GetStreamerInfos()->GetLast()>1; +} + //______________________________________________________________________________ TBufferFile::TBufferFile(TBuffer::EMode mode) :TBuffer(mode), @@ -2744,7 +2758,7 @@ void TBufferFile::SkipVersion(const TClass *cl) // We could have a file created using a Foreign class before // the introduction of the CheckSum. We need to check if ((!cl->IsLoaded() || cl->IsForeign()) && - cl->GetStreamerInfos()->GetLast()>1 ) { + Class_Has_StreamerInfo(cl) ) { const TList *list = ((TFile*)fParent)->GetStreamerInfoCache(); const TStreamerInfo *local = list ? (TStreamerInfo*)list->FindObject(cl->GetName()) : 0; @@ -2848,7 +2862,7 @@ Version_t TBufferFile::ReadVersion(UInt_t *startpos, UInt_t *bcnt, const TClass // We could have a file created using a Foreign class before // the introduction of the CheckSum. We need to check if ((!cl->IsLoaded() || cl->IsForeign()) && - cl->GetStreamerInfos()->GetLast()>1 ) { + Class_Has_StreamerInfo(cl) ) { const TList *list = ((TFile*)fParent)->GetStreamerInfoCache(); const TStreamerInfo *local = list ? (TStreamerInfo*)list->FindObject(cl->GetName()) : 0; @@ -2959,7 +2973,7 @@ Version_t TBufferFile::ReadVersionForMemberWise(const TClass *cl) // We could have a file created using a Foreign class before // the introduction of the CheckSum. We need to check if ((!cl->IsLoaded() || cl->IsForeign()) && - cl->GetStreamerInfos()->GetLast()>1 ) { + Class_Has_StreamerInfo(cl) ) { const TList *list = ((TFile*)fParent)->GetStreamerInfoCache(); const TStreamerInfo *local = list ? (TStreamerInfo*)list->FindObject(cl->GetName()) : 0; @@ -3609,14 +3623,20 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, Int_t versio // count is the number of bytes for this object in the buffer // - TObjArray *infos = cl->GetStreamerInfos(); - Int_t ninfos = infos->GetSize(); + TObjArray* infos; + Int_t ninfos; + { + R__LOCKGUARD(gCINTMutex); + infos = cl->GetStreamerInfos(); + ninfos = infos->GetSize(); + } if (version < -1 || version >= ninfos) { - Error("ReadBuffer1", "class: %s, attempting to access a wrong version: %d, object skipped at offset %d", - cl->GetName(), version, Length() ); - CheckByteCount(start, count, cl); - return 0; + Error("ReadBuffer1", "class: %s, attempting to access a wrong version: %d, object skipped at offset %d", + cl->GetName(), version, Length() ); + CheckByteCount(start, count, cl); + return 0; } + //--------------------------------------------------------------------------- // The ondisk class has been specified so get foreign streamer info //--------------------------------------------------------------------------- @@ -3636,6 +3656,8 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, Int_t versio //--------------------------------------------------------------------------- else { // The StreamerInfo should exist at this point. + + R__LOCKGUARD(gCINTMutex); sinfo = (TStreamerInfo*)infos->At(version); if (sinfo == 0) { // Unless the data is coming via a socket connection from with schema evolution @@ -3722,6 +3744,7 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass // Get local streamer info //--------------------------------------------------------------------------- else { + R__LOCKGUARD(gCINTMutex); // The StreamerInfo should exist at this point. TObjArray *infos = cl->GetStreamerInfos(); Int_t infocapacity = infos->Capacity(); @@ -3795,12 +3818,17 @@ Int_t TBufferFile::WriteClassBuffer(const TClass *cl, void *pointer) //build the StreamerInfo if first time for the class TStreamerInfo *sinfo = (TStreamerInfo*)const_cast(cl)->GetCurrentStreamerInfo(); if (sinfo == 0) { - const_cast(cl)->BuildRealData(pointer); - sinfo = new TStreamerInfo(const_cast(cl)); - const_cast(cl)->SetCurrentStreamerInfo(sinfo); - cl->GetStreamerInfos()->AddAtAndExpand(sinfo,cl->GetClassVersion()); - if (gDebug > 0) printf("Creating StreamerInfo for class: %s, version: %d\n",cl->GetName(),cl->GetClassVersion()); - sinfo->Build(); + //Have to be sure between the check and the taking of the lock if the current streamer has changed + R__LOCKGUARD(gCINTMutex); + sinfo = (TStreamerInfo*)const_cast(cl)->GetCurrentStreamerInfo(); + if(sinfo == 0) { + const_cast(cl)->BuildRealData(pointer); + sinfo = new TStreamerInfo(const_cast(cl)); + const_cast(cl)->SetCurrentStreamerInfo(sinfo); + cl->GetStreamerInfos()->AddAtAndExpand(sinfo,cl->GetClassVersion()); + if (gDebug > 0) printf("Creating StreamerInfo for class: %s, version: %d\n",cl->GetName(),cl->GetClassVersion()); + sinfo->Build(); + } } else if (!sinfo->IsCompiled()) { const_cast(cl)->BuildRealData(pointer); sinfo->BuildOld(); diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 28a655a3506f5..4ef2c845f6713 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -39,6 +39,7 @@ #include "TTree.h" #include "TVirtualCollectionProxy.h" #include "TVirtualCollectionIterators.h" +#include "TVirtualMutex.h" #include "TVirtualPad.h" #include "TBranchSTL.h" #include "TVirtualArray.h" @@ -1946,6 +1947,7 @@ void TBranchElement::InitInfo() // FIXME: Check that the found streamer info checksum matches our branch class checksum here. // Check to see if the class code was unloaded/reloaded // since we were created. + R__LOCKGUARD(gCINTMutex); if (fCheckSum && (cl->IsForeign() || (!cl->IsLoaded() && (fClassVersion == 1) && cl->GetStreamerInfos()->At(1) && (fCheckSum != ((TVirtualStreamerInfo*) cl->GetStreamerInfos()->At(1))->GetCheckSum())))) { // Try to compensate for a class that got unloaded on us. // Search through the streamer infos by checksum From bf04e1c21b18af9609b46156f199544364ddc4b1 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 1 Feb 2014 17:52:03 +0100 Subject: [PATCH 10/38] Thread safe caching of TStreamerInfo in TBranchSTL We now protect all threaded access to TClass::GetStreamerInfos. In addition this change properly handles updating the TStreamerInfo cache in a thread safe manner. --- tree/tree/inc/TBranchSTL.h | 11 +++++++++++ tree/tree/src/TBranchSTL.cxx | 13 ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/tree/tree/inc/TBranchSTL.h b/tree/tree/inc/TBranchSTL.h index 13e861c270b8b..4dabe440d3b97 100644 --- a/tree/tree/inc/TBranchSTL.h +++ b/tree/tree/inc/TBranchSTL.h @@ -15,6 +15,9 @@ #include "TBranchObject.h" #include "TBranchElement.h" #include "TIndArray.h" +#if __cplusplus > 199711L +#include +#endif #include #include #include @@ -72,9 +75,17 @@ class TBranchSTL: public TBranch { TIndArray fInd; //! Indices TString fContName; // Class name of referenced object TString fClassName; // Name of the parent class, if we're the data member +#if __cplusplus > 199711L + mutable std::atomic fClassVersion; // Version number of the class +#else mutable Int_t fClassVersion; // Version number of the class +#endif UInt_t fClCheckSum; // Class checksum +#if __cplusplus > 199711L + mutable std::atomic fInfo; //! The streamer info +#else mutable TStreamerInfo *fInfo; //! The streamer info +#endif char* fObject; //! Pointer to object at address or the Int_t fID; // Element serial number in the streamer info }; diff --git a/tree/tree/src/TBranchSTL.cxx b/tree/tree/src/TBranchSTL.cxx index f2eb83def66ba..40a5247fa7582 100644 --- a/tree/tree/src/TBranchSTL.cxx +++ b/tree/tree/src/TBranchSTL.cxx @@ -17,6 +17,8 @@ #include "TBasket.h" #include "TStreamerInfo.h" #include "TStreamerElement.h" +#include "TInterpreter.h" // For gCINTMutex +#include "TVirtualMutex.h" #include #include @@ -561,6 +563,9 @@ TStreamerInfo* TBranchSTL::GetInfo() const // If the checksum is there and we're dealing with the foreign class //------------------------------------------------------------------------ if( fClCheckSum && cl->IsForeign() ) { + // NOTE: We do not need a R__LOCKGUARD2 since the TClass constructor + // is guaranteed to set the gCINTMutex if it is not already available. + R__LOCKGUARD(gCINTMutex); //--------------------------------------------------------------------- // Loop over the infos //--------------------------------------------------------------------- @@ -579,8 +584,10 @@ TStreamerInfo* TBranchSTL::GetInfo() const } } } - fInfo->SetBit(TVirtualStreamerInfo::kCannotOptimize); - fInfo->BuildOld(); + if((*fInfo).IsOptimized()) { + (*fInfo).SetBit(TVirtualStreamerInfo::kCannotOptimize); + (*fInfo).BuildOld(); + } } return fInfo; } @@ -672,7 +679,7 @@ void TBranchSTL::SetAddress( void* addr ) // Get the appropriate streamer element //------------------------------------------------------------------------ GetInfo(); - TStreamerElement *el = (TStreamerElement*)fInfo->GetElements()->At( fID ); + TStreamerElement *el = (TStreamerElement*)(*fInfo).GetElements()->At( fID ); //------------------------------------------------------------------------ // Set up the addresses From a5752afb104c03c9bdc15a8bd86a2c25d7d4beef Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 1 Feb 2014 18:21:36 +0100 Subject: [PATCH 11/38] Only set TStreamerInfo to unoptimized if it is optimized When running with multiple threads, we need to avoid unnecessary changes to TStreamerInfos. Therefore we check to see if the TStreamerInfo is optimized before setting it as unoptimized and rebuild it. --- tree/tree/src/TBranchElement.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 4ef2c845f6713..6e8207a631176 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -5477,7 +5477,7 @@ Int_t TBranchElement::Unroll(const char* name, TClass* clParent, TClass* cl, cha // independently in the tree. // TStreamerInfo* sinfo = fTree->BuildStreamerInfo(cl); - if (sinfo && splitlevel > 0) { + if (sinfo && splitlevel > 0 && (!sinfo->IsCompiled() || sinfo->IsOptimized()) ) { sinfo->SetBit(TVirtualStreamerInfo::kCannotOptimize); sinfo->Compile(); } From 0f9bbe8c2b2a458923110519b847e11dbd71a447 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 1 Feb 2014 18:35:35 +0100 Subject: [PATCH 12/38] Fix threading issues in TClass The following threading issues were corrected - TROOT::GetListOfClasses was protected via gCINTMutex - Access to TStreamerInfo lists were protected via gCINTMutex - Setting of the cache for current TStreamerInfo and for the list of conversion TStreamerInfos is protected via std::atomic<> --- core/meta/inc/TClass.h | 15 +++++++++-- core/meta/src/TClass.cxx | 57 +++++++++++++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/core/meta/inc/TClass.h b/core/meta/inc/TClass.h index 93d5066fe0f1b..3a3b3c3645e34 100644 --- a/core/meta/inc/TClass.h +++ b/core/meta/inc/TClass.h @@ -35,7 +35,9 @@ #endif #include #include - +#if __cplusplus > 199711L +#include +#endif class TBaseClass; class TBrowser; class TDataMember; @@ -87,7 +89,11 @@ friend class ROOT::TGenericClassInfo; private: mutable TObjArray *fStreamerInfo; //Array of TVirtualStreamerInfo +#if __cplusplus > 199711L + mutable std::atomic*> fConversionStreamerInfo; //Array of the streamer infos derived from another class. +#else mutable std::map *fConversionStreamerInfo; //Array of the streamer infos derived from another class. +#endif TList *fRealData; //linked list for persistent members including base classes TList *fBase; //linked list for base classes TList *fData; //linked list for data members @@ -135,7 +141,11 @@ friend class ROOT::TGenericClassInfo; mutable Bool_t fIsOffsetStreamerSet; //!saved remember if fOffsetStreamer has been set. mutable Long_t fOffsetStreamer; //!saved info to call Streamer Int_t fStreamerType; //!cached of the streaming method to use +#if __cplusplus > 199711L + mutable std::atomic fCurrentInfo; //!cached current streamer info. +#else mutable TVirtualStreamerInfo *fCurrentInfo; //!cached current streamer info. +#endif TClassRef *fRefStart; //!List of references to this object TVirtualRefProxy *fRefProxy; //!Pointer to reference proxy if this class represents a reference ROOT::TSchemaRuleSet *fSchemaRules; //! Schema evolution rules @@ -155,6 +165,7 @@ friend class ROOT::TGenericClassInfo; void SetClassVersion(Version_t version); void SetClassSize(Int_t sizof) { fSizeof = sizof; } + TVirtualStreamerInfo* DetermineCurrentStreamerInfo(); // Various implementation for TClass::Stramer void StreamerExternal(void *object, TBuffer &b, const TClass *onfile_class) const; @@ -266,7 +277,7 @@ friend class ROOT::TGenericClassInfo; const char *GetContextMenuTitle() const { return fContextMenuTitle; } TVirtualStreamerInfo *GetCurrentStreamerInfo() { if (fCurrentInfo) return fCurrentInfo; - else return (fCurrentInfo=(TVirtualStreamerInfo*)(fStreamerInfo->At(fClassVersion))); + else return DetermineCurrentStreamerInfo(); } TList *GetListOfDataMembers(); TList *GetListOfBases(); diff --git a/core/meta/src/TClass.cxx b/core/meta/src/TClass.cxx index 8f7d42b3e8bcc..27c99d4b6f7e9 100644 --- a/core/meta/src/TClass.cxx +++ b/core/meta/src/TClass.cxx @@ -264,6 +264,8 @@ void TClass::AddClass(TClass *cl) // static: Add a class to the list and map of classes. if (!cl) return; + + R__LOCKGUARD2(gCINTMutex); gROOT->GetListOfClasses()->Add(cl); if (cl->GetTypeInfo()) { GetIdMap()->Add(cl->GetTypeInfo()->name(),cl); @@ -277,6 +279,8 @@ void TClass::RemoveClass(TClass *oldcl) // static: Remove a class from the list and map of classes if (!oldcl) return; + + R__LOCKGUARD2(gCINTMutex); gROOT->GetListOfClasses()->Remove(oldcl); if (oldcl->GetTypeInfo()) { GetIdMap()->Remove(oldcl->GetTypeInfo()->name()); @@ -1116,7 +1120,7 @@ void TClass::Init(const char *name, Version_t cversion, TClass::TClass(const TClass& cl) : TDictionary(cl), fStreamerInfo(cl.fStreamerInfo), - fConversionStreamerInfo(cl.fConversionStreamerInfo), + fConversionStreamerInfo(0), fRealData(cl.fRealData), fBase(cl.fBase), fData(cl.fData), @@ -1159,11 +1163,11 @@ TClass::TClass(const TClass& cl) : fIsOffsetStreamerSet(cl.fIsOffsetStreamerSet), fOffsetStreamer(cl.fOffsetStreamer), fStreamerType(cl.fStreamerType), - fCurrentInfo(cl.fCurrentInfo), + fCurrentInfo(0), fRefStart(cl.fRefStart), fRefProxy(cl.fRefProxy), fSchemaRules(cl.fSchemaRules), - fStreamerImpl(cl.fStreamerImpl) + fStreamerImpl(0) { //copy constructor @@ -1274,11 +1278,15 @@ TClass::~TClass() delete fSchemaRules; if (fConversionStreamerInfo) { std::map::iterator it; - std::map::iterator end = fConversionStreamerInfo->end(); - for( it = fConversionStreamerInfo->begin(); it != end; ++it ) { + std::map::iterator end = (*fConversionStreamerInfo).end(); + for( it = (*fConversionStreamerInfo).begin(); it != end; ++it ) { delete it->second; } +#if __cplusplus > 199711L + delete fConversionStreamerInfo.load(); +#else delete fConversionStreamerInfo; +#endif } } @@ -2035,6 +2043,9 @@ TObject *TClass::Clone(const char *new_name) const Error("Clone","The name of the class must be changed when cloning a TClass object."); return 0; } + + // Need to lock access to TROOT::GetListOfClasses so the cloning happens atomically + R__LOCKGUARD2(gCINTMutex); // Temporarily remove the original from the list of classes. TClass::RemoveClass(const_cast(this)); @@ -2677,6 +2688,9 @@ TClass *TClass::GetClass(const type_info& typeinfo, Bool_t load, Bool_t /* silen { // Return pointer to class with name. + //protect access to TROOT::GetListOfClasses + R__LOCKGUARD2(gCINTMutex); + if (!gROOT->GetListOfClasses()) return 0; //printf("TClass::GetClass called, typeinfo.name=%s\n",typeinfo.name()); @@ -3278,8 +3292,8 @@ void TClass::ls(Option_t *options) const if (fConversionStreamerInfo) { std::map::iterator it; - std::map::iterator end = fConversionStreamerInfo->end(); - for( it = fConversionStreamerInfo->begin(); it != end; ++it ) { + std::map::iterator end = (*fConversionStreamerInfo).end(); + for( it = (*fConversionStreamerInfo).begin(); it != end; ++it ) { it->second->ls(options); } } @@ -4469,6 +4483,18 @@ void TClass::SetClassVersion(Version_t version) fCurrentInfo = 0; } +//______________________________________________________________________________ +TVirtualStreamerInfo* TClass::DetermineCurrentStreamerInfo() +{ + // Determine and set pointer to current TVirtualStreamerInfo + + R__LOCKGUARD2(gCINTMutex); + if(!fCurrentInfo) { + fCurrentInfo=(TVirtualStreamerInfo*)(fStreamerInfo->At(fClassVersion)); + } + return fCurrentInfo; +} + //______________________________________________________________________________ void TClass::SetCurrentStreamerInfo(TVirtualStreamerInfo *info) { @@ -4628,6 +4654,8 @@ void TClass::PostLoadCheck() } else if (IsLoaded() && fClassInfo && fStreamerInfo && (!IsForeign()||fClassVersion>1) ) { + R__LOCKGUARD(gCINTMutex); + TVirtualStreamerInfo *info = (TVirtualStreamerInfo*)(fStreamerInfo->At(fClassVersion)); // Here we need to check whether this TVirtualStreamerInfo (which presumably has been // loaded from a file) is consistent with the definition in the library we just loaded. @@ -5384,6 +5412,7 @@ TVirtualStreamerInfo *TClass::FindStreamerInfo(UInt_t checksum) const { // Find the TVirtualStreamerInfo in the StreamerInfos corresponding to checksum + R__LOCKGUARD(gCINTMutex); Int_t ninfos = fStreamerInfo->GetEntriesFast()-1; for (Int_t i=-1;iGetEntriesFast()-1; for (Int_t i=-1;i::iterator it; + R__LOCKGUARD(gCINTMutex); - it = fConversionStreamerInfo->find( cl->GetName() ); + it = (*fConversionStreamerInfo).find( cl->GetName() ); - if( it != fConversionStreamerInfo->end() ) { + if( it != (*fConversionStreamerInfo).end() ) { arr = it->second; } @@ -5542,10 +5572,11 @@ TVirtualStreamerInfo *TClass::FindConversionStreamerInfo( const TClass* cl, UInt TVirtualStreamerInfo* info = 0; if (fConversionStreamerInfo) { std::map::iterator it; + R__LOCKGUARD(gCINTMutex); + + it = (*fConversionStreamerInfo).find( cl->GetName() ); - it = fConversionStreamerInfo->find( cl->GetName() ); - - if( it != fConversionStreamerInfo->end() ) { + if( it != (*fConversionStreamerInfo).end() ) { arr = it->second; } if (arr) { From bb39bbcc0d5f7d5b43d41f8bd9f47b666decfc23 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 22:49:10 +0100 Subject: [PATCH 13/38] Made TPluginManager thread-safe for I/O Several changes were needed to make this thread-safe for the I/O case 1) Protect the list of handlers with its own Mutex 2) Use a thread_local variable to hold the status of reading a directory 3) Use gCINTMutex to protect access to fBasesLoaded. Had to use gCINTMutex since we need threads trying to read the same dirs to wait for the first reader to finish so the lock has to be held over the whole call. In addition, gCINTMutex is sometimes taken before and sometimes during the call so using a different mutex would lead to a deadlock situation. NOTE: fReadingDirs is not used anymore but is still in the code to allow binary compatibility to easy testing. This should be removed in the future. --- core/base/src/TPluginManager.cxx | 35 ++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/core/base/src/TPluginManager.cxx b/core/base/src/TPluginManager.cxx index 8ff58da8a6c6d..f218e7abccde6 100644 --- a/core/base/src/TPluginManager.cxx +++ b/core/base/src/TPluginManager.cxx @@ -103,6 +103,8 @@ TPluginManager *gPluginMgr; // main plugin manager created in TROOT +static TVirtualMutex *gPluginManagerMutex; +static thread_local bool fgReadingDirs = false; ClassImp(TPluginHandler) @@ -470,6 +472,7 @@ void TPluginManager::LoadHandlersFromPluginDirs(const char *base) // dependency, check on some OS or ROOT capability or downloading // of the plugin. + R__LOCKGUARD2(gCINTMutex); if (!fBasesLoaded) { fBasesLoaded = new THashTable(); fBasesLoaded->SetOwner(); @@ -485,7 +488,7 @@ void TPluginManager::LoadHandlersFromPluginDirs(const char *base) fBasesLoaded->Add(new TObjString(sbase)); } - fReadingDirs = kTRUE; + fgReadingDirs = kTRUE; TString plugindirs = gEnv->GetValue("Root.PluginPath", (char*)0); #ifdef WIN32 @@ -530,7 +533,7 @@ void TPluginManager::LoadHandlersFromPluginDirs(const char *base) } delete dirs; - fReadingDirs = kFALSE; + fgReadingDirs = kFALSE; } //______________________________________________________________________________ @@ -541,20 +544,25 @@ void TPluginManager::AddHandler(const char *base, const char *regexp, // Add plugin handler to the list of handlers. If there is already a // handler defined for the same base and regexp it will be replaced. - if (!fHandlers) { - fHandlers = new TList; - fHandlers->SetOwner(); + { + R__LOCKGUARD2(gPluginManagerMutex); + if (!fHandlers) { + fHandlers = new TList; + fHandlers->SetOwner(); + } } - // make sure there is no previous handler for the same case RemoveHandler(base, regexp); - if (fReadingDirs) + if (fgReadingDirs) origin = gInterpreter->GetCurrentMacroName(); TPluginHandler *h = new TPluginHandler(base, regexp, className, pluginName, ctor, origin); - fHandlers->Add(h); + { + R__LOCKGUARD2(gPluginManagerMutex); + fHandlers->Add(h); + } } //______________________________________________________________________________ @@ -562,7 +570,7 @@ void TPluginManager::RemoveHandler(const char *base, const char *regexp) { // Remove handler for the specified base class and the specified // regexp. If regexp=0 remove all handlers for the specified base. - + R__LOCKGUARD2(gPluginManagerMutex); if (!fHandlers) return; TIter next(fHandlers); @@ -570,10 +578,10 @@ void TPluginManager::RemoveHandler(const char *base, const char *regexp) while ((h = (TPluginHandler*) next())) { if (h->fBase == base) { - if (!regexp || h->fRegexp == regexp) { - fHandlers->Remove(h); - delete h; - } + if (!regexp || h->fRegexp == regexp) { + fHandlers->Remove(h); + delete h; + } } } } @@ -587,6 +595,7 @@ TPluginHandler *TPluginManager::FindHandler(const char *base, const char *uri) LoadHandlersFromPluginDirs(base); + R__LOCKGUARD2(gPluginManagerMutex); TIter next(fHandlers); TPluginHandler *h; From c417108b50e58a04ebb88fc3e7c5ddd40bdf5476 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 22:55:31 +0100 Subject: [PATCH 14/38] Mutex lock removed from TStorage::ObjectDealloc to avoid deadlocks The order of locks between gGlobalMutex and gCINTMutex is undefined and TStorage::ObjectDealloc calls were involved in order reversal problems. As it turned out, there is no data structure that needs protecting in TStorage::ObjectDealloc so we do not need the lock. --- core/base/src/TStorage.cxx | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/base/src/TStorage.cxx b/core/base/src/TStorage.cxx index 7e482380ef811..5207b49d7ff3b 100644 --- a/core/base/src/TStorage.cxx +++ b/core/base/src/TStorage.cxx @@ -344,9 +344,6 @@ void TStorage::ObjectDealloc(void *vp) { // Used to deallocate a TObject on the heap (via TObject::operator delete()). - // Needs to be protected by global mutex - R__LOCKGUARD(gGlobalMutex); - #ifndef NOCINT // to handle delete with placement called via CINT Long_t gvp = 0; From c9698f7cb7fab444dfc46209e240e76374e3b7e5 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 22:58:25 +0100 Subject: [PATCH 15/38] Use thread_local for statics in TUUID TUUID used statics that were causing race conditions. The fix was to change the statics to thread_local. --- core/base/src/TUUID.cxx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/base/src/TUUID.cxx b/core/base/src/TUUID.cxx index b5b7fade79c54..4e7c72a13d91e 100644 --- a/core/base/src/TUUID.cxx +++ b/core/base/src/TUUID.cxx @@ -111,6 +111,7 @@ #include "TInetAddress.h" #include "TMD5.h" #include "Bytes.h" +#include "TVirtualMutex.h" #include #include #ifdef R__WIN32 @@ -124,6 +125,7 @@ #endif #endif +#include ClassImp(TUUID) @@ -132,9 +134,9 @@ TUUID::TUUID() { // Create a UUID. - static uuid_time_t time_last; - static UShort_t clockseq; - static Bool_t firstTime = kTRUE; + static thread_local uuid_time_t time_last; + static thread_local UShort_t clockseq; + static thread_local Bool_t firstTime = kTRUE; if (firstTime) { if (gSystem) { // try to get a unique seed per process @@ -321,9 +323,9 @@ void TUUID::GetCurrentTime(uuid_time_t *timestamp) const UShort_t uuids_per_tick = 1024; - static uuid_time_t time_last; - static UShort_t uuids_this_tick; - static Bool_t init = kFALSE; + static thread_local uuid_time_t time_last; + static thread_local UShort_t uuids_this_tick; + static thread_local Bool_t init = kFALSE; if (!init) { GetSystemTime(&time_last); From ed8dba6413746bcf61f20e16610af98f7236a979 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:04:09 +0100 Subject: [PATCH 16/38] Fix thread-safety issues with TError -There was a potential deadlock situation between gErrorMutex and gCINTMutex. The fix was to limit the scope of gErrorMutex locks -Changed buffers from static to static thread_local. This allowed gErrorMutex to be more limited in scope. --- core/base/src/TError.cxx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/core/base/src/TError.cxx b/core/base/src/TError.cxx index f78e1d48a8515..ba374430c8be9 100644 --- a/core/base/src/TError.cxx +++ b/core/base/src/TError.cxx @@ -62,10 +62,8 @@ static void DebugPrint(const char *fmt, ...) { // Print debugging message to stderr and, on Windows, to the system debugger. - static Int_t buf_size = 2048; - static char *buf = 0; - - R__LOCKGUARD2(gErrorMutex); + static thread_local Int_t buf_size = 2048; + static thread_local char *buf = 0; va_list ap; va_start(ap, fmt); @@ -90,6 +88,8 @@ static void DebugPrint(const char *fmt, ...) } va_end(ap); + R__LOCKGUARD2(gErrorMutex); + fprintf(stderr, "%s", buf); #ifdef WIN32 @@ -197,10 +197,9 @@ void ErrorHandler(Int_t level, const char *location, const char *fmt, va_list ap { // General error handler function. It calls the user set error handler. - R__LOCKGUARD2(gErrorMutex); - static Int_t buf_size = 2048; - static char *buf = 0; + static thread_local Int_t buf_size = 2048; + static thread_local char *buf = 0; int vc = 0; va_list sap; @@ -233,9 +232,10 @@ void ErrorHandler(Int_t level, const char *location, const char *fmt, va_list ap va_end(ap); char *bp; - if (level >= kSysError && level < kFatal) + if (level >= kSysError && level < kFatal) { + R__LOCKGUARD2(gErrorMutex); bp = Form("%s (%s)", buf, gSystem->GetError()); - else + } else bp = buf; if (level != kFatal) From 8bd0dcb1ea45097dd632c35498e981c1c1d49a9d Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:06:34 +0100 Subject: [PATCH 17/38] Use gCINTMutex to protect access to G__getgvp This fixed a problem found by helgrind. --- core/meta/src/TCint.cxx | 1 + 1 file changed, 1 insertion(+) diff --git a/core/meta/src/TCint.cxx b/core/meta/src/TCint.cxx index 0e33edf7316d3..cf804e71dd9e5 100644 --- a/core/meta/src/TCint.cxx +++ b/core/meta/src/TCint.cxx @@ -2470,6 +2470,7 @@ Long_t TCint::GetExecByteCode() const Long_t TCint::Getgvp() const { // Interface to CINT function + R__LOCKGUARD(gCINTMutex); return (Long_t)G__getgvp(); } From ae1d5e8e74337fe49b2ec65e12e1f270341518d2 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:12:33 +0100 Subject: [PATCH 18/38] Use gCINTMutex to protect access to TBaseClass::Property Helgrind found a race condition involving TClass::GetBaseClassOffsetRecurse calling TBaseClass::Property. --- core/meta/src/TClass.cxx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/meta/src/TClass.cxx b/core/meta/src/TClass.cxx index 27c99d4b6f7e9..9e5ce76f07c44 100644 --- a/core/meta/src/TClass.cxx +++ b/core/meta/src/TClass.cxx @@ -2354,8 +2354,11 @@ Int_t TClass::GetBaseClassOffsetRecurse(const TClass *cl) c = inh->GetClassPointer(kTRUE); // kFALSE); if (c) { if (cl == c) { - if ((inh->Property() & G__BIT_ISVIRTUALBASE) != 0) - return -2; + { + R__LOCKGUARD(gCINTMutex); + if ((inh->Property() & G__BIT_ISVIRTUALBASE) != 0) + return -2; + } return inh->GetDelta(); } off = c->GetBaseClassOffsetRecurse(cl); From 6f5e0a0bb0e321fd2b3c7ad41d5e4342a2257d4a Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:14:26 +0100 Subject: [PATCH 19/38] Fix thread-safety problems with cleanup of containers Both TList and TObjArray modify gROOT::GetListOfCleanups() as part of their cleanup methods. This change uses gROOTMutex to protect that list. --- core/cont/src/TList.cxx | 35 +++++++++++++++++++++++++++++------ core/cont/src/TObjArray.cxx | 17 ++++++++++++++--- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/core/cont/src/TList.cxx b/core/cont/src/TList.cxx index 344d61678d622..99fa1a323fa72 100644 --- a/core/cont/src/TList.cxx +++ b/core/cont/src/TList.cxx @@ -69,6 +69,7 @@ #include "TList.h" #include "TClass.h" #include "TROOT.h" +#include "TVirtualMutex.h" #include namespace std {} using namespace std; @@ -360,8 +361,15 @@ void TList::Clear(Option_t *option) // list (of Primitives of the canvas) that was connecting it // (indirectly) to the list of cleanups. // So let's temporarily add the current list and remove it later. - bool needRegister = fFirst && TROOT::Initialized() && !gROOT->GetListOfCleanups()->FindObject(this); - if (needRegister) gROOT->GetListOfCleanups()->Add(this); + bool needRegister = fFirst && TROOT::Initialized(); + if(needRegister) { + R__LOCKGUARD2(gROOTMutex); + needRegister = needRegister && !gROOT->GetListOfCleanups()->FindObject(this); + } + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + gROOT->GetListOfCleanups()->Add(this); + } while (fFirst) { TObjLink *tlk = fFirst; fFirst = fFirst->Next(); @@ -376,7 +384,10 @@ void TList::Clear(Option_t *option) } delete tlk; } - if (needRegister) ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + } fFirst = fLast = fCache = 0; fSize = 0; Changed(); @@ -405,8 +416,15 @@ void TList::Delete(Option_t *option) // list (of Primitives of the canvas) that was connecting it // (indirectly) to the list of cleanups. // So let's temporarily add the current list and remove it later. - bool needRegister = fFirst && TROOT::Initialized() && !gROOT->GetListOfCleanups()->FindObject(this); - if (needRegister) gROOT->GetListOfCleanups()->Add(this); + bool needRegister = fFirst && TROOT::Initialized(); + if(needRegister) { + R__LOCKGUARD2(gROOTMutex); + needRegister = needRegister && !gROOT->GetListOfCleanups()->FindObject(this); + } + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + gROOT->GetListOfCleanups()->Add(this); + } while (fFirst) { TObjLink *tlk = fFirst; fFirst = fFirst->Next(); @@ -419,7 +437,12 @@ void TList::Delete(Option_t *option) delete tlk; } - if (needRegister) ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + } + fFirst = fLast = fCache = 0; fSize = 0; diff --git a/core/cont/src/TObjArray.cxx b/core/cont/src/TObjArray.cxx index 0336f5970e29b..878907602a527 100644 --- a/core/cont/src/TObjArray.cxx +++ b/core/cont/src/TObjArray.cxx @@ -53,6 +53,7 @@ #include "TObjArray.h" #include "TError.h" #include "TROOT.h" +#include "TVirtualMutex.h" #include ClassImp(TObjArray) @@ -338,15 +339,25 @@ void TObjArray::Delete(Option_t *) // list (of Primitives of the canvas) that was connecting it // (indirectly) to the list of cleanups. // So let's temporarily add the current list and remove it later. - bool needRegister = fSize && TROOT::Initialized() && !gROOT->GetListOfCleanups()->FindObject(this); - if (needRegister) gROOT->GetListOfCleanups()->Add(this); + bool needRegister = fSize && TROOT::Initialized(); + if(needRegister) { + R__LOCKGUARD2(gROOTMutex); + needRegister = needRegister && !gROOT->GetListOfCleanups()->FindObject(this); + } + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + gROOT->GetListOfCleanups()->Add(this); + } for (Int_t i = 0; i < fSize; i++) { if (fCont[i] && fCont[i]->IsOnHeap()) { TCollection::GarbageCollect(fCont[i]); fCont[i] = 0; } } - if (needRegister) ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + if (needRegister) { + R__LOCKGUARD2(gROOTMutex); + ROOT::GetROOT()->GetListOfCleanups()->Remove(this); + } Init(fSize, fLowerBound); } From 545c70e2149d484c393af8be7caba98307351157 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:16:55 +0100 Subject: [PATCH 20/38] Fix thread-safety issues of StreamerInfos -Made TStreamerInfo::fgElement thread_local -Added an additional test bit to avoid rerunning TStreamerInfo::BuildOld. This avoids many cases of changing a TStreamerInfo once it is already in use. --- core/meta/inc/TVirtualStreamerInfo.h | 3 ++- io/io/inc/TStreamerInfo.h | 7 ++++++- io/io/src/TStreamerInfo.cxx | 6 ++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/core/meta/inc/TVirtualStreamerInfo.h b/core/meta/inc/TVirtualStreamerInfo.h index 569d7e667ffd6..4d2add6bd81e3 100644 --- a/core/meta/inc/TVirtualStreamerInfo.h +++ b/core/meta/inc/TVirtualStreamerInfo.h @@ -60,7 +60,8 @@ class TVirtualStreamerInfo : public TNamed { kIgnoreTObjectStreamer = BIT(13), // eventhough BIT(13) is taken up by TObject (to preserverse forward compatibility) kRecovered = BIT(14), kNeedCheck = BIT(15), - kIsCompiled = BIT(16) + kIsCompiled = BIT(16), + kBuildOldUsed = BIT(17) }; enum EReadWrite { diff --git a/io/io/inc/TStreamerInfo.h b/io/io/inc/TStreamerInfo.h index 044538ba77ffc..600c6c4895f4d 100644 --- a/io/io/inc/TStreamerInfo.h +++ b/io/io/inc/TStreamerInfo.h @@ -112,7 +112,11 @@ class TStreamerInfo : public TVirtualStreamerInfo { TStreamerInfoActions::TActionSequence *fWriteMemberWise; //! List of write action resulting from the compilation for use in member wise streaming. static Int_t fgCount; //Number of TStreamerInfo instances +#if __cplusplus > 199711L + static thread_local TStreamerElement *fgElement; //Pointer to current TStreamerElement +#else static TStreamerElement *fgElement; //Pointer to current TStreamerElement +#endif template static T GetTypedValueAux(Int_t type, void *ladd, int k, Int_t len); static void PrintValueAux(char *ladd, Int_t atype, TStreamerElement * aElement, Int_t aleng, Int_t *count); @@ -133,7 +137,8 @@ class TStreamerInfo : public TVirtualStreamerInfo { kIgnoreTObjectStreamer = BIT(13), // eventhough BIT(13) is taken up by TObject (to preserverse forward compatibility) kRecovered = BIT(14), kNeedCheck = BIT(15), - kIsCompiled = BIT(16) + kIsCompiled = BIT(16), + kBuildOldUsed = BIT(17) }; enum EReadWrite { diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index d90c87142b9d8..16939e88a1b57 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -72,7 +72,11 @@ #include "TStreamerInfoActions.h" +#if __cplusplus > 199711L +thread_local TStreamerElement *TStreamerInfo::fgElement = 0; +#else TStreamerElement *TStreamerInfo::fgElement = 0; +#endif Int_t TStreamerInfo::fgCount = 0; const Int_t kMaxLen = 1024; @@ -1270,6 +1274,8 @@ void TStreamerInfo::BuildOld() // rebuild the TStreamerInfo structure R__LOCKGUARD(gCINTMutex); + if( TestBit(kBuildOldUsed) && !IsOptimized() ) return; + SetBit(kBuildOldUsed); if (gDebug > 0) { printf("\n====>Rebuilding TStreamerInfo for class: %s, version: %d\n", GetName(), fClassVersion); From ecbe8cc2344ac7f51c9235f104596958a8590a01 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 3 Feb 2014 23:25:11 +0100 Subject: [PATCH 21/38] Made gTree thread_local --- hist/hbook/src/THbookFile.cxx | 2 +- tree/tree/src/TBranch.cxx | 2 +- tree/tree/src/TBranchBrowsable.cxx | 2 +- tree/tree/src/TBranchClones.cxx | 2 +- tree/tree/src/TFriendElement.cxx | 2 +- tree/tree/src/TLeaf.cxx | 2 +- tree/tree/src/TTree.cxx | 2 +- tree/treeplayer/src/TTreeFormula.cxx | 2 +- tree/treeplayer/src/TTreePlayer.cxx | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/hist/hbook/src/THbookFile.cxx b/hist/hbook/src/THbookFile.cxx index bafecd94aa2ce..89ee64eb6f4f7 100644 --- a/hist/hbook/src/THbookFile.cxx +++ b/hist/hbook/src/THbookFile.cxx @@ -236,7 +236,7 @@ extern "C" void type_of_call hldir(DEFCHAR,DEFCHAR); Bool_t THbookFile::fgPawInit = kFALSE; Int_t *THbookFile::fgLuns = 0; -R__EXTERN TTree *gTree; +R__EXTERN thread_local TTree *gTree; ClassImp(THbookFile) diff --git a/tree/tree/src/TBranch.cxx b/tree/tree/src/TBranch.cxx index 5f805185a6625..06042163a772c 100644 --- a/tree/tree/src/TBranch.cxx +++ b/tree/tree/src/TBranch.cxx @@ -43,7 +43,7 @@ #include #include -R__EXTERN TTree* gTree; +R__EXTERN thread_local TTree* gTree; Int_t TBranch::fgCount = 0; diff --git a/tree/tree/src/TBranchBrowsable.cxx b/tree/tree/src/TBranchBrowsable.cxx index 12f7b2d8bbea2..eecc9f0651f4b 100644 --- a/tree/tree/src/TBranchBrowsable.cxx +++ b/tree/tree/src/TBranchBrowsable.cxx @@ -27,7 +27,7 @@ #include "TRef.h" #include -R__EXTERN TTree *gTree; +R__EXTERN thread_local TTree *gTree; ClassImp(TVirtualBranchBrowsable); diff --git a/tree/tree/src/TBranchClones.cxx b/tree/tree/src/TBranchClones.cxx index c3a7721f1334c..035caba55ad00 100644 --- a/tree/tree/src/TBranchClones.cxx +++ b/tree/tree/src/TBranchClones.cxx @@ -31,7 +31,7 @@ #include -R__EXTERN TTree* gTree; +R__EXTERN thread_local TTree* gTree; ClassImp(TBranchClones) diff --git a/tree/tree/src/TFriendElement.cxx b/tree/tree/src/TFriendElement.cxx index 7e05599abfed8..2fc96cbd85fff 100644 --- a/tree/tree/src/TFriendElement.cxx +++ b/tree/tree/src/TFriendElement.cxx @@ -30,7 +30,7 @@ #include "TFile.h" #include "TROOT.h" -R__EXTERN TTree *gTree; +R__EXTERN thread_local TTree *gTree; ClassImp(TFriendElement) diff --git a/tree/tree/src/TLeaf.cxx b/tree/tree/src/TLeaf.cxx index 56fe73735701b..eadaad35e7888 100644 --- a/tree/tree/src/TLeaf.cxx +++ b/tree/tree/src/TLeaf.cxx @@ -24,7 +24,7 @@ #include -R__EXTERN TTree* gTree; +R__EXTERN thread_local TTree* gTree; ClassImp(TLeaf) diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 9cbde8ace55b9..3d5c542ca4fc0 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -376,7 +376,7 @@ Int_t TTree::fgBranchStyle = 1; // Use new TBranch style with TBranchElement. Long64_t TTree::fgMaxTreeSize = 100000000000LL; -TTree* gTree; +thread_local TTree* gTree; ClassImp(TTree) diff --git a/tree/treeplayer/src/TTreeFormula.cxx b/tree/treeplayer/src/TTreeFormula.cxx index 8b59c50405da0..4cf88404e7227 100644 --- a/tree/treeplayer/src/TTreeFormula.cxx +++ b/tree/treeplayer/src/TTreeFormula.cxx @@ -54,7 +54,7 @@ #include const Int_t kMaxLen = 1024; -R__EXTERN TTree *gTree; +R__EXTERN thread_local TTree *gTree; ClassImp(TTreeFormula) diff --git a/tree/treeplayer/src/TTreePlayer.cxx b/tree/treeplayer/src/TTreePlayer.cxx index 83e23aa049c00..0076b20fde491 100644 --- a/tree/treeplayer/src/TTreePlayer.cxx +++ b/tree/treeplayer/src/TTreePlayer.cxx @@ -82,7 +82,7 @@ #include "Math/MinimizerOptions.h" R__EXTERN Foption_t Foption; -R__EXTERN TTree *gTree; +R__EXTERN thread_local TTree *gTree; TVirtualFitter *tFitter=0; From 551a92c7f57b762878c9b1abad13943adce128fe Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 02:54:33 +0100 Subject: [PATCH 22/38] Properly avoid repeated calls to TStreamerInfo::Build* -Internal cloning of TStreamerInfo resets kBuildOldUsed so BuildOld will run -BuildOld calls itself recursively so we only set kBuildOldUsed once we leave the function -Avoid repeated calls to Build These changes are needed to avoid having multiple threads do the same work on the same TStreamerInfos which can cause interference between threads. --- io/io/src/TStreamerInfo.cxx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index 16939e88a1b57..43d9ea7f15b54 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -213,6 +213,8 @@ void TStreamerInfo::Build() // one by one the list of data members of the analyzed class. R__LOCKGUARD(gCINTMutex); + // Did another thread already do the work? + if (fIsBuilt) return; // This is used to avoid unwanted recursive call to Build fIsBuilt = kTRUE; @@ -508,6 +510,8 @@ void TStreamerInfo::Build() if (!infoalloc) { Error("Build","Could you create a TStreamerInfo for %s\n",TString::Format("%s@@%d",GetName(),GetClassVersion()).Data()); } else { + // Tell clone we should rerun BuildOld + infoalloc->SetBit(kBuildOldUsed,false); infoalloc->BuildCheck(); infoalloc->BuildOld(); TClass *allocClass = infoalloc->GetClass(); @@ -1266,6 +1270,15 @@ namespace { } return kFALSE; } + + //Makes sure kBuildOldUsed set once BuildOld finishes + struct BuildOldGuard { + BuildOldGuard(TStreamerInfo* info): fInfo(info) {} + ~BuildOldGuard() { + fInfo->SetBit(TStreamerInfo::kBuildOldUsed); + } + TStreamerInfo* fInfo; + }; } //______________________________________________________________________________ @@ -1275,7 +1288,7 @@ void TStreamerInfo::BuildOld() R__LOCKGUARD(gCINTMutex); if( TestBit(kBuildOldUsed) && !IsOptimized() ) return; - SetBit(kBuildOldUsed); + BuildOldGuard buildOldGuard(this); if (gDebug > 0) { printf("\n====>Rebuilding TStreamerInfo for class: %s, version: %d\n", GetName(), fClassVersion); @@ -1948,6 +1961,7 @@ void TStreamerInfo::BuildOld() if (!infoalloc) { Error("BuildOld","Unable to create the StreamerInfo for %s.",TString::Format("%s@@%d",GetName(),GetOnFileClassVersion()).Data()); } else { + infoalloc->SetBit(kBuildOldUsed,false); infoalloc->BuildCheck(); infoalloc->BuildOld(); allocClass = infoalloc->GetClass(); From 25d5ea556bec1a2695d9e6c2d7d2cde8e11b9c04 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 02:57:46 +0100 Subject: [PATCH 23/38] Added additional locks in TClass to protect CINT data structures --- core/meta/src/TClass.cxx | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/core/meta/src/TClass.cxx b/core/meta/src/TClass.cxx index 9e5ce76f07c44..9d7c3c41d635d 100644 --- a/core/meta/src/TClass.cxx +++ b/core/meta/src/TClass.cxx @@ -2354,16 +2354,17 @@ Int_t TClass::GetBaseClassOffsetRecurse(const TClass *cl) c = inh->GetClassPointer(kTRUE); // kFALSE); if (c) { if (cl == c) { - { - R__LOCKGUARD(gCINTMutex); - if ((inh->Property() & G__BIT_ISVIRTUALBASE) != 0) - return -2; - } + R__LOCKGUARD(gCINTMutex); + if ((inh->Property() & G__BIT_ISVIRTUALBASE) != 0) + return -2; return inh->GetDelta(); } off = c->GetBaseClassOffsetRecurse(cl); if (off == -2) return -2; - if (off != -1) return off + inh->GetDelta(); + if (off != -1) { + R__LOCKGUARD(gCINTMutex); + return off + inh->GetDelta(); + } } lnk = lnk->Next(); } @@ -3004,7 +3005,10 @@ TList *TClass::GetListOfBases() if (!gInterpreter) Fatal("GetListOfBases", "gInterpreter not initialized"); - gInterpreter->CreateListOfBaseClasses(this); + R__LOCKGUARD(gCINTMutex); + if(!fBase) { + gInterpreter->CreateListOfBaseClasses(this); + } } return fBase; } @@ -4254,8 +4258,10 @@ void TClass::Destructor(void *obj, Bool_t dtorOnly) // or it will be interpreted, otherwise we fail // because there is no destructor code at all. if (dtorOnly) { + R__LOCKGUARD2(gCINTMutex); gCint->ClassInfo_Destruct(fClassInfo,p); } else { + R__LOCKGUARD2(gCINTMutex); gCint->ClassInfo_Delete(fClassInfo,p); } } else if (!fClassInfo && fCollectionProxy) { From 8ff03b16bb58176b8f11401b7069e2aca065cdb3 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 21:16:21 +0100 Subject: [PATCH 24/38] Made TROOT ReadingObject methods thread safe Made use of a thread_local to hold the ReadingObject status since the status is only meant to apply to a call chain. --- core/base/inc/TROOT.h | 4 ++-- core/base/src/TROOT.cxx | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/core/base/inc/TROOT.h b/core/base/inc/TROOT.h index 78d3a96a07ac7..45ae6eafd9c2b 100644 --- a/core/base/inc/TROOT.h +++ b/core/base/inc/TROOT.h @@ -241,7 +241,7 @@ friend class TCintWithCling; Long_t ProcessLine(const char *line, Int_t *error = 0); Long_t ProcessLineSync(const char *line, Int_t *error = 0); Long_t ProcessLineFast(const char *line, Int_t *error = 0); - Bool_t ReadingObject() const { /* Deprecated (will be removed in next release) */ return fReadingObject; } + Bool_t ReadingObject() const; void RefreshBrowsers(); void RemoveClass(TClass *); void Reset(Option_t *option=""); @@ -258,7 +258,7 @@ friend class TCintWithCling; void SetEscape(Bool_t flag = kTRUE) { fEscape = flag; } void SetLineIsProcessing() { fLineIsProcessing++; } void SetLineHasBeenProcessed() { if (fLineIsProcessing) fLineIsProcessing--; } - void SetReadingObject(Bool_t flag = kTRUE) { fReadingObject = flag; } + void SetReadingObject(Bool_t flag = kTRUE); void SetMustClean(Bool_t flag = kTRUE) { fMustClean=flag; } void SetSelectedPrimitive(const TObject *obj) { fPrimitive = obj; } void SetSelectedPad(TVirtualPad *pad) { fSelectPad = pad; } diff --git a/core/base/src/TROOT.cxx b/core/base/src/TROOT.cxx index a3135619033c7..5ddb953c79b74 100644 --- a/core/base/src/TROOT.cxx +++ b/core/base/src/TROOT.cxx @@ -1817,6 +1817,28 @@ void TROOT::ReadGitInfo() delete [] filename; } +static thread_local Bool_t fgReadingObject = false; +//______________________________________________________________________________ +Bool_t TROOT::ReadingObject() const +{ + /* Deprecated (will be removed in next release) */ +#if __cplusplus > 199711L + return fgReadingObject; +#else + return fReadingObject; +#endif +} + +void TROOT::SetReadingObject(Bool_t flag) +{ +#if __cplusplus > 199711L + fgReadingObject = flag; +#else + fReadingObject = flag; +#endif +} + + //______________________________________________________________________________ const char *TROOT::GetGitDate() { From 15d51d1b94ac3503f8e1b6aa50b1b5b432dbbad8 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 21:18:45 +0100 Subject: [PATCH 25/38] Made global counters in TFile atomic To properly handle cases where different TFiles are being used on different threads, the various global counters in TFile were converted to std::atomic<>. --- io/io/inc/TFile.h | 12 +++++++++++- io/io/src/TFile.cxx | 9 ++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/io/io/inc/TFile.h b/io/io/inc/TFile.h index 9018215336d0e..1f22a8a3f9fde 100644 --- a/io/io/inc/TFile.h +++ b/io/io/inc/TFile.h @@ -21,6 +21,9 @@ // // ////////////////////////////////////////////////////////////////////////// +#if __cplusplus > 199711L +#include +#endif #ifndef ROOT_TDirectoryFile #include "TDirectoryFile.h" #endif @@ -104,13 +107,20 @@ class TFile : public TDirectoryFile { static Bool_t fgCacheFileForce; //Indicates, to force all READ to CACHEREAD static UInt_t fgOpenTimeout; //Timeout for open operations in ms - 0 corresponds to blocking i/o static Bool_t fgOnlyStaged ; //Before the file is opened, it is checked, that the file is staged, if not, the open fails + +#if __cplusplus > 199711L + static std::atomic fgBytesWrite; //Number of bytes written by all TFile objects + static std::atomic fgBytesRead; //Number of bytes read by all TFile objects + static std::atomic fgFileCounter; //Counter for all opened files + static std::atomic fgReadCalls; //Number of bytes read from all TFile objects +#else static Long64_t fgBytesWrite; //Number of bytes written by all TFile objects static Long64_t fgBytesRead; //Number of bytes read by all TFile objects static Long64_t fgFileCounter; //Counter for all opened files static Int_t fgReadCalls; //Number of bytes read from all TFile objects +#endif static Int_t fgReadaheadSize; //Readahead buffer size static Bool_t fgReadInfo; //if true (default) ReadStreamerInfo is called when opening a file - virtual EAsyncOpenStatus GetAsyncOpenStatus() { return fAsyncOpenStatus; } virtual void Init(Bool_t create); Bool_t FlushWriteCache(); diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 9c93e556c5a5d..6f4880697a51e 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -118,11 +118,18 @@ #include "TSchemaRuleSet.h" #include "TThreadSlots.h" +#if __cplusplus > 199711L +std::atomic TFile::fgBytesRead{0}; +std::atomic TFile::fgBytesWrite{0}; +std::atomic TFile::fgFileCounter{0}; +std::atomic TFile::fgReadCalls{0}; +#else Long64_t TFile::fgBytesRead = 0; Long64_t TFile::fgBytesWrite = 0; Long64_t TFile::fgFileCounter = 0; -Int_t TFile::fgReadaheadSize = 256000; Int_t TFile::fgReadCalls = 0; +#endif +Int_t TFile::fgReadaheadSize = 256000; Bool_t TFile::fgReadInfo = kTRUE; TList *TFile::fgAsyncOpenRequests = 0; TString TFile::fgCacheFileDir; From 40ebff6da19f8713344f2292426abe287a1db92d Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 21:22:40 +0100 Subject: [PATCH 26/38] Used std::atomic to protect TStreamerInfo counters fLiveCount and fgCount are now atomic since they can be read/modified on different threads without a mutex. --- io/io/inc/TStreamerInfo.h | 11 +++++++++-- io/io/src/TStreamerInfo.cxx | 5 +++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/io/io/inc/TStreamerInfo.h b/io/io/inc/TStreamerInfo.h index 600c6c4895f4d..87528fbb4e5fb 100644 --- a/io/io/inc/TStreamerInfo.h +++ b/io/io/inc/TStreamerInfo.h @@ -20,6 +20,9 @@ // Describe Streamer information for one class version // // // ////////////////////////////////////////////////////////////////////////// +#if __cplusplus > 199711L +#include +#endif #ifndef ROOT_TVirtualStreamerInfo #include "TVirtualStreamerInfo.h" @@ -104,17 +107,21 @@ class TStreamerInfo : public TVirtualStreamerInfo { Version_t fOldVersion; //! Version of the TStreamerInfo object read from the file Int_t fNVirtualInfoLoc; //! Number of virtual info location to update. ULong_t *fVirtualInfoLoc; //![fNVirtualInfoLoc] Location of the pointer to the TStreamerInfo inside the object (when emulated) +#if __cplusplus > 199711L + std::atomic fLiveCount; //! Number of outstanding pointer to this StreamerInfo. +#else ULong_t fLiveCount; //! Number of outstanding pointer to this StreamerInfo. - +#endif TStreamerInfoActions::TActionSequence *fReadObjectWise; //! List of read action resulting from the compilation. TStreamerInfoActions::TActionSequence *fReadMemberWise; //! List of read action resulting from the compilation for use in member wise streaming. TStreamerInfoActions::TActionSequence *fWriteObjectWise; //! List of write action resulting from the compilation. TStreamerInfoActions::TActionSequence *fWriteMemberWise; //! List of write action resulting from the compilation for use in member wise streaming. - static Int_t fgCount; //Number of TStreamerInfo instances #if __cplusplus > 199711L + static std::atomic fgCount; //Number of TStreamerInfo instances static thread_local TStreamerElement *fgElement; //Pointer to current TStreamerElement #else + static Int_t fgCount; //Number of TStreamerInfo instances static TStreamerElement *fgElement; //Pointer to current TStreamerElement #endif template static T GetTypedValueAux(Int_t type, void *ladd, int k, Int_t len); diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index 43d9ea7f15b54..8a9721b84ef12 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -73,11 +73,12 @@ #include "TStreamerInfoActions.h" #if __cplusplus > 199711L -thread_local TStreamerElement *TStreamerInfo::fgElement = 0; +thread_local TStreamerElement *TStreamerInfo::fgElement = nullptr; +std::atomic TStreamerInfo::fgCount{0}; #else TStreamerElement *TStreamerInfo::fgElement = 0; -#endif Int_t TStreamerInfo::fgCount = 0; +#endif const Int_t kMaxLen = 1024; From fba383ea217799975929122839d84fb219cc232b Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 5 Feb 2014 21:26:10 +0100 Subject: [PATCH 27/38] Use std::atomic to protect access to TClass state Changed fVersionUsed and fgClassCount of TClass to be std::atomic since both could be read/modified from different threads without a mutex. Also put back fgCallingNew as a class member to minimize code changes with non-thread safe version. --- core/meta/inc/TClass.h | 10 ++++++++++ core/meta/src/TClass.cxx | 9 +++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/core/meta/inc/TClass.h b/core/meta/inc/TClass.h index 3a3b3c3645e34..62b9ed21e1ce5 100644 --- a/core/meta/inc/TClass.h +++ b/core/meta/inc/TClass.h @@ -136,7 +136,11 @@ friend class ROOT::TGenericClassInfo; mutable Int_t fCanSplit; //!Indicates whether this class can be split or not. mutable Long_t fProperty; //!Property +#if __cplusplus > 199711L + mutable std::atomic fVersionUsed; //!Indicates whether GetClassVersion has been called +#else mutable Bool_t fVersionUsed; //!Indicates whether GetClassVersion has been called +#endif mutable Bool_t fIsOffsetStreamerSet; //!saved remember if fOffsetStreamer has been set. mutable Long_t fOffsetStreamer; //!saved info to call Streamer @@ -177,7 +181,13 @@ friend class ROOT::TGenericClassInfo; void StreamerDefault(void *object, TBuffer &b, const TClass *onfile_class) const; static IdMap_t *GetIdMap(); //Map from typeid to TClass pointer +#if __cplusplus > 199711L + static thread_local ENewType fgCallingNew; //Intent of why/how TClass::New() is called + static std::atomic fgClassCount; //provides unique id for a each class +#else + static ENewType fgCallingNew; //Intent of why/how TClass::New() is called static Int_t fgClassCount; //provides unique id for a each class +#endif //stored in TObject::fUniqueID // Internal status bits enum { kLoading = BIT(14) }; diff --git a/core/meta/src/TClass.cxx b/core/meta/src/TClass.cxx index 9d7c3c41d635d..7c692e5422330 100644 --- a/core/meta/src/TClass.cxx +++ b/core/meta/src/TClass.cxx @@ -92,8 +92,13 @@ namespace { }; } -static thread_local TClass::ENewType fgCallingNew = TClass::kRealNew; //Intent of why/how TClass::New() is called +#if __cplusplus > 199711L +std::atomic TClass::fgClassCount; +thread_local TClass::ENewType TClass::fgCallingNew = TClass::kRealNew; +#else Int_t TClass::fgClassCount; +TClass::ENewType TClass::fgCallingNew = TClass::kRealNew; +#endif struct ObjRepoValue { ObjRepoValue(const TClass *what, Version_t version) : fClass(what),fVersion(version) {} @@ -1159,7 +1164,7 @@ TClass::TClass(const TClass& cl) : fSizeof(cl.fSizeof), fCanSplit(cl.fCanSplit), fProperty(cl.fProperty), - fVersionUsed(cl.fVersionUsed), + fVersionUsed(), fIsOffsetStreamerSet(cl.fIsOffsetStreamerSet), fOffsetStreamer(cl.fOffsetStreamer), fStreamerType(cl.fStreamerType), From 6659ee5d7a735f109fa5da6834a83173e69ef556 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 7 Feb 2014 23:53:31 +0100 Subject: [PATCH 28/38] Need to reset kBuildOldUsed bit when reading TStreamerInfo The kBuildOldUsed bit is transient, like kIsCompiled, and therefore must be reset when a TStreamerInfo is read from a file. --- io/io/src/TStreamerInfo.cxx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index 8a9721b84ef12..01add3f41b944 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -2091,6 +2091,7 @@ void TStreamerInfo::Clear(Option_t *option) fNdata = 0; fSize = 0; ResetBit(kIsCompiled); + ResetBit(kBuildOldUsed); if (fReadObjectWise) fReadObjectWise->fActions.clear(); if (fReadMemberWise) fReadMemberWise->fActions.clear(); @@ -4336,6 +4337,7 @@ void TStreamerInfo::Streamer(TBuffer &R__b) R__b.ClassEnd(TStreamerInfo::Class()); R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); ResetBit(kIsCompiled); + ResetBit(kBuildOldUsed); return; } //====process old versions before automatic schema evolution From 8e6ccdd167bb766eeca6136eadc140789e3426e6 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 12 Feb 2014 08:44:27 -0600 Subject: [PATCH 29/38] Need to use CINT mutex in Cintex::Callback::operator() During the call the Cintex::Callback::operator(), several CINT data structures are modified and therefore we must take the lock. --- cint/cintex/src/Cintex.cxx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cint/cintex/src/Cintex.cxx b/cint/cintex/src/Cintex.cxx index 92f59d20760fe..9b73af23b4648 100644 --- a/cint/cintex/src/Cintex.cxx +++ b/cint/cintex/src/Cintex.cxx @@ -25,6 +25,8 @@ #include #include "TROOT.h" +#include "TInterpreter.h" //gCINTMutex +#include "TVirtualMutex.h" using namespace ROOT::Reflex; using namespace ROOT::Cintex; @@ -187,6 +189,7 @@ namespace ROOT { } void Callback::operator () ( const Type& t ) { + R__LOCKGUARD2(gCINTMutex); ArtificialSourceFile asf; int autoload = G__set_class_autoloading(0); // To avoid recursive loads if ( t.IsClass() || t.IsStruct() ) { @@ -205,6 +208,7 @@ namespace ROOT { } void Callback::operator () ( const Member& m ) { + R__LOCKGUARD2(gCINTMutex); ArtificialSourceFile asf; int autoload = G__set_class_autoloading(0); // To avoid recursive loads if ( m.IsFunctionMember() ) { From ea025acf602fdab4e2f2051fd417704b57cfcd74 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 12 Feb 2014 08:46:17 -0600 Subject: [PATCH 30/38] Minimize the time spent holding the Cintex mutex lock To avoid lock order problems, we minimize the time spend holding the Cintex lock by ROOTClassEnhancer. In addition, there was a race condition in the previous code during the returning of the value. That race condition is now fixed. --- cint/cintex/src/ROOTClassEnhancer.cxx | 55 +++++++++++++++++---------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/cint/cintex/src/ROOTClassEnhancer.cxx b/cint/cintex/src/ROOTClassEnhancer.cxx index f7ddf8ef5c098..68187aa2e6b1d 100644 --- a/cint/cintex/src/ROOTClassEnhancer.cxx +++ b/cint/cintex/src/ROOTClassEnhancer.cxx @@ -36,6 +36,9 @@ #include #include #include +#if __cplusplus > 199711L +#include +#endif #if ROOT_VERSION_CODE >= ROOT_VERSION(5,1,1) #include "TVirtualIsAProxy.h" @@ -55,7 +58,11 @@ namespace ROOT { namespace Cintex { Type fType; string fName; +#if __cplusplus > 199711L + std::atomic fTclass; +#else TClass* fTclass; +#endif TClass* fLastClass; std::map fSub_types; const std::type_info* fLastType; @@ -367,38 +374,44 @@ namespace ROOT { namespace Cintex { if ( ! obj || ! fIsVirtual ) { return Tclass(); } - else { - // Avoid the case that the first word is a virtual_base_offset_table instead of - // a virtual_function_table - long Offset = **(long**)obj; - if ( Offset == 0 ) return Tclass(); - DynamicStruct_t* p = (DynamicStruct_t*)obj; - const std::type_info& typ = typeid(*p); + // Avoid the case that the first word is a virtual_base_offset_table instead of + // a virtual_function_table + long Offset = **(long**)obj; + if ( Offset == 0 ) return Tclass(); - if ( &typ == fMyType ) { - return Tclass(); - } + DynamicStruct_t* p = (DynamicStruct_t*)obj; + const std::type_info& typ = typeid(*p); + + if ( &typ == fMyType ) { + return Tclass(); + } + { R__LOCKGUARD2(gCintexMutex); if ( &typ == fLastType ) { return fLastClass; } + // Check if TypeNth is already in sub-class cache - else if ( 0 != (fLastClass=fSub_types[&typ]) ) { + if ( 0 != (fLastClass=fSub_types[&typ]) ) { fLastType = &typ; + return fLastClass; } - // Last resort: lookup root class - else { - std::string nam; - Type t = Type::ByTypeInfo(typ); - if (t) nam = CintName(t); - else nam = CintName(Tools::Demangle(typ)); - fLastClass = ROOT::GetROOT()->GetClass(nam.c_str()); - fSub_types[fLastType=&typ] = fLastClass; - } + } + // Last resort: lookup root class + TClass* returnValue; + std::string nam; + Type t = Type::ByTypeInfo(typ); + if (t) nam = CintName(t); + else nam = CintName(Tools::Demangle(typ)); + returnValue = ROOT::GetROOT()->GetClass(nam.c_str()); + { + R__LOCKGUARD2(gCintexMutex); + fLastClass = returnValue; + fSub_types[fLastType=&typ] = fLastClass; } //std::cout << "Cintex: IsA:" << TypeNth.Name(SCOPED) << " dynamic:" << dtype.Name(SCOPED) << std::endl; - return fLastClass; + return returnValue; } TClass* ROOTClassEnhancerInfo::Default_CreateClass( Type typ, ROOT::TGenericClassInfo* info) { From 865a4015a4062642d9f063a6d06d0e3928d9ed14 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 13 Feb 2014 18:02:26 +0100 Subject: [PATCH 31/38] Make last error string in TSystem a thread_local To handle errors occuring on different threads, the caching of the last error string was changed from being a member data to a class static thread_local. This meshes well with Unix since errno is a thread local there. --- core/base/inc/TSystem.h | 8 ++++++++ core/base/src/TSystem.cxx | 13 ++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/core/base/inc/TSystem.h b/core/base/inc/TSystem.h index 0d1687c1d2e5e..fe39050428227 100644 --- a/core/base/inc/TSystem.h +++ b/core/base/inc/TSystem.h @@ -316,6 +316,10 @@ class TSystem : public TNamed { TSeqCollection *fCompiled; //List of shared libs from compiled macros to be deleted TSeqCollection *fHelpers; //List of helper classes for alternative file/directory access +#if __cplusplus > 199711L + static thread_local TString fgLastErrorString; //Last system error message +#endif + TSystem *FindHelper(const char *path, void *dirptr = 0); virtual Bool_t ConsistentWith(const char *path, void *dirptr = 0); virtual const char *ExpandFileName(const char *fname); @@ -340,7 +344,11 @@ class TSystem : public TNamed { virtual void SetProgname(const char *name); virtual void SetDisplay(); void SetErrorStr(const char *errstr); +#if __cplusplus > 199711L + const char *GetErrorStr() const { return fgLastErrorString; } +#else const char *GetErrorStr() const { return fLastErrorString; } +#endif virtual const char *GetError(); void RemoveOnExit(TObject *obj); virtual const char *HostName(); diff --git a/core/base/src/TSystem.cxx b/core/base/src/TSystem.cxx index f517c25d5b993..9df61fa4528c6 100644 --- a/core/base/src/TSystem.cxx +++ b/core/base/src/TSystem.cxx @@ -64,6 +64,13 @@ static Int_t *gLibraryVersion = 0; // Set in TVersionCheck, used in Load() static Int_t gLibraryVersionIdx = 0; // Set in TVersionCheck, used in Load() static Int_t gLibraryVersionMax = 256; +#if __cplusplus > 199711L +thread_local TString TSystem::fgLastErrorString; +#define LAST_ERROR_STRING fgLastErrorString +#else +#define LAST_ERROR_STRING fLastErrorString +#endif + ClassImp(TProcessEventTimer) //______________________________________________________________________________ @@ -246,7 +253,7 @@ void TSystem::SetErrorStr(const char *errstr) // library that does not use standard errno). ResetErrno(); // so GetError() uses the fLastErrorString - fLastErrorString = errstr; + LAST_ERROR_STRING = errstr; } //______________________________________________________________________________ @@ -254,8 +261,8 @@ const char *TSystem::GetError() { // Return system error string. - if (GetErrno() == 0 && fLastErrorString != "") - return fLastErrorString; + if (GetErrno() == 0 && LAST_ERROR_STRING != "") + return LAST_ERROR_STRING; return Form("errno: %d", GetErrno()); } From 48a6405f213fd2f6ea333c461571eb5f1dc9b5da Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 13 Feb 2014 18:12:30 +0100 Subject: [PATCH 32/38] Thread safety fixes for TUnixSystem TSystem changed the last error string to be a class thread_local so this code matches that change. Caching of external path is now a thread_local to avoid race conditions. Caching of initial time is done via an atomic. --- core/unix/src/TUnixSystem.cxx | 37 ++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/core/unix/src/TUnixSystem.cxx b/core/unix/src/TUnixSystem.cxx index e22c9d1227e86..a26b8ef8318c9 100644 --- a/core/unix/src/TUnixSystem.cxx +++ b/core/unix/src/TUnixSystem.cxx @@ -37,6 +37,9 @@ #include "TVirtualMutex.h" #include "TObjArray.h" #include +#if __cplusplus > 199711L +#include +#endif //#define G__OLDEXPAND @@ -438,7 +441,11 @@ static void SigHandler(ESignals sig) //______________________________________________________________________________ static const char *GetExePath() { +#if __cplusplus > 199711L + static thread_local TString exepath; +#else static TString exepath; +#endif if (exepath == "") { #if defined(R__MACOSX) exepath = _dyld_get_image_name(0); @@ -582,6 +589,11 @@ static void DylibAdded(const struct mach_header *mh, intptr_t /* vmaddr_slide */ } #endif +#if __cplusplus > 199711L +#define LAST_ERROR_STRING fgLastErrorString +#else +#define LAST_ERROR_STRING fLastErrorString +#endif ClassImp(TUnixSystem) @@ -727,8 +739,9 @@ const char *TUnixSystem::GetError() // Return system error string. Int_t err = GetErrno(); - if (err == 0 && fLastErrorString != "") - return fLastErrorString; + if (err == 0 && LAST_ERROR_STRING != "") + return LAST_ERROR_STRING; + #if defined(R__SOLARIS) || defined (R__LINUX) || defined(R__AIX) || \ defined(R__FBSD) || defined(R__OBSD) || defined(R__HURD) return strerror(err); @@ -1576,7 +1589,8 @@ Bool_t TUnixSystem::AccessPathName(const char *path, EAccessMode mode) if (::access(StripOffProto(path, "file:"), mode) == 0) return kFALSE; - fLastErrorString = GetError(); + LAST_ERROR_STRING = GetError(); + return kTRUE; } @@ -1623,7 +1637,7 @@ int TUnixSystem::Rename(const char *f, const char *t) // Rename a file. Returns 0 when successful, -1 in case of failure. int ret = ::rename(f, t); - fLastErrorString = GetError(); + LAST_ERROR_STRING = GetError(); return ret; } @@ -1825,7 +1839,7 @@ Bool_t TUnixSystem::ExpandPathName(TString &patbuf0) } else { hd = UnixHomedirectory(0); if (hd == 0) { - fLastErrorString = GetError(); + LAST_ERROR_STRING = GetError(); return kTRUE; } cmd += hd; @@ -1835,7 +1849,7 @@ Bool_t TUnixSystem::ExpandPathName(TString &patbuf0) cmd += stuffedPat; if ((pf = ::popen(cmd.Data(), "r")) == 0) { - fLastErrorString = GetError(); + LAST_ERROR_STRING = GetError(); return kTRUE; } @@ -1858,7 +1872,7 @@ Bool_t TUnixSystem::ExpandPathName(TString &patbuf0) while (ch != EOF) { ch = fgetc(pf); if (ch == ' ' || ch == '\t') { - fLastErrorString = "expression ambigous"; + LAST_ERROR_STRING = "expression ambigous"; ::pclose(pf); return kTRUE; } @@ -3769,8 +3783,13 @@ void TUnixSystem::UnixIgnoreSignal(ESignals sig, Bool_t ignr) // If ignr is true ignore the specified signal, else restore previous // behaviour. +#if __cplusplus > 199711L + static thread_local Bool_t ignoreSig[kMAXSIGNALS] = { kFALSE }; + static thread_local struct sigaction oldsigact[kMAXSIGNALS]; +#else static Bool_t ignoreSig[kMAXSIGNALS] = { kFALSE }; static struct sigaction oldsigact[kMAXSIGNALS]; +#endif if (ignr != ignoreSig[sig]) { ignoreSig[sig] = ignr; @@ -3874,7 +3893,11 @@ Long64_t TUnixSystem::UnixNow() { // Get current time in milliseconds since 0:00 Jan 1 1995. +#if __cplusplus > 199711L + static std::atomic jan95{0}; +#else static time_t jan95 = 0; +#endif if (!jan95) { struct tm tp; tp.tm_year = 95; From 371da0080ce15accb5be9c8fea208d32edb85906 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 13 Feb 2014 18:17:25 +0100 Subject: [PATCH 33/38] Use Mutex to protect gObjectVersionRepository TClass.cxx has a file scope global gObjectVersionRepository which can be modified simultaneously on multiple threads. Therefore it needed a mutex lock when reading/writing. --- core/meta/src/TClass.cxx | 71 ++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/core/meta/src/TClass.cxx b/core/meta/src/TClass.cxx index 7c692e5422330..de29906cb012b 100644 --- a/core/meta/src/TClass.cxx +++ b/core/meta/src/TClass.cxx @@ -105,6 +105,8 @@ struct ObjRepoValue { const TClass *fClass; Version_t fVersion; }; + +static TVirtualMutex* gOVRMutex = 0; typedef std::multimap RepoCont_t; static RepoCont_t gObjectVersionRepository; @@ -118,8 +120,10 @@ static void RegisterAddressInRepository(const char * /*where*/, void *location, // } else { // Warning(where, "Registering address %p again of class '%s' version %d", location, what->GetName(), version); // } - gObjectVersionRepository.insert(RepoCont_t::value_type(location, RepoCont_t::mapped_type(what,version))); - + { + R__LOCKGUARD2(gOVRMutex); + gObjectVersionRepository.insert(RepoCont_t::value_type(location, RepoCont_t::mapped_type(what,version))); + } #if 0 // This code could be used to prevent an address to be registered twice. std::pair tmp = gObjectVersionRepository.insert(RepoCont_t::value_type>(location, RepoCont_t::mapped_type(what,version))); @@ -138,6 +142,7 @@ static void UnregisterAddressInRepository(const char * /*where*/, void *location { // Remove an address from the repository of address/object. + R__LOCKGUARD2(gOVRMutex); RepoCont_t::iterator cur = gObjectVersionRepository.find(location); for (; cur != gObjectVersionRepository.end();) { RepoCont_t::iterator tmp = cur++; @@ -159,6 +164,7 @@ static void MoveAddressInRepository(const char * /*where*/, void *oldadd, void * // Move not only the object itself but also any base classes or sub-objects. size_t objsize = what->Size(); long delta = (char*)newadd - (char*)oldadd; + R__LOCKGUARD2(gOVRMutex); RepoCont_t::iterator cur = gObjectVersionRepository.find(oldadd); for (; cur != gObjectVersionRepository.end();) { RepoCont_t::iterator tmp = cur++; @@ -4285,20 +4291,24 @@ void TClass::Destructor(void *obj, Bool_t dtorOnly) // Was this object allocated through TClass? std::multiset knownVersions; - RepoCont_t::iterator iter = gObjectVersionRepository.find(p); - if (iter == gObjectVersionRepository.end()) { - // No, it wasn't, skip special version handling. - //Error("Destructor2", "Attempt to delete unregistered object of class '%s' at address %p!", GetName(), p); - inRepo = kFALSE; - } else { - //objVer = iter->second; - for (; (iter != gObjectVersionRepository.end()) && (iter->first == p); ++iter) { - Version_t ver = iter->second.fVersion; - knownVersions.insert(ver); - if (ver == fClassVersion && this == iter->second.fClass) { - verFound = kTRUE; - } - } + R__LOCKGUARD2(gOVRMutex); + + { + RepoCont_t::iterator iter = gObjectVersionRepository.find(p); + if (iter == gObjectVersionRepository.end()) { + // No, it wasn't, skip special version handling. + //Error("Destructor2", "Attempt to delete unregistered object of class '%s' at address %p!", GetName(), p); + inRepo = kFALSE; + } else { + //objVer = iter->second; + for (; (iter != gObjectVersionRepository.end()) && (iter->first == p); ++iter) { + Version_t ver = iter->second.fVersion; + knownVersions.insert(ver); + if (ver == fClassVersion && this == iter->second.fClass) { + verFound = kTRUE; + } + } + } } if (!inRepo || verFound) { @@ -4396,19 +4406,22 @@ void TClass::DeleteArray(void *ary, Bool_t dtorOnly) // Was this array object allocated through TClass? std::multiset knownVersions; - RepoCont_t::iterator iter = gObjectVersionRepository.find(p); - if (iter == gObjectVersionRepository.end()) { - // No, it wasn't, we cannot know what to do. - //Error("DeleteArray", "Attempt to delete unregistered array object, element type '%s', at address %p!", GetName(), p); - inRepo = kFALSE; - } else { - for (; (iter != gObjectVersionRepository.end()) && (iter->first == p); ++iter) { - Version_t ver = iter->second.fVersion; - knownVersions.insert(ver); - if (ver == fClassVersion && this == iter->second.fClass ) { - verFound = kTRUE; - } - } + { + R__LOCKGUARD2(gOVRMutex); + RepoCont_t::iterator iter = gObjectVersionRepository.find(p); + if (iter == gObjectVersionRepository.end()) { + // No, it wasn't, we cannot know what to do. + //Error("DeleteArray", "Attempt to delete unregistered array object, element type '%s', at address %p!", GetName(), p); + inRepo = kFALSE; + } else { + for (; (iter != gObjectVersionRepository.end()) && (iter->first == p); ++iter) { + Version_t ver = iter->second.fVersion; + knownVersions.insert(ver); + if (ver == fClassVersion && this == iter->second.fClass ) { + verFound = kTRUE; + } + } + } } if (!inRepo || verFound) { From 703352216a0d493a06c54d7dfa72e598fb5e597e Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 13 Feb 2014 18:21:06 +0100 Subject: [PATCH 34/38] TStreamerElement cached strings changed to thread_local Full name and include name were statics which were set the first time called. This was not thread safe. These were changed to thread_local. Alternatively, this could be change to use std::call_once. --- core/meta/src/TStreamerElement.cxx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/meta/src/TStreamerElement.cxx b/core/meta/src/TStreamerElement.cxx index 52b8c631785a0..2f4bfbe3fae07 100644 --- a/core/meta/src/TStreamerElement.cxx +++ b/core/meta/src/TStreamerElement.cxx @@ -39,7 +39,12 @@ namespace std {} using namespace std; const Int_t kMaxLen = 1024; + +#if __cplusplus > 199711L +static thread_local TString gIncludeName(kMaxLen); +#else static TString gIncludeName(kMaxLen); +#endif extern void *gMmallocDesc; @@ -295,7 +300,11 @@ const char *TStreamerElement::GetFullName() const // Note that this function stores the name into a static array. // You should copy the result. +#if __cplusplus > 199711L + static thread_local TString name(kMaxLen); +#else static TString name(kMaxLen); +#endif char cdim[20]; name = GetName(); for (Int_t i=0;i Date: Thu, 13 Feb 2014 18:24:29 +0100 Subject: [PATCH 35/38] Fix thread-safety issues with string formating The stand-alone formating functions return a pointer to a static buffer. That is not thread-safe since the buffer could be updated on a different thread while another thread is reading it. This change makes the static buffer variable thread_local. --- core/base/src/TString.cxx | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index b6500b95c0978..b53843f10b5b7 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -2348,20 +2348,32 @@ static const int cb_size = 4096; static const int fld_size = 2048; // a circular formating buffer +#if __cplusplus > 199711L +static thread_local char gFormbuf[cb_size]; // some slob for form overflow +static thread_local char *gBfree = gFormbuf; +static thread_local char *gEndbuf = &gFormbuf[cb_size-1]; +#else static char gFormbuf[cb_size]; // some slob for form overflow static char *gBfree = gFormbuf; static char *gEndbuf = &gFormbuf[cb_size-1]; - +#endif //______________________________________________________________________________ static char *SlowFormat(const char *format, va_list ap, int hint) { // Format a string in a formatting buffer (using a printf style // format descriptor). +#if __cplusplus > 199711L + static thread_local char *slowBuffer = 0; + static thread_local int slowBufferSize = 0; +#else static char *slowBuffer = 0; static int slowBufferSize = 0; + //NOTE: since slowBuffer is returned from this function, + // this lock guard is ineffectual in protecting slowBuffer reads R__LOCKGUARD2(gStringMutex); +#endif if (hint == -1) hint = fld_size; if (hint > slowBufferSize) { @@ -2407,8 +2419,12 @@ static char *Format(const char *format, va_list ap) // Format a string in a circular formatting buffer (using a printf style // format descriptor). +#if __cplusplus <= 199711L + //NOTE: since an address into the shared buffer is returned from this + // function, this lock is ineffectual in protecting reads from + // the buffer. R__LOCKGUARD2(gStringMutex); - +#endif char *buf = gBfree; if (buf+fld_size > gEndbuf) From dde43fdb8a5e4e935cba51e2a808df278d146387 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 13 Feb 2014 21:22:10 +0100 Subject: [PATCH 36/38] fgIsA needs to be thread safe Classes which inherit from TObject have a fgIsA class member defined by the code generator. However, the value for fgIsA is set lazily and therefor must be protected in a threaded environment. fgIsA was changed to be std::atomic<> and the setting of the value was protected by gCINTMutex to avoid having the calculation done more than once. --- cint/reflex/python/genreflex/gendict.py | 11 ++++++++--- core/base/inc/Rtypes.h | 13 ++++++++++--- core/utils/src/rootcint.cxx | 8 +++++--- core/utils/src/rootcling.cxx | 8 +++++--- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/cint/reflex/python/genreflex/gendict.py b/cint/reflex/python/genreflex/gendict.py index f64ce1f15b8b6..195b4e3baf53d 100644 --- a/cint/reflex/python/genreflex/gendict.py +++ b/cint/reflex/python/genreflex/gendict.py @@ -2654,6 +2654,8 @@ def ClassDefImplementation(selclasses, self) : returnValue += '#endif\n' returnValue += '#include "TClass.h"\n' returnValue += '#include "TMemberInspector.h"\n' + returnValue += '#include "TInterpreter.h"\n' + returnValue += '#include "TVirtualMutex.h"\n' returnValue += '#include "RtypesImp.h"\n' # for GenericShowMembers etc returnValue += '#include "TIsAProxy.h"\n' haveClassDef = 0 @@ -2725,8 +2727,11 @@ def ClassDefImplementation(selclasses, self) : specclname = clname returnValue += template + 'TClass* ' + specclname + '::Class() {\n' - returnValue += ' if (!fgIsA)\n' - returnValue += ' fgIsA = TClass::GetClass("' + clname[2:] + '");\n' + returnValue += ' if (!fgIsA) {\n' + returnValue += ' R__LOCKGUARD2(gCINTMutex);' + returnValue += ' if (!fgIsA)\n' + returnValue += ' fgIsA = TClass::GetClass("' + clname[2:] + '");\n' + returnValue += ' }\n' returnValue += ' return fgIsA;\n' returnValue += '}\n' returnValue += template + 'const char * ' + specclname + '::Class_Name() {return "' + clname[2:] + '";}\n' @@ -2814,7 +2819,7 @@ def ClassDefImplementation(selclasses, self) : returnValue += ' b.WriteClassBuffer(' + clname + '::Class(),this);\n' returnValue += ' }\n' returnValue += '}\n' - returnValue += template + 'TClass* ' + specclname + '::fgIsA = 0;\n' + returnValue += template + 'atomic_TClass_ptr ' + specclname + '::fgIsA(0);\n' returnValue += namespacelevel * '}' + '\n' elif derivesFromTObject : # no fgIsA etc members but derives from TObject! diff --git a/core/base/inc/Rtypes.h b/core/base/inc/Rtypes.h index 345e82a30f1ee..7acc10bea8c1a 100644 --- a/core/base/inc/Rtypes.h +++ b/core/base/inc/Rtypes.h @@ -34,7 +34,9 @@ #include #include // part of stdio.h on systems that have it #include // part of string.h on systems that have it - +#if __cplusplus > 199711L +#include +#endif //---- forward declared class types -------------------------------------------- @@ -268,12 +270,17 @@ namespace ROOT { #include "TGenericClassInfo.h" #endif +#if __cplusplus > 199711L +typedef std::atomic atomic_TClass_ptr; +#else +typedef TClass* atomic_TClass_ptr; +#endif // Common part of ClassDef definition. // DeclFileLine() is not part of it since CINT uses that as trigger for // the class comment string. #define _ClassDef_(name,id) \ private: \ - static TClass *fgIsA; \ + static atomic_TClass_ptr fgIsA; \ public: \ static TClass *Class(); \ static const char *Class_Name(); \ @@ -290,7 +297,7 @@ public: \ // Version without any virtual functions. #define _ClassDefNV_(name,id) \ private: \ -static TClass *fgIsA; \ +static atomic_TClass_ptr fgIsA; \ public: \ static TClass *Class(); \ static const char *Class_Name(); \ diff --git a/core/utils/src/rootcint.cxx b/core/utils/src/rootcint.cxx index 43ff08dab1e82..ab5b862ae9e95 100644 --- a/core/utils/src/rootcint.cxx +++ b/core/utils/src/rootcint.cxx @@ -2398,7 +2398,7 @@ void WriteClassFunctions(G__ClassInfo &cl, int /*tmplt*/ = 0) (*dictSrcOut) << "//_______________________________________" << "_______________________________________" << std::endl; if (add_template_keyword) (*dictSrcOut) << "template <> "; - (*dictSrcOut) << "TClass *" << clsname.c_str() << "::fgIsA = 0; // static to hold class pointer" << std::endl + (*dictSrcOut) << "atomic_TClass_ptr " << clsname.c_str() << "::fgIsA(0); // static to hold class pointer" << std::endl << std::endl << "//_______________________________________" @@ -2433,8 +2433,8 @@ void WriteClassFunctions(G__ClassInfo &cl, int /*tmplt*/ = 0) << "_______________________________________" << std::endl; if (add_template_keyword) (*dictSrcOut) << "template <> "; (*dictSrcOut) << "TClass *" << clsname.c_str() << "::Class()" << std::endl << "{" << std::endl; - (*dictSrcOut) << " if (!fgIsA) fgIsA = ::ROOT::GenerateInitInstanceLocal((const ::"; - (*dictSrcOut) << cl.Fullname() << "*)0x0)->GetClass();" << std::endl + (*dictSrcOut) << " if (!fgIsA) { R__LOCKGUARD2(gCINTMutex); if(!fgIsA) {fgIsA = ::ROOT::GenerateInitInstanceLocal((const ::"; + (*dictSrcOut) << cl.Fullname() << "*)0x0)->GetClass();} }" << std::endl << " return fgIsA;" << std::endl << "}" << std::endl << std::endl; @@ -4942,6 +4942,8 @@ int main(int argc, char **argv) (*dictSrcOut) << "#include \"TClass.h\"" << std::endl << "#include \"TBuffer.h\"" << std::endl << "#include \"TMemberInspector.h\"" << std::endl + << "#include \"TInterpreter.h\"" << std::endl + << "#include \"TVirtualMutex.h\"" << std::endl << "#include \"TError.h\"" << std::endl << std::endl << "#ifndef G__ROOT" << std::endl << "#define G__ROOT" << std::endl diff --git a/core/utils/src/rootcling.cxx b/core/utils/src/rootcling.cxx index 4ea7ae2a79e0f..46acb8318d806 100644 --- a/core/utils/src/rootcling.cxx +++ b/core/utils/src/rootcling.cxx @@ -3483,7 +3483,7 @@ void WriteClassFunctions(const clang::CXXRecordDecl *cl) (*dictSrcOut) << "//_______________________________________" << "_______________________________________" << std::endl; if (add_template_keyword) (*dictSrcOut) << "template <> "; - (*dictSrcOut) << "TClass *" << clsname.c_str() << "::fgIsA = 0; // static to hold class pointer" << std::endl + (*dictSrcOut) << "atomic_TClass_ptr " << clsname.c_str() << "::fgIsA(0); // static to hold class pointer" << std::endl << std::endl << "//_______________________________________" @@ -3518,8 +3518,8 @@ void WriteClassFunctions(const clang::CXXRecordDecl *cl) << "_______________________________________" << std::endl; if (add_template_keyword) (*dictSrcOut) << "template <> "; (*dictSrcOut) << "TClass *" << clsname.c_str() << "::Class()" << std::endl << "{" << std::endl; - (*dictSrcOut) << " if (!fgIsA) fgIsA = ::ROOT::GenerateInitInstanceLocal((const ::"; - (*dictSrcOut) << fullname.c_str() << "*)0x0)->GetClass();" << std::endl + (*dictSrcOut) << " if (!fgIsA) { R__LOCKGUARD2(gCINTMutex); if(!fgIsA) {fgIsA = ::ROOT::GenerateInitInstanceLocal((const ::"; + (*dictSrcOut) << fullname.c_str() << "*)0x0)->GetClass();} }" << std::endl << " return fgIsA;" << std::endl << "}" << std::endl << std::endl; @@ -6514,6 +6514,8 @@ int main(int argc, char **argv) << "#include \"TCintWithCling.h\"" << std::endl << "#include \"TBuffer.h\"" << std::endl << "#include \"TMemberInspector.h\"" << std::endl + << "#include \"TInterpreter.h\"" << std::endl + << "#include \"TVirtualMutex.h\"" << std::endl << "#include \"TError.h\"" << std::endl << std::endl << "#ifndef G__ROOT" << std::endl << "#define G__ROOT" << std::endl From 0266347d1739445db7a1c46ead9e5389da9e89c1 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 14 Feb 2014 00:17:05 +0100 Subject: [PATCH 37/38] Fix caching bug in ROOTClassEnhancer::IsA When looking up the value of a cache in the map, it was possible to temporarily have fLastClass and fLastType out of synch and cause the program to return the wrong TClass. --- cint/cintex/src/ROOTClassEnhancer.cxx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cint/cintex/src/ROOTClassEnhancer.cxx b/cint/cintex/src/ROOTClassEnhancer.cxx index 68187aa2e6b1d..66d46ff0a2e66 100644 --- a/cint/cintex/src/ROOTClassEnhancer.cxx +++ b/cint/cintex/src/ROOTClassEnhancer.cxx @@ -393,7 +393,9 @@ namespace ROOT { namespace Cintex { } // Check if TypeNth is already in sub-class cache - if ( 0 != (fLastClass=fSub_types[&typ]) ) { + TClass* findClass = fSub_types[&typ]; + if ( 0 != findClass ) { + fLastClass = findClass; fLastType = &typ; return fLastClass; } From 44e5441eaa5fc4249937264ebd4f3791792ab2e9 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 14 Feb 2014 00:19:38 +0100 Subject: [PATCH 38/38] Avoid holding gCINTMutex and gROOTMutex simultaneously in TPluginManager By having a TObjArray be deleted outside of the gCINTMutex lock we no longer have both mutexes being held at the same time and therefore we avoid potential deadlocks. --- core/base/src/TPluginManager.cxx | 110 ++++++++++++++++--------------- 1 file changed, 57 insertions(+), 53 deletions(-) diff --git a/core/base/src/TPluginManager.cxx b/core/base/src/TPluginManager.cxx index f218e7abccde6..a549fdb71e6b6 100644 --- a/core/base/src/TPluginManager.cxx +++ b/core/base/src/TPluginManager.cxx @@ -472,68 +472,72 @@ void TPluginManager::LoadHandlersFromPluginDirs(const char *base) // dependency, check on some OS or ROOT capability or downloading // of the plugin. - R__LOCKGUARD2(gCINTMutex); - if (!fBasesLoaded) { - fBasesLoaded = new THashTable(); - fBasesLoaded->SetOwner(); + //The destructor of TObjArray takes the gROOTMutex lock so we want to + // delete the object after release the gCINTMutex lock + TObjArray *dirs = nullptr; + { + R__LOCKGUARD2(gCINTMutex); + if (!fBasesLoaded) { + fBasesLoaded = new THashTable(); + fBasesLoaded->SetOwner(); - // make sure we have gPluginMgr availble in the plugin macros - gInterpreter->InitializeDictionaries(); - } - TString sbase = base; - if (sbase != "") { - sbase.ReplaceAll("::", "@@"); - if (fBasesLoaded->FindObject(sbase)) - return; - fBasesLoaded->Add(new TObjString(sbase)); - } + // make sure we have gPluginMgr availble in the plugin macros + gInterpreter->InitializeDictionaries(); + } + TString sbase = base; + if (sbase != "") { + sbase.ReplaceAll("::", "@@"); + if (fBasesLoaded->FindObject(sbase)) + return; + fBasesLoaded->Add(new TObjString(sbase)); + } - fgReadingDirs = kTRUE; + fgReadingDirs = kTRUE; - TString plugindirs = gEnv->GetValue("Root.PluginPath", (char*)0); + TString plugindirs = gEnv->GetValue("Root.PluginPath", (char*)0); #ifdef WIN32 - TObjArray *dirs = plugindirs.Tokenize(";"); + dirs = plugindirs.Tokenize(";"); #else - TObjArray *dirs = plugindirs.Tokenize(":"); + dirs = plugindirs.Tokenize(":"); #endif - TString d; - for (Int_t i = 0; i < dirs->GetEntriesFast(); i++) { - d = ((TObjString*)dirs->At(i))->GetString(); - // check if directory already scanned - Int_t skip = 0; - for (Int_t j = 0; j < i; j++) { - TString pd = ((TObjString*)dirs->At(j))->GetString(); - if (pd == d) { - skip++; - break; - } - } - if (!skip) { - if (sbase != "") { - const char *p = gSystem->ConcatFileName(d, sbase); - LoadHandlerMacros(p); - delete [] p; - } else { - void *dirp = gSystem->OpenDirectory(d); - if (dirp) { - if (gDebug > 0) - Info("LoadHandlersFromPluginDirs", "%s", d.Data()); - const char *f1; - while ((f1 = gSystem->GetDirEntry(dirp))) { - TString f = f1; - const char *p = gSystem->ConcatFileName(d, f); - LoadHandlerMacros(p); - fBasesLoaded->Add(new TObjString(f)); - delete [] p; - } - } - gSystem->FreeDirectory(dirp); - } + TString d; + for (Int_t i = 0; i < dirs->GetEntriesFast(); i++) { + d = ((TObjString*)dirs->At(i))->GetString(); + // check if directory already scanned + Int_t skip = 0; + for (Int_t j = 0; j < i; j++) { + TString pd = ((TObjString*)dirs->At(j))->GetString(); + if (pd == d) { + skip++; + break; + } + } + if (!skip) { + if (sbase != "") { + const char *p = gSystem->ConcatFileName(d, sbase); + LoadHandlerMacros(p); + delete [] p; + } else { + void *dirp = gSystem->OpenDirectory(d); + if (dirp) { + if (gDebug > 0) + Info("LoadHandlersFromPluginDirs", "%s", d.Data()); + const char *f1; + while ((f1 = gSystem->GetDirEntry(dirp))) { + TString f = f1; + const char *p = gSystem->ConcatFileName(d, f); + LoadHandlerMacros(p); + fBasesLoaded->Add(new TObjString(f)); + delete [] p; + } + } + gSystem->FreeDirectory(dirp); + } + } } + fgReadingDirs = kFALSE; } - delete dirs; - fgReadingDirs = kFALSE; } //______________________________________________________________________________