From 6aa6b07e4ed19f0ba0cdf5987647d0b6c98d8540 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 15:21:04 +0200 Subject: [PATCH 01/21] Address Jan's feedback --- src/coreclr/vm/appdomain.hpp | 15 ++-- src/coreclr/vm/frozenobjectheap.cpp | 91 ++++++++++++++++-------- src/coreclr/vm/frozenobjectheap.h | 9 +++ src/coreclr/vm/profilingenumerators.cpp | 8 ++- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 8 ++- 5 files changed, 96 insertions(+), 35 deletions(-) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 2103ba6c654593..00a6030818d86c 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2449,14 +2449,21 @@ class SystemDomain : public BaseDomain _ASSERTE(m_pGlobalStringLiteralMap); return m_pGlobalStringLiteralMap; } - static FrozenObjectHeapManager* GetFrozenObjectHeapManager() + static FrozenObjectHeapManager* GetFrozenObjectHeapManager(bool initialize = true) { WRAPPER_NO_CONTRACT; - if (m_FrozenObjectHeapManager == NULL) + if (VolatileLoad(&m_FrozenObjectHeapManager) == nullptr) { - LazyInitFrozenObjectsHeap(); + if (initialize) + { + LazyInitFrozenObjectsHeap(); + } + else + { + return nullptr; + } } - return m_FrozenObjectHeapManager; + return VolatileLoad(&m_FrozenObjectHeapManager); } #endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 45492155d2089c..152dfb4740d195 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -10,7 +10,10 @@ #define FOH_COMMIT_SIZE (64 * 1024) FrozenObjectHeapManager::FrozenObjectHeapManager(): - m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), + // This lock is used in both COOP and PREEMP (by profiler) modes + m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), + // This lock is used only in COOP mode + m_SegmentRegistrationCrst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), m_CurrentSegment(nullptr) { } @@ -33,6 +36,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t #else // FEATURE_BASICFREEZE Object* obj = nullptr; + FrozenObjectSegment* currentSegment = nullptr; { CrstHolder ch(&m_Crst); @@ -60,24 +64,20 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t return nullptr; } - if (m_CurrentSegment == nullptr) - { - // Create the first segment on first allocation - m_CurrentSegment = new FrozenObjectSegment(FOH_SEGMENT_DEFAULT_SIZE); - m_FrozenSegments.Append(m_CurrentSegment); - _ASSERT(m_CurrentSegment != nullptr); - } - - obj = m_CurrentSegment->TryAllocateObject(type, objectSize); - - // The only case where it can be null is when the current segment is full and we need - // to create a new one + obj = m_CurrentSegment == nullptr ? nullptr : m_CurrentSegment->TryAllocateObject(type, objectSize); + // obj is nullptr if the current segment is full or hasn't been allocated yet if (obj == nullptr) { - // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects - // Use the same size in case if prevSegmentSize*2 operation overflows. - size_t prevSegmentSize = m_CurrentSegment->GetSize(); - m_CurrentSegment = new FrozenObjectSegment(max(prevSegmentSize, prevSegmentSize * 2)); + size_t newSegmentSize = FOH_SEGMENT_DEFAULT_SIZE; + if (m_CurrentSegment != nullptr) + { + // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects + // Use the same size in case if prevSegmentSize*2 operation overflows. + const size_t prevSegmentSize = m_CurrentSegment->GetSize(); + newSegmentSize = max(prevSegmentSize, prevSegmentSize * 2); + } + + m_CurrentSegment = new FrozenObjectSegment(newSegmentSize); m_FrozenSegments.Append(m_CurrentSegment); // Try again @@ -86,7 +86,23 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t // This time it's not expected to be null _ASSERT(obj != nullptr); } + currentSegment = m_CurrentSegment; + } + + // If the currently used segment hasn't been registered yet, do it now. + // We do it under a new lock because the main one (m_Crst) can be used by Profiler in a GC's thread + // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock. + if (!currentSegment->IsRegistered()) + { + CrstHolder regLock(&m_SegmentRegistrationCrst); + + // Double-checked locking + if (!currentSegment->IsRegistered()) + { + currentSegment->Register(); + } } + if (publish) { PublishFrozenObject(obj); @@ -103,6 +119,7 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : m_pCurrent(nullptr), m_SizeCommitted(0), m_Size(sizeHint), + m_IsRegistered(false), m_SegmentHandle(nullptr) COMMA_INDEBUG(m_ObjectsCount(0)) { @@ -135,29 +152,36 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : ThrowOutOfMemory(); } + m_pStart = static_cast(committedAlloc); + m_pCurrent = m_pStart + sizeof(ObjHeader); + m_SizeCommitted = FOH_COMMIT_SIZE; + INDEBUG(m_ObjectsCount = 0); + // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect // DATA_ALIGNMENT alignment as well _ASSERT(IS_ALIGNED(committedAlloc, DATA_ALIGNMENT)); +} + +void FrozenObjectSegment::Register() +{ + // Caller is expected to make sure it's not registered twice + _ASSERT(!VolatileLoad(&m_IsInitialized)); segment_info si; - si.pvMem = committedAlloc; + si.pvMem = m_pStart; si.ibFirstObject = sizeof(ObjHeader); - si.ibAllocated = si.ibFirstObject; - si.ibCommit = FOH_COMMIT_SIZE; + si.ibAllocated = (size_t)m_pCurrent; // there can be multiple objects already allocated + si.ibCommit = m_SizeCommitted; si.ibReserved = m_Size; + // NOTE: RegisterFrozenSegment may take a GC lock inside. m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle == nullptr) { - ClrVirtualFree(alloc, 0, MEM_RELEASE); + ClrVirtualFree(m_pStart, 0, MEM_RELEASE); ThrowOutOfMemory(); } - - m_pStart = static_cast(committedAlloc); - m_pCurrent = m_pStart + sizeof(ObjHeader); - m_SizeCommitted = si.ibCommit; - INDEBUG(m_ObjectsCount = 0); - return; + VolatileStore(&m_IsRegistered, true); } Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t objectSize) @@ -201,8 +225,17 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje m_pCurrent += objectSize; - // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommitted); + if (m_IsRegistered) + { + // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part + // NOTE: UpdateFrozenSegment is not expected to take any lock inside + GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommitted); + } + else + { + // The segment is not yet registered so the upcoming RegisterFrozenSegment + // will let GC know about this object (and all others) as is. + } return object; } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index d2c0bb62f134af..4de91f91895491 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -31,6 +31,7 @@ class FrozenObjectHeapManager private: Crst m_Crst; + Crst m_SegmentRegistrationCrst; SArray m_FrozenSegments; FrozenObjectSegment* m_CurrentSegment; @@ -47,6 +48,11 @@ class FrozenObjectSegment { return m_Size; } + bool IsRegistered() const + { + return VolatileLoad(&m_IsRegistered); + } + void Register(); private: Object* GetFirstObject() const; @@ -69,6 +75,9 @@ class FrozenObjectSegment // Total memory reserved for the current segment size_t m_Size; + // Whether GC knows about this segment already or it hasn't been registered yet + bool m_IsRegistered; + segment_handle m_SegmentHandle; INDEBUG(size_t m_ObjectsCount); diff --git a/src/coreclr/vm/profilingenumerators.cpp b/src/coreclr/vm/profilingenumerators.cpp index 1d19a87324be92..7040c4a2a1a683 100644 --- a/src/coreclr/vm/profilingenumerators.cpp +++ b/src/coreclr/vm/profilingenumerators.cpp @@ -103,7 +103,13 @@ BOOL ProfilerObjectEnum::Init() } CONTRACTL_END; - FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); + // initialize = false to avoid breaking the NOTHROW contract + FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(/*initialize*/ false); + if (foh == nullptr) + { + return TRUE; + } + CrstHolder ch(&foh->m_Crst); const unsigned segmentsCount = foh->m_FrozenSegments.GetCount(); diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index 1f2e4f26001859..273c04d0978b9d 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -7672,7 +7672,13 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, return E_INVALIDARG; } - FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); + // initialize = false to avoid breaking the NOTHROW contract + FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(/*initialize*/ false); + if (foh == nullptr) + { + *pcObjectRanges = 0; + return S_OK; + } CrstHolder ch(&foh->m_Crst); const unsigned segmentsCount = foh->m_FrozenSegments.GetCount(); From aac9cd477e3275f001a303d5f12d16abac544de6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 15:31:48 +0200 Subject: [PATCH 02/21] Fix Debug build --- src/coreclr/vm/frozenobjectheap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 152dfb4740d195..7ec89b3acfa0e8 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -165,7 +165,7 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : void FrozenObjectSegment::Register() { // Caller is expected to make sure it's not registered twice - _ASSERT(!VolatileLoad(&m_IsInitialized)); + _ASSERT(!VolatileLoad(&m_IsRegistered)); segment_info si; si.pvMem = m_pStart; From 45bbad804cc16414148858fad5eba7890836d783 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 15:37:37 +0200 Subject: [PATCH 03/21] Clean up --- src/coreclr/vm/frozenobjectheap.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 7ec89b3acfa0e8..06e933994c17ea 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -165,7 +165,7 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : void FrozenObjectSegment::Register() { // Caller is expected to make sure it's not registered twice - _ASSERT(!VolatileLoad(&m_IsRegistered)); + _ASSERT(!IsRegistered()); segment_info si; si.pvMem = m_pStart; @@ -225,7 +225,7 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje m_pCurrent += objectSize; - if (m_IsRegistered) + if (!IsRegistered()) { // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part // NOTE: UpdateFrozenSegment is not expected to take any lock inside From 3962cfd57a1f32a5b7daad2fab0a6cd867e1466c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 17:04:17 +0200 Subject: [PATCH 04/21] Fix debug assert --- src/coreclr/vm/frozenobjectheap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 06e933994c17ea..05b079b211a7e1 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -186,7 +186,7 @@ void FrozenObjectSegment::Register() Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t objectSize) { - _ASSERT(m_pStart != nullptr && m_Size > 0 && m_SegmentHandle != nullptr); // Expected to be inited + _ASSERT((m_pStart != nullptr) && (m_Size > 0)); _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); _ASSERT(objectSize <= FOH_COMMIT_SIZE); From e5b68d50f64d85228bf0f6586393ea8c47dc837b Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 20 Aug 2023 17:09:55 +0200 Subject: [PATCH 05/21] Update frozenobjectheap.cpp --- src/coreclr/vm/frozenobjectheap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 05b079b211a7e1..658a2b783f5cf0 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -225,7 +225,7 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje m_pCurrent += objectSize; - if (!IsRegistered()) + if (IsRegistered()) { // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part // NOTE: UpdateFrozenSegment is not expected to take any lock inside From b4c1f17196fd534eb1c2c3632f8a3180fcf2c7fb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 20:14:00 +0200 Subject: [PATCH 06/21] Address feedback --- src/coreclr/vm/appdomain.hpp | 25 +++++++++++++--------- src/coreclr/vm/methodtable.cpp | 5 +++-- src/coreclr/vm/profilingenumerators.cpp | 7 ++++-- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 27 +++++++++++++++++------- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 00a6030818d86c..7633fb9f2881bf 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2449,22 +2449,27 @@ class SystemDomain : public BaseDomain _ASSERTE(m_pGlobalStringLiteralMap); return m_pGlobalStringLiteralMap; } - static FrozenObjectHeapManager* GetFrozenObjectHeapManager(bool initialize = true) + static FrozenObjectHeapManager* GetFrozenObjectHeapManager() { - WRAPPER_NO_CONTRACT; + CONTRACTL + { + THROWS; + MODE_COOPERATIVE; + } + CONTRACTL_END; + if (VolatileLoad(&m_FrozenObjectHeapManager) == nullptr) { - if (initialize) - { - LazyInitFrozenObjectsHeap(); - } - else - { - return nullptr; - } + LazyInitFrozenObjectsHeap(); } return VolatileLoad(&m_FrozenObjectHeapManager); } + static FrozenObjectHeapManager* GetFrozenObjectHeapManagerNoThrow() + { + LIMITED_METHOD_CONTRACT; + + return VolatileLoad(&m_FrozenObjectHeapManager); + } #endif // DACCESS_COMPILE #if defined(FEATURE_COMINTEROP_APARTMENT_SUPPORT) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 1016a80971b7a0..688bf29079e08d 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4187,8 +4187,9 @@ void MethodTable::AllocateRegularStaticBox(FieldDesc* pField, Object** boxedStat THROWS; GC_TRIGGERS; MODE_COOPERATIVE; - CONTRACTL_END; } + CONTRACTL_END + _ASSERT(pField->IsStatic() && !pField->IsSpecialStatic() && pField->IsByValue()); // Static fields are not pinned in collectible types so we need to protect the address @@ -4222,8 +4223,8 @@ OBJECTREF MethodTable::AllocateStaticBox(MethodTable* pFieldMT, BOOL fPinned, OB THROWS; GC_TRIGGERS; MODE_COOPERATIVE; - CONTRACTL_END; } + CONTRACTL_END _ASSERTE(pFieldMT->IsValueType()); diff --git a/src/coreclr/vm/profilingenumerators.cpp b/src/coreclr/vm/profilingenumerators.cpp index 7040c4a2a1a683..a18637f45df19c 100644 --- a/src/coreclr/vm/profilingenumerators.cpp +++ b/src/coreclr/vm/profilingenumerators.cpp @@ -103,8 +103,7 @@ BOOL ProfilerObjectEnum::Init() } CONTRACTL_END; - // initialize = false to avoid breaking the NOTHROW contract - FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(/*initialize*/ false); + FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManagerNoThrow(); if (foh == nullptr) { return TRUE; @@ -119,6 +118,10 @@ BOOL ProfilerObjectEnum::Init() for (unsigned segmentIdx = 0; segmentIdx < segmentsCount; segmentIdx++) { const FrozenObjectSegment* segment = segments[segmentIdx]; + if (!segment->IsRegistered()) + { + continue; + } Object* currentObj = segment->GetFirstObject(); while (currentObj != nullptr) diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index 273c04d0978b9d..e4c21422244a49 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -7672,8 +7672,7 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, return E_INVALIDARG; } - // initialize = false to avoid breaking the NOTHROW contract - FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(/*initialize*/ false); + FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManagerNoThrow(); if (foh == nullptr) { *pcObjectRanges = 0; @@ -7685,25 +7684,37 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, FrozenObjectSegment** segments = foh->m_FrozenSegments.GetElements(); if (segments != nullptr && segmentsCount > 0) { - const ULONG segmentsToInspect = min(cObjectRanges, (ULONG)segmentsCount); + unsigned totalRegisteredSegments = 0; - for (unsigned segIdx = 0; segIdx < segmentsToInspect; segIdx++) + for (unsigned segIdx = 0; segIdx < segmentsCount; segIdx++) { - uint8_t* firstObj = segments[segIdx]->m_pStart + sizeof(ObjHeader); + if (segIdx >= cObjectRanges) + { + break; + } + + FrozenObjectSegment* segment = segments[segIdx]; + if (!segment->IsRegistered()) + { + continue; + } + totalRegisteredSegments++; + + uint8_t* firstObj = segment->m_pStart + sizeof(ObjHeader); // Start of the segment (first object) ranges[segIdx].rangeStart = (ObjectID)firstObj; // Total size reserved for a segment - ranges[segIdx].rangeLengthReserved = (UINT_PTR)segments[segIdx]->m_Size - sizeof(ObjHeader); + ranges[segIdx].rangeLengthReserved = (UINT_PTR)segment->m_Size - sizeof(ObjHeader); // Size of the segment that is currently in use - ranges[segIdx].rangeLength = (UINT_PTR)(segments[segIdx]->m_pCurrent - firstObj); + ranges[segIdx].rangeLength = (UINT_PTR)segment->m_pCurrent - firstObj); } if (pcObjectRanges != nullptr) { - *pcObjectRanges = (ULONG)segmentsCount; + *pcObjectRanges = (ULONG)totalRegisteredSegments; } } else From d29da2c88478b78b462f1b2ac0671e134d09f4f9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 20:54:43 +0200 Subject: [PATCH 07/21] fix build --- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index e4c21422244a49..1953261693fc7c 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -7709,7 +7709,7 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, ranges[segIdx].rangeLengthReserved = (UINT_PTR)segment->m_Size - sizeof(ObjHeader); // Size of the segment that is currently in use - ranges[segIdx].rangeLength = (UINT_PTR)segment->m_pCurrent - firstObj); + ranges[segIdx].rangeLength = (UINT_PTR)(segment->m_pCurrent - firstObj); } if (pcObjectRanges != nullptr) From 2772f8f1d9bba4724b4f522937fd65447021a801 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 20 Aug 2023 21:04:06 +0200 Subject: [PATCH 08/21] Update src/coreclr/vm/frozenobjectheap.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/frozenobjectheap.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 658a2b783f5cf0..14196a0dd3e6b1 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -38,6 +38,8 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t Object* obj = nullptr; FrozenObjectSegment* currentSegment = nullptr; { + GCX_PREEMP(); + CrstHolder ch(&m_Crst); _ASSERT(type != nullptr); From ae4ea9043b63970d8e8fcf7b90da1f52c36c86bd Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 20 Aug 2023 21:29:30 +0200 Subject: [PATCH 09/21] Address feedback --- src/coreclr/vm/frozenobjectheap.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 14196a0dd3e6b1..f9ed9b3e68e2fa 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -10,9 +10,9 @@ #define FOH_COMMIT_SIZE (64 * 1024) FrozenObjectHeapManager::FrozenObjectHeapManager(): - // This lock is used in both COOP and PREEMP (by profiler) modes + // This lock is used in PREEMP mode m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), - // This lock is used only in COOP mode + // This lock is used in COOP mode m_SegmentRegistrationCrst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), m_CurrentSegment(nullptr) { @@ -180,7 +180,6 @@ void FrozenObjectSegment::Register() m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle == nullptr) { - ClrVirtualFree(m_pStart, 0, MEM_RELEASE); ThrowOutOfMemory(); } VolatileStore(&m_IsRegistered, true); From 0a731c492e0ecb78687c1119a1c40138a1204be4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 22 Aug 2023 23:46:49 +0200 Subject: [PATCH 10/21] Add gc lock for UpdateFrozenSegment --- src/coreclr/gc/gc.cpp | 12 ++++ src/coreclr/gc/gcee.cpp | 9 ++- src/coreclr/gc/gcpriv.h | 1 + src/coreclr/vm/frozenobjectheap.cpp | 70 +++++++++++------------- src/coreclr/vm/frozenobjectheap.h | 4 +- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 2 +- 6 files changed, 54 insertions(+), 44 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 02a9b8f26c2f56..d4e7ec9622e648 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -9955,6 +9955,18 @@ BOOL gc_heap::insert_ro_segment (heap_segment* seg) return TRUE; } +void gc_heap::update_ro_segment (heap_segment* seg, uint8_t* allocated, uint8_t* committed) +{ + enter_spin_lock (&gc_heap::gc_lock); + + assert (use_frozen_segments_p); + assert (heap_segment_read_only_p(seg)); + heap_segment_allocated (seg) = allocated; + heap_segment_committed (seg) = committed; + + leave_spin_lock (&gc_heap::gc_lock); +} + // No one is calling this function right now. If this is getting called we need // to take care of decommitting the mark array for it - we will need to remember // which portion of the mark array was committed and only decommit that. diff --git a/src/coreclr/gc/gcee.cpp b/src/coreclr/gc/gcee.cpp index 6dbbfd64a7a514..32738da9b603ab 100644 --- a/src/coreclr/gc/gcee.cpp +++ b/src/coreclr/gc/gcee.cpp @@ -510,9 +510,12 @@ bool GCHeap::IsInFrozenSegment(Object *object) void GCHeap::UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed) { #ifdef FEATURE_BASICFREEZE - heap_segment* heap_seg = reinterpret_cast(seg); - heap_segment_committed(heap_seg) = committed; - heap_segment_allocated(heap_seg) = allocated; +#ifdef MULTIPLE_HEAPS + gc_heap* heap = gc_heap::g_heaps[0]; +#else + gc_heap* heap = pGenGCHeap; +#endif //MULTIPLE_HEAPS + heap->update_ro_segment (reinterpret_cast(seg), allocated, committed); #endif // FEATURE_BASICFREEZE } diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index da0085ce19610d..1a73add83b429f 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2380,6 +2380,7 @@ class gc_heap #ifdef FEATURE_BASICFREEZE PER_HEAP_METHOD BOOL insert_ro_segment (heap_segment* seg); PER_HEAP_METHOD void remove_ro_segment (heap_segment* seg); + PER_HEAP_METHOD void update_ro_segment (heap_segment* seg, uint8_t* allocated, uint8_t* committed); #endif //FEATURE_BASICFREEZE PER_HEAP_METHOD BOOL set_ro_segment_in_range (heap_segment* seg); #ifndef USE_REGIONS diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index f9ed9b3e68e2fa..1682688679426c 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -91,18 +91,12 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t currentSegment = m_CurrentSegment; } - // If the currently used segment hasn't been registered yet, do it now. + // Let GC know about the new segment or changes in it. // We do it under a new lock because the main one (m_Crst) can be used by Profiler in a GC's thread // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock. - if (!currentSegment->IsRegistered()) { CrstHolder regLock(&m_SegmentRegistrationCrst); - - // Double-checked locking - if (!currentSegment->IsRegistered()) - { - currentSegment->Register(); - } + currentSegment->RegisterOrUpdate(); } if (publish) @@ -119,7 +113,9 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : m_pStart(nullptr), m_pCurrent(nullptr), + m_pCurrentRegistered(nullptr), m_SizeCommitted(0), + m_SizeCommittedRegistered(0), m_Size(sizeHint), m_IsRegistered(false), m_SegmentHandle(nullptr) @@ -164,25 +160,33 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : _ASSERT(IS_ALIGNED(committedAlloc, DATA_ALIGNMENT)); } -void FrozenObjectSegment::Register() +void FrozenObjectSegment::RegisterOrUpdate() { - // Caller is expected to make sure it's not registered twice - _ASSERT(!IsRegistered()); - - segment_info si; - si.pvMem = m_pStart; - si.ibFirstObject = sizeof(ObjHeader); - si.ibAllocated = (size_t)m_pCurrent; // there can be multiple objects already allocated - si.ibCommit = m_SizeCommitted; - si.ibReserved = m_Size; - - // NOTE: RegisterFrozenSegment may take a GC lock inside. - m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); - if (m_SegmentHandle == nullptr) + m_SizeCommittedRegistered = m_SizeCommitted; + m_pCurrentRegistered = m_pCurrent; + + if (!IsRegistered()) { - ThrowOutOfMemory(); + segment_info si; + si.pvMem = m_pStart; + si.ibFirstObject = sizeof(ObjHeader); + si.ibAllocated = (size_t)m_pCurrentRegistered; + si.ibCommit = m_SizeCommittedRegistered; + si.ibReserved = m_Size; + + // NOTE: RegisterFrozenSegment may take a GC lock inside. + m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); + if (m_SegmentHandle == nullptr) + { + ThrowOutOfMemory(); + } + VolatileStore(&m_IsRegistered, true); + } + else + { + GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( + m_SegmentHandle, m_pCurrentRegistered, m_pStart + m_SizeCommittedRegistered); } - VolatileStore(&m_IsRegistered, true); } Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t objectSize) @@ -226,24 +230,12 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje m_pCurrent += objectSize; - if (IsRegistered()) - { - // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part - // NOTE: UpdateFrozenSegment is not expected to take any lock inside - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommitted); - } - else - { - // The segment is not yet registered so the upcoming RegisterFrozenSegment - // will let GC know about this object (and all others) as is. - } - return object; } Object* FrozenObjectSegment::GetFirstObject() const { - if (m_pStart + sizeof(ObjHeader) == m_pCurrent) + if (m_pStart + sizeof(ObjHeader) == m_pCurrentRegistered) { // Segment is empty return nullptr; @@ -255,11 +247,11 @@ Object* FrozenObjectSegment::GetNextObject(Object* obj) const { // Input must not be null and should be within the segment _ASSERT(obj != nullptr); - _ASSERT((uint8_t*)obj >= m_pStart + sizeof(ObjHeader) && (uint8_t*)obj < m_pCurrent); + _ASSERT((uint8_t*)obj >= m_pStart + sizeof(ObjHeader) && (uint8_t*)obj < m_pCurrentRegistered); // FOH doesn't support objects with non-DATA_ALIGNMENT alignment yet. uint8_t* nextObj = (reinterpret_cast(obj) + ALIGN_UP(obj->GetSize(), DATA_ALIGNMENT)); - if (nextObj < m_pCurrent) + if (nextObj < m_pCurrentRegistered) { return reinterpret_cast(nextObj); } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 4de91f91895491..dbfd3ad87e8470 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -52,7 +52,7 @@ class FrozenObjectSegment { return VolatileLoad(&m_IsRegistered); } - void Register(); + void RegisterOrUpdate(); private: Object* GetFirstObject() const; @@ -66,11 +66,13 @@ class FrozenObjectSegment // // m_pCurrent <= m_SizeCommitted uint8_t* m_pCurrent; + uint8_t* m_pCurrentRegistered; // Memory committed in the current segment // // m_SizeCommitted <= m_pStart + FOH_SIZE_RESERVED size_t m_SizeCommitted; + size_t m_SizeCommittedRegistered; // Total memory reserved for the current segment size_t m_Size; diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index 1953261693fc7c..7f23631ee41015 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -7709,7 +7709,7 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, ranges[segIdx].rangeLengthReserved = (UINT_PTR)segment->m_Size - sizeof(ObjHeader); // Size of the segment that is currently in use - ranges[segIdx].rangeLength = (UINT_PTR)(segment->m_pCurrent - firstObj); + ranges[segIdx].rangeLength = (UINT_PTR)(segment->m_pCurrentRegistered - firstObj); } if (pcObjectRanges != nullptr) From 714a0822612dc73e77ba2376670a752f12ff23fc Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 23 Aug 2023 13:48:09 +0200 Subject: [PATCH 11/21] Address feedback --- .../src/System/Threading/Monitor.CoreCLR.cs | 3 -- src/coreclr/vm/frozenobjectheap.cpp | 45 +++++++++++++------ src/coreclr/vm/frozenobjectheap.h | 11 +++-- 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs index 9fd823ba175708..02ce5435c70dd1 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs @@ -41,9 +41,6 @@ public static partial class Monitor // in the typical case. Note that the method has to be transparent for inlining to be allowed by the VM. public static void Enter(object obj, ref bool lockTaken) { - if (lockTaken) - ThrowLockTakenException(); - ReliableEnter(obj, ref lockTaken); Debug.Assert(lockTaken); } diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 1682688679426c..e9b56c8793d255 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -36,7 +36,10 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t #else // FEATURE_BASICFREEZE Object* obj = nullptr; - FrozenObjectSegment* currentSegment = nullptr; + FrozenObjectSegment* curSeg = nullptr; + uint8_t* curSegmentCurrent = nullptr; + size_t curSegSizeCommitted = 0; + { GCX_PREEMP(); @@ -75,7 +78,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t { // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects // Use the same size in case if prevSegmentSize*2 operation overflows. - const size_t prevSegmentSize = m_CurrentSegment->GetSize(); + const size_t prevSegmentSize = m_CurrentSegment->m_Size; newSegmentSize = max(prevSegmentSize, prevSegmentSize * 2); } @@ -88,7 +91,9 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t // This time it's not expected to be null _ASSERT(obj != nullptr); } - currentSegment = m_CurrentSegment; + curSeg = m_CurrentSegment; + curSegSizeCommitted = curSeg->m_SizeCommitted; + curSegmentCurrent = curSeg->m_pCurrent; } // Let GC know about the new segment or changes in it. @@ -96,7 +101,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock. { CrstHolder regLock(&m_SegmentRegistrationCrst); - currentSegment->RegisterOrUpdate(); + curSeg->RegisterOrUpdate(curSegmentCurrent, curSegSizeCommitted); } if (publish) @@ -119,7 +124,6 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : m_Size(sizeHint), m_IsRegistered(false), m_SegmentHandle(nullptr) - COMMA_INDEBUG(m_ObjectsCount(0)) { _ASSERT(m_Size > FOH_COMMIT_SIZE); _ASSERT(m_Size % FOH_COMMIT_SIZE == 0); @@ -153,20 +157,20 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : m_pStart = static_cast(committedAlloc); m_pCurrent = m_pStart + sizeof(ObjHeader); m_SizeCommitted = FOH_COMMIT_SIZE; - INDEBUG(m_ObjectsCount = 0); // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect // DATA_ALIGNMENT alignment as well _ASSERT(IS_ALIGNED(committedAlloc, DATA_ALIGNMENT)); } -void FrozenObjectSegment::RegisterOrUpdate() +void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited) { - m_SizeCommittedRegistered = m_SizeCommitted; - m_pCurrentRegistered = m_pCurrent; - if (!IsRegistered()) { + // Other threads won't touch these fields until we set m_IsRegistered to true + m_SizeCommittedRegistered = sizeCommited; + m_pCurrentRegistered = current; + segment_info si; si.pvMem = m_pStart; si.ibFirstObject = sizeof(ObjHeader); @@ -184,8 +188,23 @@ void FrozenObjectSegment::RegisterOrUpdate() } else { - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( - m_SegmentHandle, m_pCurrentRegistered, m_pStart + m_SizeCommittedRegistered); + if (current > VolatileLoad(&m_pCurrentRegistered)) + { + GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( + m_SegmentHandle, current, m_pStart + sizeCommited); + + // Profiler thread won't hold the registration lock, but, presumably, it won't see these values randomly + // because UpdateFrozenSegment will stuck in GC's lock while profiler is enumerating frozen segments + // (it's generally recommended to enumerate heaps in GarbageCollectionFinished/Started events). + // If profiler is invoked outside of GC's lock, then the accuracy is not guaranteed, but let's at least + // bump SizeCommited first: + VolatileStore(&m_SizeCommittedRegistered, sizeCommited); + VolatileStore(&m_pCurrentRegistered, current); + } + else + { + // Some other thread already advanced it. + } } } @@ -223,8 +242,6 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje m_SizeCommitted += FOH_COMMIT_SIZE; } - INDEBUG(m_ObjectsCount++); - Object* object = reinterpret_cast(m_pCurrent); object->SetMethodTable(type); diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index dbfd3ad87e8470..d72aa0a8f088a5 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -44,15 +44,11 @@ class FrozenObjectSegment public: FrozenObjectSegment(size_t sizeHint); Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); - size_t GetSize() const - { - return m_Size; - } bool IsRegistered() const { return VolatileLoad(&m_IsRegistered); } - void RegisterOrUpdate(); + void RegisterOrUpdate(uint8_t* current, size_t sizeCommited); private: Object* GetFirstObject() const; @@ -61,6 +57,9 @@ class FrozenObjectSegment // Start of the reserved memory, the first object starts at "m_pStart + sizeof(ObjHeader)" (its pMT) uint8_t* m_pStart; + // NOTE: To handle potential race conditions, only m_[x]Registered fields should be accessed + // externally as they guarantee that GC is aware of the current state of the segment. + // Pointer to the end of the current segment, ready to be used as a pMT for a new object // meaning that "m_pCurrent - sizeof(ObjHeader)" is the actual start of the new object (header). // @@ -81,10 +80,10 @@ class FrozenObjectSegment bool m_IsRegistered; segment_handle m_SegmentHandle; - INDEBUG(size_t m_ObjectsCount); friend class ProfilerObjectEnum; friend class ProfToEEInterfaceImpl; + friend class FrozenObjectHeapManager; }; #endif // _FROZENOBJECTHEAP_H From 2eb4791bcfb11fe32854e05360bc8e0e8b3bd7ad Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 23 Aug 2023 13:49:49 +0200 Subject: [PATCH 12/21] Oops, revert unrelated change --- .../src/System/Threading/Monitor.CoreCLR.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs index 02ce5435c70dd1..9fd823ba175708 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs @@ -41,6 +41,9 @@ public static partial class Monitor // in the typical case. Note that the method has to be transparent for inlining to be allowed by the VM. public static void Enter(object obj, ref bool lockTaken) { + if (lockTaken) + ThrowLockTakenException(); + ReliableEnter(obj, ref lockTaken); Debug.Assert(lockTaken); } From 841824a34559b8f266bcb7df381a1407ae728ec3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 23 Aug 2023 15:34:22 +0200 Subject: [PATCH 13/21] Use preemptive for the whole method --- src/coreclr/vm/frozenobjectheap.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index e9b56c8793d255..c2607a800441f7 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -10,10 +10,8 @@ #define FOH_COMMIT_SIZE (64 * 1024) FrozenObjectHeapManager::FrozenObjectHeapManager(): - // This lock is used in PREEMP mode m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), - // This lock is used in COOP mode - m_SegmentRegistrationCrst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), + m_SegmentRegistrationCrst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), m_CurrentSegment(nullptr) { } @@ -35,14 +33,14 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t return nullptr; #else // FEATURE_BASICFREEZE + GCX_PREEMP(); + Object* obj = nullptr; FrozenObjectSegment* curSeg = nullptr; uint8_t* curSegmentCurrent = nullptr; size_t curSegSizeCommitted = 0; { - GCX_PREEMP(); - CrstHolder ch(&m_Crst); _ASSERT(type != nullptr); From 3da2145a07e3b1a0d498a90c9353a645563071d2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 23 Aug 2023 19:01:36 +0200 Subject: [PATCH 14/21] PublishFrozenObject has to be called in COOP --- src/coreclr/vm/frozenobjectheap.cpp | 111 ++++++++++++++-------------- 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index c2607a800441f7..3c9511e8a6c9bc 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -26,14 +26,13 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t THROWS; MODE_COOPERATIVE; } - CONTRACTL_END + CONTRACTL_END #ifndef FEATURE_BASICFREEZE - // GC is required to support frozen segments - return nullptr; + // GC is required to support frozen segments + return nullptr; #else // FEATURE_BASICFREEZE - GCX_PREEMP(); Object* obj = nullptr; FrozenObjectSegment* curSeg = nullptr; @@ -41,66 +40,70 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t size_t curSegSizeCommitted = 0; { - CrstHolder ch(&m_Crst); - - _ASSERT(type != nullptr); - _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE); - - // Currently we don't support frozen objects with special alignment requirements - // TODO: We should also give up on arrays of doubles on 32-bit platforms. - // (we currently never allocate them on frozen segments) - #ifdef FEATURE_64BIT_ALIGNMENT - if (type->RequiresAlign8()) + GCX_PREEMP(); { - // Align8 objects are not supported yet - return nullptr; - } - #endif + CrstHolder ch(&m_Crst); - // NOTE: objectSize is expected be the full size including header - _ASSERT(objectSize >= MIN_OBJECT_SIZE); + _ASSERT(type != nullptr); + _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE); - if (objectSize > FOH_COMMIT_SIZE) - { - // The current design doesn't allow objects larger than FOH_COMMIT_SIZE and - // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects. - return nullptr; - } - - obj = m_CurrentSegment == nullptr ? nullptr : m_CurrentSegment->TryAllocateObject(type, objectSize); - // obj is nullptr if the current segment is full or hasn't been allocated yet - if (obj == nullptr) - { - size_t newSegmentSize = FOH_SEGMENT_DEFAULT_SIZE; - if (m_CurrentSegment != nullptr) + // Currently we don't support frozen objects with special alignment requirements + // TODO: We should also give up on arrays of doubles on 32-bit platforms. + // (we currently never allocate them on frozen segments) +#ifdef FEATURE_64BIT_ALIGNMENT + if (type->RequiresAlign8()) { - // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects - // Use the same size in case if prevSegmentSize*2 operation overflows. - const size_t prevSegmentSize = m_CurrentSegment->m_Size; - newSegmentSize = max(prevSegmentSize, prevSegmentSize * 2); + // Align8 objects are not supported yet + return nullptr; } +#endif - m_CurrentSegment = new FrozenObjectSegment(newSegmentSize); - m_FrozenSegments.Append(m_CurrentSegment); + // NOTE: objectSize is expected be the full size including header + _ASSERT(objectSize >= MIN_OBJECT_SIZE); - // Try again - obj = m_CurrentSegment->TryAllocateObject(type, objectSize); + if (objectSize > FOH_COMMIT_SIZE) + { + // The current design doesn't allow objects larger than FOH_COMMIT_SIZE and + // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects. + return nullptr; + } - // This time it's not expected to be null - _ASSERT(obj != nullptr); + obj = m_CurrentSegment == nullptr ? nullptr : m_CurrentSegment->TryAllocateObject(type, objectSize); + // obj is nullptr if the current segment is full or hasn't been allocated yet + if (obj == nullptr) + { + size_t newSegmentSize = FOH_SEGMENT_DEFAULT_SIZE; + if (m_CurrentSegment != nullptr) + { + // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects + // Use the same size in case if prevSegmentSize*2 operation overflows. + const size_t prevSegmentSize = m_CurrentSegment->m_Size; + newSegmentSize = max(prevSegmentSize, prevSegmentSize * 2); + } + + m_CurrentSegment = new FrozenObjectSegment(newSegmentSize); + m_FrozenSegments.Append(m_CurrentSegment); + + // Try again + obj = m_CurrentSegment->TryAllocateObject(type, objectSize); + + // This time it's not expected to be null + _ASSERT(obj != nullptr); + } + curSeg = m_CurrentSegment; + curSegSizeCommitted = curSeg->m_SizeCommitted; + curSegmentCurrent = curSeg->m_pCurrent; + } // end of m_Crst lock + + // Let GC know about the new segment or changes in it. + // We do it under a new lock because the main one (m_Crst) can be used by Profiler in a GC's thread + // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock. + { + CrstHolder regLock(&m_SegmentRegistrationCrst); + curSeg->RegisterOrUpdate(curSegmentCurrent, curSegSizeCommitted); } - curSeg = m_CurrentSegment; - curSegSizeCommitted = curSeg->m_SizeCommitted; - curSegmentCurrent = curSeg->m_pCurrent; - } - // Let GC know about the new segment or changes in it. - // We do it under a new lock because the main one (m_Crst) can be used by Profiler in a GC's thread - // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock. - { - CrstHolder regLock(&m_SegmentRegistrationCrst); - curSeg->RegisterOrUpdate(curSegmentCurrent, curSegSizeCommitted); - } + } // end of GCX_PREEMP if (publish) { From c980018a6a825602a0865c48a2c846cfa9500612 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 24 Aug 2023 20:37:03 +0200 Subject: [PATCH 15/21] Update src/coreclr/vm/frozenobjectheap.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/frozenobjectheap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 3c9511e8a6c9bc..2d0cb23becc9d5 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -11,7 +11,7 @@ FrozenObjectHeapManager::FrozenObjectHeapManager(): m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), - m_SegmentRegistrationCrst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE), + m_SegmentRegistrationCrst(CrstFrozenObjectHeap), m_CurrentSegment(nullptr) { } From e8d93857a317d9a0f4c8f15053a061660e9cae4b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 24 Aug 2023 21:21:04 +0200 Subject: [PATCH 16/21] Address feedback --- src/coreclr/vm/frozenobjectheap.cpp | 22 ++++++++-------------- src/coreclr/vm/profilingenumerators.cpp | 5 ----- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 24 ++++++------------------ 3 files changed, 14 insertions(+), 37 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 2d0cb23becc9d5..a14ed5e9783d0a 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -26,14 +26,13 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t THROWS; MODE_COOPERATIVE; } - CONTRACTL_END + CONTRACTL_END #ifndef FEATURE_BASICFREEZE - // GC is required to support frozen segments - return nullptr; + // GC is required to support frozen segments + return nullptr; #else // FEATURE_BASICFREEZE - Object* obj = nullptr; FrozenObjectSegment* curSeg = nullptr; uint8_t* curSegmentCurrent = nullptr; @@ -169,8 +168,8 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited if (!IsRegistered()) { // Other threads won't touch these fields until we set m_IsRegistered to true - m_SizeCommittedRegistered = sizeCommited; - m_pCurrentRegistered = current; + VolatileStore(&m_SizeCommittedRegistered, sizeCommited); + VolatileStore(&m_pCurrentRegistered, current); segment_info si; si.pvMem = m_pStart; @@ -191,16 +190,11 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited { if (current > VolatileLoad(&m_pCurrentRegistered)) { - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( - m_SegmentHandle, current, m_pStart + sizeCommited); - - // Profiler thread won't hold the registration lock, but, presumably, it won't see these values randomly - // because UpdateFrozenSegment will stuck in GC's lock while profiler is enumerating frozen segments - // (it's generally recommended to enumerate heaps in GarbageCollectionFinished/Started events). - // If profiler is invoked outside of GC's lock, then the accuracy is not guaranteed, but let's at least - // bump SizeCommited first: VolatileStore(&m_SizeCommittedRegistered, sizeCommited); VolatileStore(&m_pCurrentRegistered, current); + + GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( + m_SegmentHandle, current, m_pStart + sizeCommited); } else { diff --git a/src/coreclr/vm/profilingenumerators.cpp b/src/coreclr/vm/profilingenumerators.cpp index a18637f45df19c..c55d40ffbeed9e 100644 --- a/src/coreclr/vm/profilingenumerators.cpp +++ b/src/coreclr/vm/profilingenumerators.cpp @@ -118,11 +118,6 @@ BOOL ProfilerObjectEnum::Init() for (unsigned segmentIdx = 0; segmentIdx < segmentsCount; segmentIdx++) { const FrozenObjectSegment* segment = segments[segmentIdx]; - if (!segment->IsRegistered()) - { - continue; - } - Object* currentObj = segment->GetFirstObject(); while (currentObj != nullptr) { diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index 7f23631ee41015..11853acdcf86ee 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -7684,37 +7684,25 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges, FrozenObjectSegment** segments = foh->m_FrozenSegments.GetElements(); if (segments != nullptr && segmentsCount > 0) { - unsigned totalRegisteredSegments = 0; + const ULONG segmentsToInspect = min(cObjectRanges, (ULONG)segmentsCount); - for (unsigned segIdx = 0; segIdx < segmentsCount; segIdx++) + for (unsigned segIdx = 0; segIdx < segmentsToInspect; segIdx++) { - if (segIdx >= cObjectRanges) - { - break; - } - - FrozenObjectSegment* segment = segments[segIdx]; - if (!segment->IsRegistered()) - { - continue; - } - totalRegisteredSegments++; - - uint8_t* firstObj = segment->m_pStart + sizeof(ObjHeader); + uint8_t* firstObj = segments[segIdx]->m_pStart + sizeof(ObjHeader); // Start of the segment (first object) ranges[segIdx].rangeStart = (ObjectID)firstObj; // Total size reserved for a segment - ranges[segIdx].rangeLengthReserved = (UINT_PTR)segment->m_Size - sizeof(ObjHeader); + ranges[segIdx].rangeLengthReserved = (UINT_PTR)segments[segIdx]->m_Size - sizeof(ObjHeader); // Size of the segment that is currently in use - ranges[segIdx].rangeLength = (UINT_PTR)(segment->m_pCurrentRegistered - firstObj); + ranges[segIdx].rangeLength = (UINT_PTR)(segments[segIdx]->m_pCurrent - firstObj); } if (pcObjectRanges != nullptr) { - *pcObjectRanges = (ULONG)totalRegisteredSegments; + *pcObjectRanges = (ULONG)segmentsCount; } } else From a866d5ee020721cd36feeaeb69c4cde8067fa669 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 24 Aug 2023 21:46:10 +0200 Subject: [PATCH 17/21] Add asserts --- src/coreclr/gc/gc.cpp | 4 +++- src/coreclr/vm/frozenobjectheap.cpp | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index d4e7ec9622e648..daeadfe9821b8c 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -9960,7 +9960,9 @@ void gc_heap::update_ro_segment (heap_segment* seg, uint8_t* allocated, uint8_t* enter_spin_lock (&gc_heap::gc_lock); assert (use_frozen_segments_p); - assert (heap_segment_read_only_p(seg)); + assert (heap_segment_read_only_p (seg)); + assert (allocated <= committed); + assert (committed <= heap_segment_reserved (seg)); heap_segment_allocated (seg) = allocated; heap_segment_committed (seg) = committed; diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index a14ed5e9783d0a..bd2f2de2c018a6 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -165,6 +165,13 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited) { + CONTRACTL + { + THROWS; + MODE_PREEMPTIVE; + } + CONTRACTL_END + if (!IsRegistered()) { // Other threads won't touch these fields until we set m_IsRegistered to true From 514eff3ef7f6d9f5eb8731b4185e7d2765f1fb9b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Aug 2023 03:06:38 +0200 Subject: [PATCH 18/21] Address feedback --- src/coreclr/vm/frozenobjectheap.cpp | 19 +++++++++++-------- src/coreclr/vm/frozenobjectheap.h | 3 ++- src/coreclr/vm/gchelpers.cpp | 23 +++++++++++++---------- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index bd2f2de2c018a6..1282ed1ad6013c 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -19,7 +19,9 @@ FrozenObjectHeapManager::FrozenObjectHeapManager(): // Allocates an object of the give size (including header) on a frozen segment. // May return nullptr if object is too large (larger than FOH_COMMIT_SIZE) // in such cases caller is responsible to find a more appropriate heap to allocate it -Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t objectSize, bool publish) + +Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t objectSize, + void(*initFunc)(Object*, void*), void* pParam) { CONTRACTL { @@ -85,6 +87,10 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t // Try again obj = m_CurrentSegment->TryAllocateObject(type, objectSize); + if (initFunc != nullptr) + { + initFunc(obj, pParam); + } // This time it's not expected to be null _ASSERT(obj != nullptr); @@ -104,10 +110,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t } // end of GCX_PREEMP - if (publish) - { - PublishFrozenObject(obj); - } + PublishFrozenObject(obj); return obj; #endif // !FEATURE_BASICFREEZE @@ -254,7 +257,7 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje Object* FrozenObjectSegment::GetFirstObject() const { - if (m_pStart + sizeof(ObjHeader) == m_pCurrentRegistered) + if (m_pStart + sizeof(ObjHeader) == m_pCurrent) { // Segment is empty return nullptr; @@ -266,11 +269,11 @@ Object* FrozenObjectSegment::GetNextObject(Object* obj) const { // Input must not be null and should be within the segment _ASSERT(obj != nullptr); - _ASSERT((uint8_t*)obj >= m_pStart + sizeof(ObjHeader) && (uint8_t*)obj < m_pCurrentRegistered); + _ASSERT((uint8_t*)obj >= m_pStart + sizeof(ObjHeader) && (uint8_t*)obj < m_pCurrent); // FOH doesn't support objects with non-DATA_ALIGNMENT alignment yet. uint8_t* nextObj = (reinterpret_cast(obj) + ALIGN_UP(obj->GetSize(), DATA_ALIGNMENT)); - if (nextObj < m_pCurrentRegistered) + if (nextObj < m_pCurrent) { return reinterpret_cast(nextObj); } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index d72aa0a8f088a5..b8cf369dcc4a22 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -27,7 +27,8 @@ class FrozenObjectHeapManager { public: FrozenObjectHeapManager(); - Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize, bool publish = true); + Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize, + void(*initFunc)(Object*,void*) = nullptr, void* pParam = nullptr); private: Crst m_Crst; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index e3c882f623b244..539bfd64e0da40 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -555,18 +555,18 @@ OBJECTREF TryAllocateFrozenSzArray(MethodTable* pArrayMT, INT32 cElements) #endif FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); - ArrayBase* orArray = static_cast(foh->TryAllocateObject(pArrayMT, PtrAlign(totalSize), /*publish*/ false)); + ArrayBase* orArray = static_cast( + foh->TryAllocateObject(pArrayMT, PtrAlign(totalSize), [](Object* obj, void* elemCntPtr){ + // Initialize newly allocated object before publish + static_cast(obj)->m_NumComponents = *static_cast(elemCntPtr); + }, &cElements)); + if (orArray == nullptr) { // We failed to allocate on a frozen segment, fallback to AllocateSzArray // E.g. if the array is too big to fit on a frozen segment return NULL; } - orArray->m_NumComponents = cElements; - - // Publish needs to be postponed in this case because we need to specify array length - PublishObjectAndNotify(orArray, GC_ALLOC_NO_FLAGS); - return ObjectToOBJECTREF(orArray); } @@ -968,12 +968,15 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIs if (preferFrozenHeap) { FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); - orString = static_cast(foh->TryAllocateObject(g_pStringClass, totalSize, /* publish = */false)); + + orString = static_cast(foh->TryAllocateObject( + g_pStringClass, totalSize, [](Object* obj, void* pStrLen) { + // Initialize newly allocated object before publish + static_cast(obj)->SetStringLength(*static_cast(pStrLen)); + }, &cchStringLength)); + if (orString != nullptr) { - orString->SetStringLength(cchStringLength); - // Publish needs to be postponed in this case because we need to specify string length - PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); orStringRef = ObjectToSTRINGREF(orString); *pIsFrozen = true; From 92f6f1cd7f01fb2b07c3e495308cb051915efa73 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Aug 2023 03:12:48 +0200 Subject: [PATCH 19/21] fix build --- src/coreclr/vm/gchelpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 539bfd64e0da40..8e8a464c4f8852 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -1142,7 +1142,7 @@ OBJECTREF TryAllocateFrozenObject(MethodTable* pObjMT) #endif // FEATURE_64BIT_ALIGNMENT FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); - Object* orObject = foh->TryAllocateObject(pObjMT, PtrAlign(pObjMT->GetBaseSize()), /*publish*/ true); + Object* orObject = foh->TryAllocateObject(pObjMT, PtrAlign(pObjMT->GetBaseSize())); return ObjectToOBJECTREF(orObject); } From 795d8f5525a6443616dfeb7cd63c5d4ccdd59482 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Aug 2023 11:33:48 +0200 Subject: [PATCH 20/21] Address feedback --- src/coreclr/vm/frozenobjectheap.cpp | 26 ++++++++++---------------- src/coreclr/vm/frozenobjectheap.h | 12 ++++-------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 1282ed1ad6013c..be5058878ac19b 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -87,14 +87,16 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t // Try again obj = m_CurrentSegment->TryAllocateObject(type, objectSize); - if (initFunc != nullptr) - { - initFunc(obj, pParam); - } // This time it's not expected to be null _ASSERT(obj != nullptr); } + + if (initFunc != nullptr) + { + initFunc(obj, pParam); + } + curSeg = m_CurrentSegment; curSegSizeCommitted = curSeg->m_SizeCommitted; curSegmentCurrent = curSeg->m_pCurrent; @@ -123,9 +125,7 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) : m_pCurrent(nullptr), m_pCurrentRegistered(nullptr), m_SizeCommitted(0), - m_SizeCommittedRegistered(0), m_Size(sizeHint), - m_IsRegistered(false), m_SegmentHandle(nullptr) { _ASSERT(m_Size > FOH_COMMIT_SIZE); @@ -175,17 +175,13 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited } CONTRACTL_END - if (!IsRegistered()) + if (VolatileLoad(&m_pCurrentRegistered) == nullptr) { - // Other threads won't touch these fields until we set m_IsRegistered to true - VolatileStore(&m_SizeCommittedRegistered, sizeCommited); - VolatileStore(&m_pCurrentRegistered, current); - segment_info si; si.pvMem = m_pStart; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = (size_t)m_pCurrentRegistered; - si.ibCommit = m_SizeCommittedRegistered; + si.ibCommit = sizeCommited; si.ibReserved = m_Size; // NOTE: RegisterFrozenSegment may take a GC lock inside. @@ -194,17 +190,15 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited { ThrowOutOfMemory(); } - VolatileStore(&m_IsRegistered, true); + VolatileStore(&m_pCurrentRegistered, current); } else { if (current > VolatileLoad(&m_pCurrentRegistered)) { - VolatileStore(&m_SizeCommittedRegistered, sizeCommited); - VolatileStore(&m_pCurrentRegistered, current); - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( m_SegmentHandle, current, m_pStart + sizeCommited); + VolatileStore(&m_pCurrentRegistered, current); } else { diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index b8cf369dcc4a22..e191731d64dd5b 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -45,10 +45,6 @@ class FrozenObjectSegment public: FrozenObjectSegment(size_t sizeHint); Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); - bool IsRegistered() const - { - return VolatileLoad(&m_IsRegistered); - } void RegisterOrUpdate(uint8_t* current, size_t sizeCommited); private: @@ -66,20 +62,20 @@ class FrozenObjectSegment // // m_pCurrent <= m_SizeCommitted uint8_t* m_pCurrent; + + // Last known value of m_pCurrent that GC is aware of. + // + // m_pCurrentRegistered <= m_pCurrent uint8_t* m_pCurrentRegistered; // Memory committed in the current segment // // m_SizeCommitted <= m_pStart + FOH_SIZE_RESERVED size_t m_SizeCommitted; - size_t m_SizeCommittedRegistered; // Total memory reserved for the current segment size_t m_Size; - // Whether GC knows about this segment already or it hasn't been registered yet - bool m_IsRegistered; - segment_handle m_SegmentHandle; friend class ProfilerObjectEnum; From fdc290b3dc97d523aef5a343f5fd74f128586097 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 25 Aug 2023 17:16:27 +0200 Subject: [PATCH 21/21] Update src/coreclr/vm/frozenobjectheap.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/frozenobjectheap.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index be5058878ac19b..41af231816a645 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -175,12 +175,12 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited } CONTRACTL_END - if (VolatileLoad(&m_pCurrentRegistered) == nullptr) + if (m_pCurrentRegistered == nullptr) { segment_info si; si.pvMem = m_pStart; si.ibFirstObject = sizeof(ObjHeader); - si.ibAllocated = (size_t)m_pCurrentRegistered; + si.ibAllocated = (size_t)current; si.ibCommit = sizeCommited; si.ibReserved = m_Size; @@ -190,15 +190,15 @@ void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited { ThrowOutOfMemory(); } - VolatileStore(&m_pCurrentRegistered, current); + m_pCurrentRegistered = current; } else { - if (current > VolatileLoad(&m_pCurrentRegistered)) + if (current > m_pCurrentRegistered) { GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment( m_SegmentHandle, current, m_pStart + sizeCommited); - VolatileStore(&m_pCurrentRegistered, current); + m_pCurrentRegistered = current; } else {