Reduce custom operator behavior in the Volatile<T> structure

src/coreclr/gc/env/volatile.h

@@ -287,12 +287,6 @@ void VolatileStoreWithoutBarrier(T* pt, T val)
 // cast the result to a float.  In general, calling Load or Store explicitly will work around
 // any problems that can't be solved by operator overloading.
 //
-// @TODO: it's not clear that we actually *want* any operator overloading here.  It's in here primarily
-// to ease the task of converting all of the old uses of the volatile keyword, but in the long
-// run it's probably better if users of this class are forced to call Load() and Store() explicitly.
-// This would make it much more clear where the memory barriers are, and which operations are actually
-// being performed, but it will have to wait for another cleanup effort.
-//
 template <typename T>
 class Volatile
 {
@@ -395,45 +389,6 @@ class Volatile
     // expects a normal pointer.
     //
     inline constexpr T volatile * operator&() {return this->GetPointer();}
-    inline constexpr T volatile const * operator&() const {return this->GetPointer();}
-
-    //
-    // Comparison operators
-    //
-    template<typename TOther>
-    inline bool operator==(const TOther& other) const {return this->Load() == other;}
-
-    template<typename TOther>
-    inline bool operator!=(const TOther& other) const {return this->Load() != other;}
-
-    //
-    // Miscellaneous operators.  Add more as necessary.
-    //
-	inline Volatile<T>& operator+=(T val) {Store(this->Load() + val); return *this;}
-	inline Volatile<T>& operator-=(T val) {Store(this->Load() - val); return *this;}
-    inline Volatile<T>& operator|=(T val) {Store(this->Load() | val); return *this;}
-    inline Volatile<T>& operator&=(T val) {Store(this->Load() & val); return *this;}
-    inline bool operator!() const { return !this->Load();}
-
-    //
-    // Prefix increment
-    //
-    inline Volatile& operator++() {this->Store(this->Load()+1); return *this;}
-
-    //
-    // Postfix increment
-    //
-    inline T operator++(int) {T val = this->Load(); this->Store(val+1); return val;}
-
-    //
-    // Prefix decrement
-    //
-    inline Volatile& operator--() {this->Store(this->Load()-1); return *this;}
-
-    //
-    // Postfix decrement
-    //
-    inline T operator--(int) {T val = this->Load(); this->Store(val-1); return val;}
 };
 
 //

src/coreclr/gc/gc.cpp

@@ -8129,7 +8129,7 @@ void gc_heap::fix_allocation_context_heaps (gc_alloc_context* gc_context, void*)
         alloc_hp_num %= gc_heap::n_heaps;
         acontext->set_alloc_heap (GCHeap::GetHeap (alloc_hp_num));
         gc_heap* hp = acontext->get_alloc_heap ()->pGenGCHeap;
-        hp->alloc_context_count++;
+        hp->alloc_context_count = hp->alloc_context_count + 1;
     }
 }
 
@@ -19223,7 +19223,7 @@ void gc_heap::balance_heaps (alloc_context* acontext)
             acontext->set_home_heap (GCHeap::GetHeap (home_hp_num));
             gc_heap* hp = acontext->get_home_heap ()->pGenGCHeap;
             acontext->set_alloc_heap (acontext->get_home_heap ());
-            hp->alloc_context_count++;
+            hp->alloc_context_count = hp->alloc_context_count + 1;
 
 #ifdef HEAP_BALANCE_INSTRUMENTATION
             uint16_t ideal_proc_no = 0;
@@ -19476,8 +19476,13 @@ void gc_heap::balance_heaps (alloc_context* acontext)
                 {
                     final_alloc_hp_num = max_hp->heap_number;
 
-                    org_hp->alloc_context_count--;
-                    max_hp->alloc_context_count++;
+                    // update the alloc_context_count for the original and new heaps.
+                    // NOTE: at this time the alloc_context_count for these heaps could have changed due to racing threads,
+                    //       but we will update the counts based on what we observed, without trying to re-check or 
+                    //       synchronize, as this is just a heuristic to improve our balancing, and doesn't need to 
+                    //       be perfectly accurate.
+                    org_hp->alloc_context_count = org_hp->alloc_context_count - 1;
+                    max_hp->alloc_context_count = max_hp->alloc_context_count + 1;
 
                     acontext->set_alloc_heap (GCHeap::GetHeap (final_alloc_hp_num));
                     if (!gc_thread_no_affinitize_p)
@@ -24206,7 +24211,7 @@ void gc_heap::pm_full_gc_init_or_clear()
             // Although arguably we should just turn off PM then...
             //assert (settings.entry_memory_load >= high_memory_load_th);
             assert (settings.entry_memory_load > 0);
-            settings.gc_index += 1;
+            settings.gc_index = settings.gc_index + 1;
             do_pre_gc();
         }
     }

src/coreclr/gc/gcpriv.h

@@ -3764,6 +3764,8 @@ class gc_heap
     PER_HEAP_FIELD_SINGLE_GC_ALLOC BOOL last_gc_before_oom;
 
 #ifdef MULTIPLE_HEAPS
+    // approximate alloc_context_count. This is updated by multiple threads without locking; the
+    // increments/decrements are non-interlocked so the value is only approximate.
     PER_HEAP_FIELD_SINGLE_GC_ALLOC VOLATILE(int) alloc_context_count;
 
     // Init-ed during a GC and updated by allocator after that GC

src/coreclr/inc/volatile.h

@@ -336,12 +336,6 @@ void VolatileLoadBarrier()
 // cast the result to a float.  In general, calling Load or Store explicitly will work around
 // any problems that can't be solved by operator overloading.
 //
-// @TODO: it's not clear that we actually *want* any operator overloading here.  It's in here primarily
-// to ease the task of converting all of the old uses of the volatile keyword, but in the long
-// run it's probably better if users of this class are forced to call Load() and Store() explicitly.
-// This would make it much more clear where the memory barriers are, and which operations are actually
-// being performed, but it will have to wait for another cleanup effort.
-//
 template <typename T>
 class Volatile
 {
@@ -454,45 +448,6 @@ class Volatile
     // expects a normal pointer.
     //
     inline constexpr T volatile * operator&() {return this->GetPointer();}
-    inline constexpr T volatile const * operator&() const {return this->GetPointer();}
-
-    //
-    // Comparison operators
-    //
-    template<typename TOther>
-    inline bool operator==(const TOther& other) const {return this->Load() == other;}
-
-    template<typename TOther>
-    inline bool operator!=(const TOther& other) const {return this->Load() != other;}
-
-    //
-    // Miscellaneous operators.  Add more as necessary.
-    //
-	inline Volatile<T>& operator+=(T val) {Store(this->Load() + val); return *this;}
-	inline Volatile<T>& operator-=(T val) {Store(this->Load() - val); return *this;}
-    inline Volatile<T>& operator|=(T val) {Store(this->Load() | val); return *this;}
-    inline Volatile<T>& operator&=(T val) {Store(this->Load() & val); return *this;}
-    inline bool operator!() const { STATIC_CONTRACT_SUPPORTS_DAC; return !this->Load();}
-
-    //
-    // Prefix increment
-    //
-    inline Volatile& operator++() {this->Store(this->Load()+1); return *this;}
-
-    //
-    // Postfix increment
-    //
-    inline T operator++(int) {T val = this->Load(); this->Store(val+1); return val;}
-
-    //
-    // Prefix decrement
-    //
-    inline Volatile& operator--() {this->Store(this->Load()-1); return *this;}
-
-    //
-    // Postfix decrement
-    //
-    inline T operator--(int) {T val = this->Load(); this->Store(val-1); return val;}
 };
 
 //

src/coreclr/pal/src/include/pal/init.h

@@ -52,6 +52,9 @@ Returns TRUE if the PAL is in an initialized state
 Warning : this will only report the PAL's state at the moment it is called.
 If it is necessary to ensure the PAL remains initialized (or not) while doing
 some work, the Initialization lock (PALInitLock()) should be held.
+
+Note: init_count is Volatile<int> which means that the read here has a read barrier
+so readers can be assured that the initialized PAL structures are fully initialized when this returns true.
 --*/
 #define PALIsInitialized() (0 < init_count)
 

src/coreclr/pal/src/include/pal/synchcache.hpp

@@ -29,9 +29,9 @@ namespace CorUnix
 
         static const int MaxDepth = 256;
 
-        Volatile<USynchCacheStackNode*> m_pHead;
+        USynchCacheStackNode* m_pHead;
         minipal_mutex m_cs;
-        Volatile<int> m_iDepth;
+        int m_iDepth;
         int m_iMaxDepth;
 #ifdef _DEBUG
         int m_iMaxTrackedDepth;
@@ -158,6 +158,7 @@ namespace CorUnix
             Unlock(pthrCurrent);
         }
 
+private:
         void Flush(CPalThread * pthrCurrent, bool fDontLock = false)
         {
             USynchCacheStackNode * pNode, * pTemp;
@@ -204,9 +205,9 @@ namespace CorUnix
                                               // instances and store them into the
                                               // cache before continuing
 
-        Volatile<USHRSynchCacheStackNode*> m_pHead;
+        USHRSynchCacheStackNode* m_pHead;
         minipal_mutex m_cs;
-        Volatile<int> m_iDepth;
+        int m_iDepth;
         int m_iMaxDepth;
 #ifdef _DEBUG
         int m_iMaxTrackedDepth;
@@ -358,6 +359,7 @@ namespace CorUnix
             Unlock(pthrCurrent);
         }
 
+private:
         void Flush(CPalThread * pthrCurrent, bool fDontLock = false)
         {
             USHRSynchCacheStackNode * pNode, * pTemp;

src/coreclr/pal/src/include/pal/synchobjects.hpp

@@ -97,7 +97,7 @@ namespace CorUnix
 
         THREAD_STATE           m_tsThreadState;
         SharedID               m_shridWaitAwakened;
-        Volatile<LONG>         m_lLocalSynchLockCount;
+        LONG                   m_lLocalSynchLockCount;
         LIST_ENTRY             m_leOwnedObjsList;
 
         ThreadNativeWaitData   m_tnwdNativeData;

src/coreclr/pal/src/init/pal.cpp

@@ -588,7 +588,9 @@ Initialize(
         }
 
         TRACE("First-time PAL initialization complete.\n");
-        init_count++;
+        // Incrementing the init_count here serves as a synchronization point,
+        // since it is a Volatile<T> variable, and modifying it will have release semantics.
+        init_count.Store(init_count.Load() + 1);
 
         /* Set LastError to a non-good value - functions within the
            PAL startup may set lasterror to a nonzero value. */
@@ -597,7 +599,7 @@ Initialize(
     }
     else
     {
-        init_count++;
+        init_count.Store(init_count.Load() + 1);
 
         TRACE("Initialization count increases to %d\n", init_count.Load());
 

src/coreclr/vm/ceeload.cpp

@@ -171,6 +171,7 @@ void Module::DoInit(AllocMemTracker *pamTracker, LPCWSTR szName)
 
 #endif
 }
+#endif //!DACCESS_COMPILE
 
 // Set the given bit on m_dwTransientFlags. Return true if we won the race to set the bit.
 BOOL Module::SetTransientFlagInterlocked(DWORD dwFlag)
@@ -187,6 +188,22 @@ BOOL Module::SetTransientFlagInterlocked(DWORD dwFlag)
     }
 }
 
+void Module::SetTransientFlagInterlockedWithMask(DWORD dwFlag, DWORD dwMask)
+{
+    LIMITED_METHOD_CONTRACT;
+
+    _ASSERTE((dwFlag & dwMask) == dwFlag);
+    for (;;)
+    {
+        DWORD dwTransientFlags = m_dwTransientFlags;
+        DWORD dwNewTransientFlags = (dwTransientFlags & ~dwMask) | dwFlag;
+        if ((DWORD)InterlockedCompareExchange((LONG*)&m_dwTransientFlags, dwNewTransientFlags, dwTransientFlags) == dwTransientFlags)
+            return;
+    }
+}
+
+#ifndef DACCESS_COMPILE
+
 #if defined(PROFILING_SUPPORTED) || defined(FEATURE_METADATA_UPDATER)
 void Module::UpdateNewlyAddedTypes()
 {
@@ -424,25 +441,25 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName)
     _ASSERTE(m_path != NULL);
     m_baseAddress = m_pPEAssembly->HasLoadedPEImage() ? m_pPEAssembly->GetLoadedLayout()->GetBase() : NULL;
     if (m_pPEAssembly->IsReflectionEmit())
-        m_dwTransientFlags |= IS_REFLECTION_EMIT;
+        m_dwTransientFlags = m_dwTransientFlags | IS_REFLECTION_EMIT;
 
     m_Crst.Init(CrstModule);
     m_LookupTableCrst.Init(CrstModuleLookupTable, CrstFlags(CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD));
     m_InstMethodHashTableCrst.Init(CrstInstMethodHashTable, CRST_REENTRANCY);
     m_ISymUnmanagedReaderCrst.Init(CrstISymUnmanagedReader, CRST_DEBUGGER_THREAD);
 
     AllocateMaps();
-    m_dwTransientFlags &= ~((DWORD)CLASSES_FREED);  // Set flag indicating LookupMaps are now in a consistent and destructable state
+    m_dwTransientFlags = m_dwTransientFlags & ~((DWORD)CLASSES_FREED);  // Set flag indicating LookupMaps are now in a consistent and destructable state
 
     if (IsSystem())
-        m_dwPersistedFlags |= SKIP_TYPE_VALIDATION; // Skip type validation on System
+        m_dwPersistedFlags = m_dwPersistedFlags | SKIP_TYPE_VALIDATION; // Skip type validation on System
 
 #ifdef FEATURE_READYTORUN
     m_pNativeImage = NULL;
     if ((m_pReadyToRunInfo = ReadyToRunInfo::Initialize(this, pamTracker)) != NULL)
     {
         if (m_pReadyToRunInfo->SkipTypeValidation())
-            m_dwPersistedFlags |= SKIP_TYPE_VALIDATION; // Skip type validation on System
+            m_dwPersistedFlags = m_dwPersistedFlags | SKIP_TYPE_VALIDATION; // Skip type validation on System
 
         m_pNativeImage = m_pReadyToRunInfo->GetNativeImage();
         if (m_pNativeImage != NULL)
@@ -489,11 +506,11 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName)
     // set profiler related JIT flags
     if (CORProfilerDisableInlining())
     {
-        m_dwTransientFlags |= PROF_DISABLE_INLINING;
+        m_dwTransientFlags = m_dwTransientFlags | PROF_DISABLE_INLINING;
     }
     if (CORProfilerDisableOptimizations())
     {
-        m_dwTransientFlags |= PROF_DISABLE_OPTIMIZATIONS;
+        m_dwTransientFlags = m_dwTransientFlags | PROF_DISABLE_OPTIMIZATIONS;
     }
 
     m_pJitInlinerTrackingMap = NULL;
@@ -517,8 +534,7 @@ void Module::SetDebuggerInfoBits(DebuggerAssemblyControlFlags newBits)
     _ASSERTE(((newBits << DEBUGGER_INFO_SHIFT_PRIV) &
               ~DEBUGGER_INFO_MASK_PRIV) == 0);
 
-    m_dwTransientFlags &= ~DEBUGGER_INFO_MASK_PRIV;
-    m_dwTransientFlags |= (newBits << DEBUGGER_INFO_SHIFT_PRIV);
+    SetTransientFlagInterlockedWithMask(newBits << DEBUGGER_INFO_SHIFT_PRIV, DEBUGGER_INFO_MASK_PRIV);
 
 #ifdef DEBUGGING_SUPPORTED
     if (IsEditAndContinueCapable())

src/coreclr/vm/ceeload.h

@@ -822,6 +822,9 @@ class Module : public ModuleBase
     // Set the given bit on m_dwTransientFlags. Return true if we won the race to set the bit.
     BOOL SetTransientFlagInterlocked(DWORD dwFlag);
 
+    // Set bits on the m_dwTransientFlags according to the given mask.
+    void SetTransientFlagInterlockedWithMask(DWORD dwFlag, DWORD dwMask);
+
     // Cannoically-cased hashtable of the available class names for
     // case insensitive lookup.  Contains pointers into
     // m_pAvailableClasses.
@@ -970,7 +973,7 @@ class Module : public ModuleBase
         SUPPORTS_DAC;
         _ASSERTE(IsEditAndContinueCapable());
         LOG((LF_ENC, LL_INFO100, "M:EnableEditAndContinue: this:%p, %s\n", this, GetDebugName()));
-        m_dwTransientFlags |= IS_EDIT_AND_CONTINUE;
+        SetTransientFlagInterlocked(IS_EDIT_AND_CONTINUE);
     }
 
 public:
@@ -1603,7 +1606,7 @@ class Module : public ModuleBase
     void SetIsRuntimeWrapExceptionsCached_ForReflectionEmitModules()
     {
         LIMITED_METHOD_CONTRACT;
-        m_dwPersistedFlags |= COMPUTED_WRAP_EXCEPTIONS;
+        m_dwPersistedFlags = m_dwPersistedFlags | COMPUTED_WRAP_EXCEPTIONS;
     }
 public:
 

src/coreclr/vm/ceemain.cpp

@@ -1242,7 +1242,7 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
         }
 
         // Indicate the EE is the shut down phase.
-        g_fEEShutDown |= ShutDown_Start;
+        InterlockedOr((LONG*)&g_fEEShutDown, ShutDown_Start);
 
         if (!IsAtProcessExit() && !g_fFastExitProcess)
         {
@@ -1367,7 +1367,7 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
             // lock -- after the OS has stopped all other threads.
             if (fIsDllUnloading && (g_fEEShutDown & ShutDown_Phase2) == 0)
             {
-                g_fEEShutDown |= ShutDown_Phase2;
+                InterlockedOr((LONG*)&g_fEEShutDown, ShutDown_Phase2);
 
                 if (!g_fFastExitProcess)
                 {
@@ -1601,7 +1601,7 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error.
                 {
                     if (GCHeapUtilities::IsGCInProgress())
                     {
-                        g_fEEShutDown |= ShutDown_Phase2;
+                        InterlockedOr((LONG*)&g_fEEShutDown, ShutDown_Phase2);
                         break;
                     }