From 27484e2c82d8b3b98322e1d758b9caf312231c9c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 19 May 2016 10:39:25 -0700 Subject: [PATCH] Inliner: locks for xml read/write access Move CritSecObject into util.h, and use it to lock around reading and writing inline Xml. Introduce CritSecHolder for RAII management of the locks. Add a simple file position cache for methods to speed up replay when the inline xml file is large. --- src/jit/compiler.cpp | 40 ++++++++++++------------- src/jit/compiler.h | 37 ----------------------- src/jit/inline.cpp | 11 ++++++- src/jit/inline.h | 21 +++++++++++-- src/jit/inlinepolicy.cpp | 78 +++++++++++++++++++++++++++++++++++------------- src/jit/inlinepolicy.h | 7 +++-- src/jit/utils.h | 74 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 185 insertions(+), 83 deletions(-) diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp index 529c4a5..ddd6d4a 100644 --- a/src/jit/compiler.cpp +++ b/src/jit/compiler.cpp @@ -4348,15 +4348,19 @@ void Compiler::compCompileFinish() #endif #if MEASURE_MEM_ALLOC - ClrEnterCriticalSection(s_memStatsLock.Val()); - genMemStats.nraTotalSizeAlloc = compGetAllocator()->getTotalBytesAllocated(); - genMemStats.nraTotalSizeUsed = compGetAllocator()->getTotalBytesUsed(); - s_aggMemStats.Add(genMemStats); - if (genMemStats.allocSz > s_maxCompMemStats.allocSz) { - s_maxCompMemStats = genMemStats; + // Grab the relevant lock. + CritSecHolder statsLock(s_memStatsLock); + + // Make the updates. + genMemStats.nraTotalSizeAlloc = compGetAllocator()->getTotalBytesAllocated(); + genMemStats.nraTotalSizeUsed = compGetAllocator()->getTotalBytesUsed(); + s_aggMemStats.Add(genMemStats); + if (genMemStats.allocSz > s_maxCompMemStats.allocSz) + { + s_maxCompMemStats = genMemStats; + } } - ClrLeaveCriticalSection(s_memStatsLock.Val()); #ifdef DEBUG if (s_dspMemStats || verbose) @@ -6349,7 +6353,7 @@ void CompTimeSummaryInfo::AddInfo(CompTimeInfo& info) { if (info.m_timerFailure) return; // Don't update if there was a failure. - ClrEnterCriticalSection(s_compTimeSummaryLock.Val()); + CritSecHolder timeLock(s_compTimeSummaryLock); m_numMethods++; bool includeInFiltered = IncludedInFilteredData(info); @@ -6381,8 +6385,6 @@ void CompTimeSummaryInfo::AddInfo(CompTimeInfo& info) } m_total.m_parentPhaseEndSlop += info.m_parentPhaseEndSlop; m_maximum.m_parentPhaseEndSlop = max(m_maximum.m_parentPhaseEndSlop, info.m_parentPhaseEndSlop); - - ClrLeaveCriticalSection(s_compTimeSummaryLock.Val()); } // Static @@ -6553,7 +6555,8 @@ void JitTimer::PrintCsvHeader() return; } - ClrEnterCriticalSection(s_csvLock.Val()); + CritSecHolder csvLock(s_csvLock); + if (_waccess(jitTimeLogCsv, 0) == -1) { // File doesn't exist, so create it and write the header @@ -6575,7 +6578,6 @@ void JitTimer::PrintCsvHeader() fprintf(fp, "\"CPS\"\n"); fclose(fp); } - ClrLeaveCriticalSection(s_csvLock.Val()); } void JitTimer::PrintCsvMethodStats(Compiler* comp) @@ -6589,7 +6591,7 @@ void JitTimer::PrintCsvMethodStats(Compiler* comp) // eeGetMethodFullName uses locks, so don't enter crit sec before this call. const char* methName = comp->eeGetMethodFullName(comp->info.compMethodHnd); - ClrEnterCriticalSection(s_csvLock.Val()); + CritSecHolder csvLock(s_csvLock); FILE* fp = _wfopen(jitTimeLogCsv, W("a")); fprintf(fp, "\"%s\",", methName); @@ -6607,8 +6609,6 @@ void JitTimer::PrintCsvMethodStats(Compiler* comp) fprintf(fp, "%I64u,", m_info.m_totalCycles); fprintf(fp, "%f\n", CycleTimer::CyclesPerSecond()); fclose(fp); - - ClrLeaveCriticalSection(s_csvLock.Val()); } // Completes the timing of the current method, and adds it to "sum". @@ -6717,11 +6717,11 @@ void Compiler::PrintAggregateLoopHoistStats(FILE* f) void Compiler::AddLoopHoistStats() { - ClrEnterCriticalSection(s_loopHoistStatsLock.Val()); - s_loopsConsidered += m_loopsConsidered; - s_loopsWithHoistedExpressions += m_loopsWithHoistedExpressions; - s_totalHoistedExpressions += m_totalHoistedExpressions; - ClrLeaveCriticalSection(s_loopHoistStatsLock.Val()); + CritSecHolder statsLock(s_loopHoistStatsLock); + + s_loopsConsidered += m_loopsConsidered; + s_loopsWithHoistedExpressions += m_loopsWithHoistedExpressions; + s_totalHoistedExpressions += m_totalHoistedExpressions; } void Compiler::PrintPerMethodLoopHoistStats() diff --git a/src/jit/compiler.h b/src/jit/compiler.h index fd90ff6..edcc951 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -873,43 +873,6 @@ struct CompTimeInfo #endif }; -// TBD: Move this to UtilCode. - -// The CLR requires that critical section locks be initialized via its ClrCreateCriticalSection API...but -// that can't be called until the CLR is initialized. If we have static data that we'd like to protect by a -// lock, and we have a statically allocated lock to protect that data, there's an issue in how to initialize -// that lock. We could insert an initialize call in the startup path, but one might prefer to keep the code -// more local. For such situations, CritSecObject solves the initialization problem, via a level of -// indirection. A pointer to the lock is initially null, and when we query for the lock pointer via "Val()". -// If the lock has not yet been allocated, this allocates one (here a leaf lock), and uses a -// CompareAndExchange-based lazy-initialization to update the field. If this fails, the allocated lock is -// destroyed. This will work as long as the first locking attempt occurs after enough CLR initialization has -// happened to make ClrCreateCriticalSection calls legal. -class CritSecObject -{ - // CRITSEC_COOKIE is an opaque pointer type. - CRITSEC_COOKIE m_pCs; -public: - CritSecObject() - { - m_pCs = NULL; - } - CRITSEC_COOKIE Val() - { - if (m_pCs == NULL) - { - // CompareExchange-based lazy init. - CRITSEC_COOKIE newCs = ClrCreateCriticalSection(CrstLeafLock, CRST_DEFAULT); - CRITSEC_COOKIE observed = InterlockedCompareExchangeT(&m_pCs, newCs, NULL); - if (observed != NULL) - { - ClrDeleteCriticalSection(newCs); - } - } - return m_pCs; - } -}; - #ifdef FEATURE_JIT_METHOD_PERF // This class summarizes the JIT time information over the course of a run: the number of methods compiled, diff --git a/src/jit/inline.cpp b/src/jit/inline.cpp index 5cea745..3bb1be7 100644 --- a/src/jit/inline.cpp +++ b/src/jit/inline.cpp @@ -723,6 +723,10 @@ InlineStrategy::InlineStrategy(Compiler* compiler) , m_InitialSizeEstimate(0) , m_CurrentSizeEstimate(0) , m_HasForceViaDiscretionary(false) +#if defined(DEBUG) || defined(INLINE_DATA) + , m_MethodXmlFilePosition(0) +#endif // defined(DEBUG) || defined(INLINE_DATA) + { // Verify compiler is a root compiler instance assert(m_Compiler->impInlineRoot() == m_Compiler); @@ -1280,8 +1284,10 @@ void InlineStrategy::DumpData() } // Static to track emission of the xml data header +// and lock to prevent interleaved file writes -bool InlineStrategy::s_HasDumpedXmlHeader = false; +bool InlineStrategy::s_HasDumpedXmlHeader = false; +CritSecObject InlineStrategy::s_XmlWriterLock; //------------------------------------------------------------------------ // DumpXml: dump xml-formatted version of the inline tree. @@ -1297,6 +1303,9 @@ void InlineStrategy::DumpXml(FILE* file, unsigned indent) return; } + // Lock to prevent interleaving of trees. + CritSecHolder writeLock(s_XmlWriterLock); + // Dump header if (!s_HasDumpedXmlHeader) { diff --git a/src/jit/inline.h b/src/jit/inline.h index cac0fc8..15ff35c 100644 --- a/src/jit/inline.h +++ b/src/jit/inline.h @@ -736,6 +736,17 @@ public: void DumpXml(FILE* file = stderr, unsigned indent = 0); static void FinalizeXml(FILE* file = stderr); + // Cache for file position of this method in the inline xml + long GetMethodXmlFilePosition() + { + return m_MethodXmlFilePosition; + } + + void SetMethodXmlFilePosition(long val) + { + m_MethodXmlFilePosition = val; + } + #endif // defined(DEBUG) || defined(INLINE_DATA) // Some inline limit values @@ -773,8 +784,9 @@ private: int EstimateSize(InlineContext* context); #if defined(DEBUG) || defined(INLINE_DATA) - static bool s_HasDumpedDataHeader; - static bool s_HasDumpedXmlHeader; + static bool s_HasDumpedDataHeader; + static bool s_HasDumpedXmlHeader; + static CritSecObject s_XmlWriterLock; #endif // defined(DEBUG) || defined(INLINE_DATA) Compiler* m_Compiler; @@ -792,6 +804,11 @@ private: int m_InitialSizeEstimate; int m_CurrentSizeEstimate; bool m_HasForceViaDiscretionary; + +#if defined(DEBUG) || defined(INLINE_DATA) + long m_MethodXmlFilePosition; +#endif // defined(DEBUG) || defined(INLINE_DATA) + }; #endif // _INLINE_H_ diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp index d684bfa..29fb2c5 100644 --- a/src/jit/inlinepolicy.cpp +++ b/src/jit/inlinepolicy.cpp @@ -2066,8 +2066,12 @@ void SizePolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) return; } -bool ReplayPolicy::s_WroteReplayBanner = false; -FILE* ReplayPolicy::s_ReplayFile = nullptr; +// Statics to track emission of the replay banner +// and provide file access to the inline xml + +bool ReplayPolicy::s_WroteReplayBanner = false; +FILE* ReplayPolicy::s_ReplayFile = nullptr; +CritSecObject ReplayPolicy::s_XmlReaderLock; //------------------------------------------------------------------------/ // ReplayPolicy: construct a new ReplayPolicy @@ -2122,17 +2126,35 @@ void ReplayPolicy::FinalizeXml() bool ReplayPolicy::FindMethod() { + if (s_ReplayFile == nullptr) + { + return false; + } + + // See if we've already found this method. + InlineStrategy* inlineStrategy = m_RootCompiler->m_inlineStrategy; + long filePosition = inlineStrategy->GetMethodXmlFilePosition(); + + if (filePosition == -1) + { + // Past lookup failed + return false; + } + else if (filePosition > 0) + { + // Past lookup succeeded, jump there + fseek(s_ReplayFile, filePosition, SEEK_SET); + return true; + } + + // Else, scan the file. Might be nice to build an index + // or something, someday. const mdMethodDef methodToken = m_RootCompiler->info.compCompHnd->getMethodDefFromMethod( m_RootCompiler->info.compMethodHnd); const unsigned methodHash = m_RootCompiler->info.compMethodHash(); - if (s_ReplayFile == nullptr) - { - return false; - } - bool foundMethod = false; char buffer[256]; fseek(s_ReplayFile, 0, SEEK_SET); @@ -2184,6 +2206,16 @@ bool ReplayPolicy::FindMethod() break; } + // Update file position cache for this method + long foundPosition = -1; + + if (foundMethod) + { + foundPosition = ftell(s_ReplayFile); + } + + inlineStrategy->SetMethodXmlFilePosition(foundPosition); + return foundMethod; } @@ -2406,26 +2438,32 @@ void ReplayPolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) // the don't inline. bool accept = false; - // First, locate the entries for the root method. - bool foundMethod = FindMethod(); - - if (foundMethod && (m_InlineContext != nullptr)) + // Grab the reader lock, since we'll be manipulating + // the file pointer as we look for the relevant inline xml. { - // Next, navigate the context tree to find the entries - // for the context that contains this candidate. - bool foundContext = FindContext(m_InlineContext); + CritSecHolder readerLock(s_XmlReaderLock); + + // First, locate the entries for the root method. + bool foundMethod = FindMethod(); - if (foundContext) + if (foundMethod && (m_InlineContext != nullptr)) { - // Finally, find this candidate within its context - CORINFO_METHOD_HANDLE calleeHandle = methodInfo->ftn; - accept = FindInline(calleeHandle); + // Next, navigate the context tree to find the entries + // for the context that contains this candidate. + bool foundContext = FindContext(m_InlineContext); + + if (foundContext) + { + // Finally, find this candidate within its context + CORINFO_METHOD_HANDLE calleeHandle = methodInfo->ftn; + accept = FindInline(calleeHandle); + } } } if (accept) { - JITLOG_THIS(m_RootCompiler, (LL_INFO100000, "Inline accepted via log replay")) + JITLOG_THIS(m_RootCompiler, (LL_INFO100000, "Inline accepted via log replay")); if (m_IsPrejitRoot) { @@ -2438,7 +2476,7 @@ void ReplayPolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) } else { - JITLOG_THIS(m_RootCompiler, (LL_INFO100000, "Inline rejected via log replay")) + JITLOG_THIS(m_RootCompiler, (LL_INFO100000, "Inline rejected via log replay")); if (m_IsPrejitRoot) { diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h index f8bb34a..21bcf51 100644 --- a/src/jit/inlinepolicy.h +++ b/src/jit/inlinepolicy.h @@ -363,9 +363,10 @@ private: bool FindInline(CORINFO_METHOD_HANDLE callee); bool FindInline(unsigned token, unsigned hash); - static bool s_WroteReplayBanner; - static FILE* s_ReplayFile; - InlineContext* m_InlineContext; + static bool s_WroteReplayBanner; + static FILE* s_ReplayFile; + static CritSecObject s_XmlReaderLock; + InlineContext* m_InlineContext; }; #endif // defined(DEBUG) || defined(INLINE_DATA) diff --git a/src/jit/utils.h b/src/jit/utils.h index db77568..af9f2db 100644 --- a/src/jit/utils.h +++ b/src/jit/utils.h @@ -578,4 +578,78 @@ public: static double round(double d); }; + +// The CLR requires that critical section locks be initialized via its ClrCreateCriticalSection API...but +// that can't be called until the CLR is initialized. If we have static data that we'd like to protect by a +// lock, and we have a statically allocated lock to protect that data, there's an issue in how to initialize +// that lock. We could insert an initialize call in the startup path, but one might prefer to keep the code +// more local. For such situations, CritSecObject solves the initialization problem, via a level of +// indirection. A pointer to the lock is initially null, and when we query for the lock pointer via "Val()". +// If the lock has not yet been allocated, this allocates one (here a leaf lock), and uses a +// CompareAndExchange-based lazy-initialization to update the field. If this fails, the allocated lock is +// destroyed. This will work as long as the first locking attempt occurs after enough CLR initialization has +// happened to make ClrCreateCriticalSection calls legal. + +class CritSecObject +{ +public: + + CritSecObject() + { + m_pCs = NULL; + } + + CRITSEC_COOKIE Val() + { + if (m_pCs == NULL) + { + // CompareExchange-based lazy init. + CRITSEC_COOKIE newCs = ClrCreateCriticalSection(CrstLeafLock, CRST_DEFAULT); + CRITSEC_COOKIE observed = InterlockedCompareExchangeT(&m_pCs, newCs, NULL); + if (observed != NULL) + { + ClrDeleteCriticalSection(newCs); + } + } + return m_pCs; + } + +private: + + // CRITSEC_COOKIE is an opaque pointer type. + CRITSEC_COOKIE m_pCs; + + // No copying or assignment allowed. + CritSecObject(const CritSecObject&) = delete; + CritSecObject& operator=(const CritSecObject&) = delete; +}; + +// Stack-based holder for a critial section lock. +// Ensures lock is released. + +class CritSecHolder +{ +public: + + CritSecHolder(CritSecObject& critSec) + : m_CritSec(critSec) + { + ClrEnterCriticalSection(m_CritSec.Val()); + } + + ~CritSecHolder() + { + ClrLeaveCriticalSection(m_CritSec.Val()); + } + +private: + + CritSecObject& m_CritSec; + + // No copying or assignment allowed. + CritSecHolder(const CritSecHolder&) = delete; + CritSecHolder& operator=(const CritSecHolder&) = delete; +}; + + #endif // _UTILS_H_ -- 2.7.4