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.

7 files changed, 185 insertions, 83 deletions

diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp
index 529c4a5b56..ddd6d4a706 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 fd90ff6b73..edcc951fa1 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 5cea745cd5..3bb1be78e2 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 cac0fc8145..15ff35c8b5 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 d684bfa266..29fb2c5b8f 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 f8bb34a845..21bcf51193 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 db7756809b..af9f2db009 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_