From 575646d004fb098588b247b72b53cd10ba1eb914 Mon Sep 17 00:00:00 2001 From: qiaopengcheng Date: Mon, 24 Apr 2023 15:16:43 +0800 Subject: [PATCH 1/4] add aligned attribute for g_SyncBlockCacheInstance to avoid unaligned accessing error on some CPUs. --- src/coreclr/vm/syncblk.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 8be38ce3b6e7df..ea0b98b0b2f3f7 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -47,7 +47,7 @@ class SyncBlockArray }; // For in-place constructor -BYTE g_SyncBlockCacheInstance[sizeof(SyncBlockCache)]; +BYTE DECLSPEC_ALIGN(sizeof(void*)) g_SyncBlockCacheInstance[sizeof(SyncBlockCache)]; SPTR_IMPL (SyncBlockCache, SyncBlockCache, s_pSyncBlockCache); From 68beb17ac87f728fb192266a7beeebd71815bdee Mon Sep 17 00:00:00 2001 From: qiaopengcheng Date: Mon, 24 Apr 2023 23:15:54 +0800 Subject: [PATCH 2/4] refactor the g_SyncBlockCacheInstance by CR. --- src/coreclr/vm/syncblk.cpp | 68 ++++++++++++++++---------------------- src/coreclr/vm/syncblk.h | 5 +-- 2 files changed, 32 insertions(+), 41 deletions(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index ea0b98b0b2f3f7..e821f2b63d441b 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -46,15 +46,12 @@ class SyncBlockArray BYTE m_Blocks[MAXSYNCBLOCK * sizeof (SyncBlock)]; }; -// For in-place constructor -BYTE DECLSPEC_ALIGN(sizeof(void*)) g_SyncBlockCacheInstance[sizeof(SyncBlockCache)]; +SyncBlockCache g_SyncBlockCacheInstance; SPTR_IMPL (SyncBlockCache, SyncBlockCache, s_pSyncBlockCache); #ifndef DACCESS_COMPILE - - #ifndef TARGET_UNIX // static SLIST_HEADER InteropSyncBlockInfo::s_InteropInfoStandbyList; @@ -463,49 +460,41 @@ size_t BitMapSize (size_t cacheSize) // // *************************************************************************** -SyncBlockCache::SyncBlockCache() - : m_pCleanupBlockList(NULL), - m_FreeBlockList(NULL), - - // NOTE: CRST_UNSAFE_ANYMODE prevents a GC mode switch when entering this crst. - // If you remove this flag, we will switch to preemptive mode when entering - // g_criticalSection, which means all functions that enter it will become - // GC_TRIGGERS. (This includes all uses of LockHolder around SyncBlockCache::GetSyncBlockCache(). - // So be sure to update the contracts if you remove this flag. - m_CacheLock(CrstSyncBlockCache, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD)), - - m_FreeCount(0), - m_ActiveCount(0), - m_SyncBlocks(0), - m_FreeSyncBlock(0), - m_FreeSyncTableIndex(1), - m_FreeSyncTableList(0), - m_SyncTableSize(SYNC_TABLE_INITIAL_SIZE), - m_OldSyncTables(0), - m_bSyncBlockCleanupInProgress(FALSE), - m_EphemeralBitmap(0) +void SyncBlockCache::Init() { CONTRACTL { - CONSTRUCTOR_CHECK; - THROWS; - GC_NOTRIGGER; - MODE_ANY; - INJECT_FAULT(COMPlusThrowOM()); + WRAPPER_NO_CONTRACT; } CONTRACTL_END; -} + m_pCleanupBlockList = NULL; + m_FreeBlockList = NULL; -// This method is NO longer called. -SyncBlockCache::~SyncBlockCache() + // NOTE: CRST_UNSAFE_ANYMODE prevents a GC mode switch when entering this crst. + // If you remove this flag, we will switch to preemptive mode when entering + // g_criticalSection, which means all functions that enter it will become + // GC_TRIGGERS. (This includes all uses of LockHolder around SyncBlockCache::GetSyncBlockCache(). + // So be sure to update the contracts if you remove this flag. + ::new (&m_CacheLock) Crst(CrstSyncBlockCache, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD)); + + m_FreeCount = 0; + m_ActiveCount = 0; + m_SyncBlocks = 0; + m_FreeSyncBlock = 0; + m_FreeSyncTableIndex = 1; + m_FreeSyncTableList = 0; + m_SyncTableSize = SYNC_TABLE_INITIAL_SIZE; + m_OldSyncTables = 0; + m_bSyncBlockCleanupInProgress = FALSE; + m_EphemeralBitmap = 0; +} + +void SyncBlockCache::Destroy() { CONTRACTL { - DESTRUCTOR_CHECK; - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; + WRAPPER_NO_CONTRACT; } CONTRACTL_END; @@ -655,7 +644,8 @@ void SyncBlockCache::Start() #endif SyncTableEntry::GetSyncTableEntry()[0].m_SyncBlock = 0; - SyncBlockCache::GetSyncBlockCache() = new (&g_SyncBlockCacheInstance) SyncBlockCache; + SyncBlockCache::GetSyncBlockCache() = &g_SyncBlockCacheInstance; + g_SyncBlockCacheInstance.Init(); SyncBlockCache::GetSyncBlockCache()->m_EphemeralBitmap = bm; @@ -681,7 +671,7 @@ void SyncBlockCache::Stop() // sync blocks which are live and thus must have their critical sections destroyed. if (SyncBlockCache::GetSyncBlockCache()) { - delete SyncBlockCache::GetSyncBlockCache(); + SyncBlockCache::GetSyncBlockCache()->Destroy(); SyncBlockCache::GetSyncBlockCache() = 0; } diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index 32ad8c522076d0..d7a949dc1e99ad 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -1356,8 +1356,9 @@ class SyncBlockCache LIMITED_METHOD_CONTRACT; } - SyncBlockCache(); - ~SyncBlockCache(); + // Note: No constructors/destructors - global instance + void Init(); + void Destroy(); static void Attach(); static void Detach(); From cf736781481f10669e29d145c5c668d4afcdf522 Mon Sep 17 00:00:00 2001 From: qiaopengcheng Date: Tue, 25 Apr 2023 07:47:42 +0800 Subject: [PATCH 3/4] delete unused operator for CR. --- src/coreclr/vm/syncblk.cpp | 11 +++++++++-- src/coreclr/vm/syncblk.h | 11 ----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index e821f2b63d441b..d30e29e0b1edb8 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -464,7 +464,11 @@ void SyncBlockCache::Init() { CONTRACTL { - WRAPPER_NO_CONTRACT; + CONSTRUCTOR_CHECK; + THROWS; + GC_NOTRIGGER; + MODE_ANY; + INJECT_FAULT(COMPlusThrowOM()); } CONTRACTL_END; @@ -494,7 +498,10 @@ void SyncBlockCache::Destroy() { CONTRACTL { - WRAPPER_NO_CONTRACT; + DESTRUCTOR_CHECK; + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index d7a949dc1e99ad..976865def135ec 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -1345,17 +1345,6 @@ class SyncBlockCache SPTR_DECL(SyncBlockCache, s_pSyncBlockCache); static SyncBlockCache*& GetSyncBlockCache(); - void *operator new(size_t size, void *pInPlace) - { - LIMITED_METHOD_CONTRACT; - return pInPlace; - } - - void operator delete(void *p) - { - LIMITED_METHOD_CONTRACT; - } - // Note: No constructors/destructors - global instance void Init(); void Destroy(); From f76a6747b8d51cecc4a86bef29151ddcdca2805e Mon Sep 17 00:00:00 2001 From: qiaopengcheng Date: Tue, 25 Apr 2023 08:03:33 +0800 Subject: [PATCH 4/4] replace Crst with CrstStatic by CR. --- src/coreclr/vm/syncblk.cpp | 4 +++- src/coreclr/vm/syncblk.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index d30e29e0b1edb8..71a32d4fe164e0 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -480,7 +480,7 @@ void SyncBlockCache::Init() // g_criticalSection, which means all functions that enter it will become // GC_TRIGGERS. (This includes all uses of LockHolder around SyncBlockCache::GetSyncBlockCache(). // So be sure to update the contracts if you remove this flag. - ::new (&m_CacheLock) Crst(CrstSyncBlockCache, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD)); + m_CacheLock.Init(CrstSyncBlockCache, (CrstFlags) (CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD)); m_FreeCount = 0; m_ActiveCount = 0; @@ -510,6 +510,8 @@ void SyncBlockCache::Destroy() //@todo we can clear this fast too I guess m_pCleanupBlockList = NULL; + m_CacheLock.Destroy(); + // destruct all arrays while (m_SyncBlocks) { diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index 976865def135ec..a45d03e7bf2e6d 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -1309,7 +1309,7 @@ class SyncBlockCache private: PTR_SLink m_pCleanupBlockList; // list of sync blocks that need cleanup SLink* m_FreeBlockList; // list of free sync blocks - Crst m_CacheLock; // cache lock + CrstStatic m_CacheLock; // cache lock DWORD m_FreeCount; // count of active sync blocks DWORD m_ActiveCount; // number active SyncBlockArray *m_SyncBlocks; // Array of new SyncBlocks.