Remove "Precious" concept for SyncBlocks now that it's nearly impossible to get into said state (and any case where it kicks in would be a bug)

Author: jkoritzinsky

src/coreclr/vm/comcallablewrapper.cpp

@@ -2102,8 +2102,6 @@ ComCallWrapper* ComCallWrapper::CreateWrapper(OBJECTREF* ppObj)
     // grab the sync block from the server
     SyncBlock* pSyncBlock = pServer->GetSyncBlock();
 
-    pSyncBlock->SetPrecious();
-
     // if the object belongs to a domain neutral class, need to allocate the wrapper in the default domain.
     // The object is potentially agile so if allocate out of the current domain and then hand out to
     // multiple domains we might never release the wrapper for that object and hence never unload the CCWC.

src/coreclr/vm/cominterfacemarshaler.cpp

@@ -124,9 +124,7 @@ void COMInterfaceMarshaler::CreateObjectRef(BOOL fDuplicate, OBJECTREF *pComObj,
         _ASSERTE(!"Creating a COM wrapper for WinRT delegates (which do not inherit from __ComObject) is not supported.");
     }
 
-    // make sure we "pin" the syncblock before switching to preemptive mode
     SyncBlock *pSB = (*pComObj)->GetSyncBlock();
-    pSB->SetPrecious();
     DWORD dwSyncBlockIndex = pSB->GetSyncBlockIndex();
 
     NewRCWHolder pNewRCW;

src/coreclr/vm/runtimecallablewrapper.cpp

@@ -377,7 +377,6 @@ OBJECTREF ComClassFactory::CreateAggregatedInstance(MethodTable* pMTClass, BOOL
 
         RCWCache* pCache = RCWCache::GetRCWCache();
 
-        _ASSERTE(cref->GetSyncBlock()->IsPrecious()); // the object already has a CCW
         DWORD dwSyncBlockIndex = cref->GetSyncBlockIndex();
 
         // create a wrapper for this COM object
@@ -1786,9 +1785,7 @@ void RCW::CreateDuplicateWrapper(MethodTable *pNewMT, RCWHolder* pNewRCW)
 
         DWORD flags = 0;
 
-        // make sure we "pin" the syncblock before switching to preemptive mode
         SyncBlock *pSB = NewWrapperObj->GetSyncBlock();
-        pSB->SetPrecious();
         DWORD dwSyncBlockIndex = pSB->GetSyncBlockIndex();
 
         pNewWrap = RCW::CreateRCW((IUnknown *)pAutoUnk, dwSyncBlockIndex, flags, pNewMT);

src/coreclr/vm/syncblk.cpp

@@ -828,29 +828,6 @@ void    SyncBlockCache::DeleteSyncBlockMemory(SyncBlock *psb)
 
 }
 
-// free a used sync block
-void SyncBlockCache::GCDeleteSyncBlock(SyncBlock *psb)
-{
-    CONTRACTL
-    {
-        INSTANCE_CHECK;
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-
-    // Destruct the SyncBlock, but don't reclaim its memory.  (Overridden
-    // operator delete).
-    delete psb;
-
-    m_ActiveCount--;
-    m_FreeCount++;
-
-    psb->m_Link.m_pNext = m_FreeBlockList;
-    m_FreeBlockList = &psb->m_Link;
-}
-
 void SyncBlockCache::GCWeakPtrScan(HANDLESCANPROC scanProc, uintptr_t lp1, uintptr_t lp2)
 {
     CONTRACTL
@@ -962,7 +939,7 @@ void SyncBlockCache::GCWeakPtrScan(HANDLESCANPROC scanProc, uintptr_t lp1, uintp
                     {
                         (*scanProc) (keyv, NULL, lp1, lp2);
                         SyncBlock   *pSB = syncTableShadow[nb].m_SyncBlock;
-                        if (*keyv != 0 && (!pSB || !pSB->IsIDisposable()))
+                        if (*keyv != 0 && !pSB)
                         {
                             if (syncTableShadow[nb].m_Object != SyncTableEntry::GetSyncTableEntry()[nb].m_Object)
                                 DebugBreak ();
@@ -1067,7 +1044,7 @@ BOOL SyncBlockCache::GCWeakPtrScanElement (int nb, HANDLESCANPROC scanProc, LPAR
 
         (*scanProc) (keyv, NULL, lp1, lp2);
         SyncBlock   *pSB = SyncTableEntry::GetSyncTableEntry()[nb].m_SyncBlock;
-        if ((*keyv == 0 ) || (pSB && pSB->IsIDisposable()))
+        if (*keyv == 0 )
         {
 #ifdef VERIFY_HEAP
             if (g_pConfig->GetHeapVerifyLevel () & EEConfig::HEAPVERIFY_SYNCBLK)
@@ -1076,14 +1053,7 @@ BOOL SyncBlockCache::GCWeakPtrScanElement (int nb, HANDLESCANPROC scanProc, LPAR
             }
 #endif
 
-            if (*keyv)
-            {
-                _ASSERTE (pSB);
-                GCDeleteSyncBlock(pSB);
-                //clean the object syncblock header
-                ((Object*)(*keyv))->GetHeader()->GCResetIndex();
-            }
-            else if (pSB)
+            if (pSB)
             {
 
                 cleanup = TRUE;
@@ -1656,11 +1626,6 @@ SyncBlock *ObjHeader::GetSyncBlock()
                     SetIndex(BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX | indx);
                 }
 
-                //If we had already an index, hold the syncblock
-                //for the lifetime of the object.
-                if (indexHeld)
-                    syncBlock->SetPrecious();
-
                 LEAVE_SPIN_LOCK(this);
             }
         }
@@ -1679,7 +1644,6 @@ SyncBlock *ObjHeader::GetSyncBlock()
 bool SyncBlock::SetInteropInfo(InteropSyncBlockInfo* pInteropInfo)
 {
     WRAPPER_NO_CONTRACT;
-    SetPrecious();
 
     // We could be agile, but not have noticed yet.  We can't assert here
     //  that we live in any given domain, nor is this an appropriate place
@@ -1701,9 +1665,6 @@ void SyncBlock::SetEnCInfo(EnCSyncBlockInfo *pEnCInfo)
 {
     WRAPPER_NO_CONTRACT;
 
-    // We can't recreate the field contents, so this SyncBlock can never go away
-    SetPrecious();
-
     // Store the field info (should only ever happen once)
     _ASSERTE( m_pEnCInfo == NULL );
     m_pEnCInfo = pEnCInfo;
@@ -1734,8 +1695,6 @@ OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
         return m_Lock;
     }
 
-    SetPrecious();
-
     // We need to create a new lock
     DWORD thinLock = m_thinLock;
     OBJECTHANDLEHolder lockHandle = GetAppDomain()->CreateHandle(lockObj);

src/coreclr/vm/syncblk.h

@@ -467,23 +467,9 @@ class SyncBlock
     DWORD GetSyncBlockIndex()
     {
         LIMITED_METHOD_CONTRACT;
-        return m_dwSyncIndex & ~SyncBlockPrecious;
+        return m_dwSyncIndex;
     }
 
-   // As soon as a syncblock acquires some state that cannot be recreated, we latch
-   // a bit.
-   void SetPrecious()
-   {
-       WRAPPER_NO_CONTRACT;
-       m_dwSyncIndex |= SyncBlockPrecious;
-   }
-
-   BOOL IsPrecious()
-   {
-       LIMITED_METHOD_CONTRACT;
-       return (m_dwSyncIndex & SyncBlockPrecious) != 0;
-   }
-
    // Get the lock information for this sync block.
    // Returns false when the lock is not locked or has not been created yet.
    BOOL TryGetLockInfo(DWORD *pThreadId, DWORD *pRecursionLevel);
@@ -496,15 +482,6 @@ class SyncBlock
 
    OBJECTHANDLE GetOrCreateLock(OBJECTREF lockObj);
 
-    // True is the syncblock and its index are disposable.
-    // If new members are added to the syncblock, this
-    // method needs to be modified accordingly
-    BOOL IsIDisposable()
-    {
-        WRAPPER_NO_CONTRACT;
-        return !IsPrecious() && m_thinLock == 0u;
-    }
-
     // Gets the InteropInfo block, creates a new one if none is present.
     InteropSyncBlockInfo* GetInteropInfo()
     {
@@ -571,8 +548,6 @@ class SyncBlock
         DWORD result = InterlockedCompareExchange((LONG*)&m_dwHashCode, hashCode, 0);
         if (result == 0)
         {
-            // the sync block now holds a hash code, which we can't afford to lose.
-            SetPrecious();
             return hashCode;
         }
         else
@@ -590,13 +565,6 @@ class SyncBlock
         // We've already destructed.  But retain the memory.
     }
 
-    enum
-    {
-        // This bit indicates that the syncblock is valuable and can neither be discarded
-        // nor re-created.
-        SyncBlockPrecious   = 0x80000000,
-    };
-
     BOOL HasCOMBstrTrailByte()
     {
         LIMITED_METHOD_CONTRACT;
@@ -610,7 +578,6 @@ class SyncBlock
     {
         WRAPPER_NO_CONTRACT;
         m_BSTRTrailByte = trailByte;
-        SetPrecious();
     }
 
     private:
@@ -727,9 +694,6 @@ class SyncBlockCache
     // returns the sync block memory to the free pool but does not destruct sync block (must own cache lock already)
     void    DeleteSyncBlockMemory(SyncBlock *sb);
 
-    // return sync block to cache or delete, called from GC
-    void    GCDeleteSyncBlock(SyncBlock *sb);
-
     void    GCWeakPtrScan(HANDLESCANPROC scanProc, uintptr_t lp1, uintptr_t lp2);
 
     void    GCDone(BOOL demoting, int max_gen);
@@ -864,14 +828,6 @@ class ObjHeader
         InterlockedAnd((LONG*)&m_SyncBlockValue, ~(BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX | BIT_SBLK_IS_HASHCODE | MASK_SYNCBLOCKINDEX));
     }
 
-    // Used only GC
-    void GCResetIndex()
-    {
-        LIMITED_METHOD_CONTRACT;
-
-        m_SyncBlockValue.RawValue() &=~(BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX | BIT_SBLK_IS_HASHCODE | MASK_SYNCBLOCKINDEX);
-    }
-
     // For now, use interlocked operations to twiddle bits in the bitfield portion.
     // If we ever have high-performance requirements where we can guarantee that no
     // other threads are accessing the ObjHeader, this can be reconsidered for those