From 166ccfe7078338d2cc97e60a743c68c308bed787 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 3 Jul 2021 23:56:13 +0300 Subject: [PATCH 01/57] Naive implementation of FrozenObjectHeap --- src/coreclr/jit/assertionprop.cpp | 14 +++++++++++-- src/coreclr/jit/codegenarmarch.cpp | 5 ----- src/coreclr/jit/codegenxarch.cpp | 5 ----- src/coreclr/vm/CMakeLists.txt | 2 ++ src/coreclr/vm/appdomain.cpp | 26 +++++++++++++++++++++++ src/coreclr/vm/appdomain.hpp | 13 ++++++++++++ src/coreclr/vm/frozenobjectheap.cpp | 24 ++++++++++++++++++++++ src/coreclr/vm/frozenobjectheap.h | 22 ++++++++++++++++++++ src/coreclr/vm/gchelpers.cpp | 29 ++++++++++++++++++++------ src/coreclr/vm/gchelpers.h | 2 +- src/coreclr/vm/jitinterface.cpp | 13 +++++++++++- src/coreclr/vm/object.cpp | 2 +- src/coreclr/vm/stringliteralmap.cpp | 32 +++++++---------------------- src/coreclr/vm/stringliteralmap.h | 4 ++-- 14 files changed, 145 insertions(+), 48 deletions(-) create mode 100644 src/coreclr/vm/frozenobjectheap.cpp create mode 100644 src/coreclr/vm/frozenobjectheap.h diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 981879142bf26..fb312f479a93b 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -734,8 +734,14 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse if (op1Type == TYP_REF) { - assert(curAssertion->op2.u1.iconVal == 0); - printf("null"); + if (curAssertion->op2.u1.iconVal == 0) + { + printf("null"); + } + else + { + printf("[%08p]", dspPtr(curAssertion->op2.u1.iconVal)); + } } else { @@ -2716,6 +2722,10 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, { return nullptr; } + if ((curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK) == GTF_ICON_STR_HDL) + { + return nullptr; + } if (curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK) { diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 71f775d70d872..fb34076714a8b 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -565,11 +565,6 @@ void CodeGen::genSetRegToIcon(regNumber reg, ssize_t val, var_types type, insFla // Reg cannot be a FP reg assert(!genIsValidFloatReg(reg)); - // The only TYP_REF constant that can come this path is a managed 'null' since it is not - // relocatable. Other ref type constants (e.g. string objects) go through a different - // code path. - noway_assert(type != TYP_REF || val == 0); - instGen_Set_Reg_To_Imm(emitActualTypeSize(type), reg, val, flags); } diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 2bd0142381f62..fb514d30a0e92 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -33,11 +33,6 @@ void CodeGen::genSetRegToIcon(regNumber reg, ssize_t val, var_types type, insFla // Reg cannot be a FP reg assert(!genIsValidFloatReg(reg)); - // The only TYP_REF constant that can come this path is a managed 'null' since it is not - // relocatable. Other ref type constants (e.g. string objects) go through a different - // code path. - noway_assert(type != TYP_REF || val == 0); - if (val == 0) { instGen_Set_Reg_To_Zero(emitActualTypeSize(type), reg, flags); diff --git a/src/coreclr/vm/CMakeLists.txt b/src/coreclr/vm/CMakeLists.txt index 1d682d2a428bb..d9853f49c7176 100644 --- a/src/coreclr/vm/CMakeLists.txt +++ b/src/coreclr/vm/CMakeLists.txt @@ -336,6 +336,7 @@ set(VM_SOURCES_WKS fcall.cpp fieldmarshaler.cpp finalizerthread.cpp + frozenobjectheap.cpp gccover.cpp gcenv.ee.static.cpp gcenv.ee.common.cpp @@ -446,6 +447,7 @@ set(VM_HEADERS_WKS fcall.h fieldmarshaler.h finalizerthread.h + frozenobjectheap.h gcenv.h gcenv.ee.h gcenv.os.h diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index f7bf57c5b52cf..a242772162e06 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -114,6 +114,7 @@ int BaseDomain::m_iNumberOfProcessors = 0; // System Domain Statics GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; +FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; DECLSPEC_ALIGN(16) static BYTE g_pSystemDomainMemory[sizeof(SystemDomain)]; @@ -1320,6 +1321,31 @@ void SystemDomain::LazyInitGlobalStringLiteralMap() } } +void SystemDomain::LazyInitFrozenObjectsHeap() +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + MODE_ANY; + INJECT_FAULT(COMPlusThrowOM();); + } + CONTRACTL_END; + + const size_t fohSize = 16 * 1024 * 1024; + + // TODO: remove commit + void* alloc = ClrVirtualAlloc(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); + NewHolder pFoh(new FrozenObjectHeap()); + + pFoh->Init(alloc, fohSize); + + if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + { + pFoh.SuppressRelease(); + } +} + /*static*/ void SystemDomain::EnumAllStaticGCRefs(promote_func* fn, ScanContext* sc) { CONTRACT_VOID diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 0487299c5f26c..8acc80ed09898 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -34,6 +34,7 @@ #include "tieredcompilation.h" #include "codeversion.h" +#include "frozenobjectheap.h" class BaseDomain; class SystemDomain; @@ -2528,6 +2529,7 @@ class SystemDomain : public BaseDomain void Init(); void Stop(); static void LazyInitGlobalStringLiteralMap(); + static void LazyInitFrozenObjectsHeap(); //**************************************************************************************** // @@ -2593,6 +2595,16 @@ class SystemDomain : public BaseDomain _ASSERTE(m_pGlobalStringLiteralMap); return m_pGlobalStringLiteralMap; } + static FrozenObjectHeap* GetSegmentWithFrozenObjects() + { +#ifdef FEATURE_BASICFREEZE + if (m_FrozenObjects == NULL) + { + SystemDomain::LazyInitFrozenObjectsHeap(); + } +#endif + return m_FrozenObjects; + } static GlobalStringLiteralMap *GetGlobalStringLiteralMapNoCreate() { LIMITED_METHOD_CONTRACT; @@ -2780,6 +2792,7 @@ class SystemDomain : public BaseDomain static CrstStatic m_SystemDomainCrst; static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; + static FrozenObjectHeap *m_FrozenObjects; static ULONG s_dNumAppDomains; // Maintain a count of children app domains. diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp new file mode 100644 index 0000000000000..28a7c80ba4c02 --- /dev/null +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -0,0 +1,24 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#include "frozenobjectheap.h" +#include "memorypool.h" + +void FrozenObjectHeap::Init(void* start, size_t size) +{ + m_pStart = static_cast(start); + m_pCurrent = m_pStart; + m_Size = size; +} + +void* FrozenObjectHeap::Alloc(size_t size) +{ + uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); + if ((obj + size + OBJHEADER_SIZE) > (m_pStart + m_Size)) + { + return nullptr; + } + m_pCurrent = obj + OBJECT_BASESIZE + size; + ZeroMemory(obj, size); + return obj + OBJECT_BASESIZE; +} diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h new file mode 100644 index 0000000000000..019478b371b71 --- /dev/null +++ b/src/coreclr/vm/frozenobjectheap.h @@ -0,0 +1,22 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef _FROZENOBJECTHEAP_H +#define _FROZENOBJECTHEAP_H + +#include "appdomain.hpp" + +class FrozenObjectHeap +{ +public: + void Init(void* start, size_t size); + void* Alloc(size_t size); + +private: + uint8_t* m_pStart; + uint8_t* m_pCurrent; + size_t m_Size; +}; + +#endif // _FROZENOBJECTHEAP_H + diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 0cecfc624a744..546dadb67dc69 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -28,6 +28,7 @@ #include "gchelpers.inl" #include "eeprofinterfaces.inl" +#include "frozenobjectheap.h" #ifdef FEATURE_COMINTEROP #include "runtimecallablewrapper.h" @@ -825,7 +826,7 @@ OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle elementType, BOOL bAll return AllocateSzArray(arrayType, (INT32) cElements, flags); } -STRINGREF AllocateString( DWORD cchStringLength ) +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap) { CONTRACTL { THROWS; @@ -851,13 +852,29 @@ STRINGREF AllocateString( DWORD cchStringLength ) SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); _ASSERTE(totalSize > cchStringLength); - SetTypeHandleOnThreadForAlloc(TypeHandle(g_pStringClass)); - GC_ALLOC_FLAGS flags = GC_ALLOC_NO_FLAGS; - if (totalSize >= g_pConfig->GetGCLOHThreshold()) - flags |= GC_ALLOC_LARGE_OBJECT_HEAP; + StringObject* orString = nullptr; + + // Try to allocate the string in Frozen Object Heap. + if (preferFrozenObjHeap && (cchStringLength < 2048)) + { + FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); + if (foh != nullptr) + { + orString = static_cast(foh->Alloc(totalSize)); + } + // if FOH is null or full - fallback to normal allocation. + } + + if (orString == nullptr) + { + SetTypeHandleOnThreadForAlloc(TypeHandle(g_pStringClass)); - StringObject* orString = (StringObject*)Alloc(totalSize, flags); + if (totalSize >= g_pConfig->GetGCLOHThreshold()) + flags |= GC_ALLOC_LARGE_OBJECT_HEAP; + + orString = static_cast(Alloc(totalSize, flags)); + } // Initialize Object orString->SetMethodTable(g_pStringClass); diff --git a/src/coreclr/vm/gchelpers.h b/src/coreclr/vm/gchelpers.h index bfebf554b0cbf..0233c97a6ac38 100644 --- a/src/coreclr/vm/gchelpers.h +++ b/src/coreclr/vm/gchelpers.h @@ -34,7 +34,7 @@ OBJECTREF AllocatePrimitiveArray(CorElementType type, DWORD cElements); OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle ElementType, BOOL bAllocateInPinnedHeap = FALSE); // Allocate a string -STRINGREF AllocateString( DWORD cchStringLength ); +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap = false); OBJECTREF DupArrayForCloning(BASEARRAYREF pRef); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index a1e4d93d881de..fd0cacdbf4d0f 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11935,7 +11935,18 @@ InfoAccessType CEEJitInfo::constructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd } else { - *ppValue = (LPVOID)ConstructStringLiteral(scopeHnd, metaTok); // throws + void** ptr = (void**)ConstructStringLiteral(scopeHnd, metaTok); // throws + + // If it's in the frozen segment we can pass the direct reference. + if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment((Object*)*ptr)) + { + *ppValue = *ptr; + result = IAT_VALUE; + } + else + { + *ppValue = (void*)ptr; + } } EE_TO_JIT_TRANSITION(); diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 36b100d6e9125..1b009f9d90c83 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -519,7 +519,7 @@ VOID Object::ValidateInner(BOOL bDeep, BOOL bVerifyNextHeader, BOOL bVerifySyncB // noRangeChecks depends on initial values being FALSE BOOL bSmallObjectHeapPtr = FALSE, bLargeObjectHeapPtr = FALSE; - if (!noRangeChecks) + if (!noRangeChecks && !GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(this)) { bSmallObjectHeapPtr = GCHeapUtilities::GetGCHeap()->IsHeapPointer(this, true); if (!bSmallObjectHeapPtr) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index ca1d80f686792..0df0936829f04 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -387,7 +387,7 @@ StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStri else { if (bAddIfNotFound) - pEntry = AddStringLiteral(pStringData); + pEntry = AddStringLiteral(pStringData, true); } return pEntry; @@ -443,7 +443,8 @@ static void LogStringLiteral(__in_z const char* action, EEStringData *pStringDat } #endif -STRINGREF AllocateStringObject(EEStringData *pStringData) + +STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap) { CONTRACTL { @@ -456,7 +457,7 @@ STRINGREF AllocateStringObject(EEStringData *pStringData) // Create the COM+ string object. DWORD cCount = pStringData->GetCharCount(); - STRINGREF strObj = AllocateString(cCount); + STRINGREF strObj = AllocateString(cCount, preferFrozenObjHeap); GCPROTECT_BEGIN(strObj) { @@ -472,7 +473,7 @@ STRINGREF AllocateStringObject(EEStringData *pStringData) return strObj; } -StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStringData) +StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStringData, bool preferFrozenObjHeap) { CONTRACTL { @@ -489,7 +490,7 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri { PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); // Create the COM+ string object. - STRINGREF strObj = AllocateStringObject(pStringData); + STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap); // Allocate a handle for the string. SetObjectReference(pStrObj[0], (OBJECTREF) strObj); @@ -525,26 +526,7 @@ StringLiteralEntry *GlobalStringLiteralMap::AddInternedString(STRINGREF *pString CONTRACTL_END; EEStringData StringData = EEStringData((*pString)->GetStringLength(), (*pString)->GetBuffer()); - StringLiteralEntry *pRet; - - { - PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); - SetObjectReference(pStrObj[0], (OBJECTREF) *pString); - - // Since the allocation might have caused a GC we need to re-get the - // string data. - StringData = EEStringData((*pString)->GetStringLength(), (*pString)->GetBuffer()); - - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(&StringData, (STRINGREF*)pStrObj[0])); - pStrObj.SuppressRelease(); - - // Insert the handle to the string into the hash table. - m_StringToEntryHashTable->InsertValue(&StringData, (LPVOID)pEntry, FALSE); - pEntry.SuppressRelease(); - pRet = pEntry; - } - - return pRet; + return AddStringLiteral(&StringData, true); } void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry) diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index 22622733c779b..69a2040663a36 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -22,7 +22,7 @@ class StringLiteralEntry; // Allocate 16 entries (approx size sizeof(StringLiteralEntry)*16) #define MAX_ENTRIES_PER_CHUNK 16 -STRINGREF AllocateStringObject(EEStringData *pStringData); +STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap); // Loader allocator specific string literal map. class StringLiteralMap @@ -92,7 +92,7 @@ class GlobalStringLiteralMap private: // Helper method to add a string to the global string literal map. - StringLiteralEntry *AddStringLiteral(EEStringData *pStringData); + StringLiteralEntry *AddStringLiteral(EEStringData *pStringData, bool preferFrozenObjHeap); // Helper method to add an interned string. StringLiteralEntry *AddInternedString(STRINGREF *pString); From 1cb0927ff2b0ca51765d84c45ba3e0a595995d69 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 00:47:51 +0300 Subject: [PATCH 02/57] Add RegisterFrozenSegment --- src/coreclr/vm/appdomain.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index a242772162e06..3931a1f6e93d3 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1332,17 +1332,21 @@ void SystemDomain::LazyInitFrozenObjectsHeap() } CONTRACTL_END; + GCX_PREEMP(); + const size_t fohSize = 16 * 1024 * 1024; // TODO: remove commit void* alloc = ClrVirtualAlloc(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); - NewHolder pFoh(new FrozenObjectHeap()); - - pFoh->Init(alloc, fohSize); - - if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + if (alloc != nullptr) { - pFoh.SuppressRelease(); + GCInterface::RegisterFrozenSegment(alloc, fohSize); + NewHolder pFoh(new FrozenObjectHeap()); + pFoh->Init(alloc, fohSize); + if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + { + pFoh.SuppressRelease(); + } } } From d2b9cbfbc91715e37bf7b5e873d511750f6c8e06 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 12:22:54 +0300 Subject: [PATCH 03/57] Clean up --- src/coreclr/vm/appdomain.cpp | 17 ++------- src/coreclr/vm/appdomain.hpp | 4 +- src/coreclr/vm/frozenobjectheap.cpp | 58 ++++++++++++++++++++++++----- src/coreclr/vm/frozenobjectheap.h | 13 +++++-- src/coreclr/vm/gchelpers.cpp | 8 ++-- src/coreclr/vm/jitinterface.cpp | 3 +- 6 files changed, 72 insertions(+), 31 deletions(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 3931a1f6e93d3..019a7de5c6122 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -114,7 +114,7 @@ int BaseDomain::m_iNumberOfProcessors = 0; // System Domain Statics GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; -FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; +FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; DECLSPEC_ALIGN(16) static BYTE g_pSystemDomainMemory[sizeof(SystemDomain)]; @@ -1334,19 +1334,10 @@ void SystemDomain::LazyInitFrozenObjectsHeap() GCX_PREEMP(); - const size_t fohSize = 16 * 1024 * 1024; - - // TODO: remove commit - void* alloc = ClrVirtualAlloc(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); - if (alloc != nullptr) + NewHolder pFoh(new FrozenObjectHeap()); + if (pFoh->Init() && InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) { - GCInterface::RegisterFrozenSegment(alloc, fohSize); - NewHolder pFoh(new FrozenObjectHeap()); - pFoh->Init(alloc, fohSize); - if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) - { - pFoh.SuppressRelease(); - } + pFoh.SuppressRelease(); } } diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 8acc80ed09898..8b3a077b07209 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2597,6 +2597,8 @@ class SystemDomain : public BaseDomain } static FrozenObjectHeap* GetSegmentWithFrozenObjects() { + WRAPPER_NO_CONTRACT; + #ifdef FEATURE_BASICFREEZE if (m_FrozenObjects == NULL) { @@ -2792,7 +2794,7 @@ class SystemDomain : public BaseDomain static CrstStatic m_SystemDomainCrst; static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; - static FrozenObjectHeap *m_FrozenObjects; + static FrozenObjectHeap *m_FrozenObjects; static ULONG s_dNumAppDomains; // Maintain a count of children app domains. diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 28a7c80ba4c02..dc8f73b51da39 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -4,21 +4,61 @@ #include "frozenobjectheap.h" #include "memorypool.h" -void FrozenObjectHeap::Init(void* start, size_t size) +bool FrozenObjectHeap::Init(size_t fohSize) { - m_pStart = static_cast(start); - m_pCurrent = m_pStart; - m_Size = size; + _ASSERT(fohSize > (FOX_MAX_OBJECT_SIZE * 2)); + + // TODO: remove commit + void* alloc = ClrVirtualAlloc(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); + if (alloc != nullptr) + { + m_SegmentHandle = GCInterface::RegisterFrozenSegment(alloc, fohSize); + if (m_SegmentHandle != nullptr) + { + m_pStart = static_cast(alloc); + m_pCurrent = m_pStart; + m_Size = fohSize; + return true; + } + } + return false; } -void* FrozenObjectHeap::Alloc(size_t size) +Object* FrozenObjectHeap::AllocateObject(size_t objectSize) { + if (objectSize > FOX_MAX_OBJECT_SIZE) + { + // object is to big + return nullptr; + } + uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); - if ((obj + size + OBJHEADER_SIZE) > (m_pStart + m_Size)) + if ((obj + objectSize + OBJHEADER_SIZE) > (m_pStart + m_Size)) { + // heap is full return nullptr; } - m_pCurrent = obj + OBJECT_BASESIZE + size; - ZeroMemory(obj, size); - return obj + OBJECT_BASESIZE; + m_pCurrent = obj + OBJECT_BASESIZE + objectSize; + ZeroMemory(obj, objectSize); + return reinterpret_cast(obj + OBJECT_BASESIZE); +} + +bool FrozenObjectHeap::IsInHeap(Object* object) +{ + auto ptr = reinterpret_cast(object); + if (ptr >= m_pStart && ptr < m_pCurrent) + { + _ASSERT(GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(object)); + return true; + } + return false; +} + +FrozenObjectHeap::~FrozenObjectHeap() +{ + if (m_SegmentHandle != nullptr) + { + GCInterface::UnregisterFrozenSegment(m_SegmentHandle); + ClrVirtualFree(m_pStart, 0, MEM_RELEASE); + } } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 019478b371b71..5a45897471c90 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -4,18 +4,25 @@ #ifndef _FROZENOBJECTHEAP_H #define _FROZENOBJECTHEAP_H -#include "appdomain.hpp" +#define FOX_MAX_OBJECT_SIZE 2 * 1024 +#define FOH_DEFAULT_SIZE 1 * 1024 * 1024 class FrozenObjectHeap { public: - void Init(void* start, size_t size); - void* Alloc(size_t size); + bool Init(size_t fohSize = FOH_DEFAULT_SIZE); + + Object* AllocateObject(size_t objectSize); + + bool IsInHeap(Object* object); + + ~FrozenObjectHeap(); private: uint8_t* m_pStart; uint8_t* m_pCurrent; size_t m_Size; + void* m_SegmentHandle; }; #endif // _FROZENOBJECTHEAP_H diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 546dadb67dc69..a2e1b4d6fda5d 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -856,14 +856,14 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap) StringObject* orString = nullptr; // Try to allocate the string in Frozen Object Heap. - if (preferFrozenObjHeap && (cchStringLength < 2048)) + if (preferFrozenObjHeap) { FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); if (foh != nullptr) { - orString = static_cast(foh->Alloc(totalSize)); + orString = (StringObject*)foh->AllocateObject(totalSize); + // if FOH is null, full, or object is too big - fallback to normal allocation. } - // if FOH is null or full - fallback to normal allocation. } if (orString == nullptr) @@ -873,7 +873,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap) if (totalSize >= g_pConfig->GetGCLOHThreshold()) flags |= GC_ALLOC_LARGE_OBJECT_HEAP; - orString = static_cast(Alloc(totalSize, flags)); + orString = (StringObject*)(Alloc(totalSize, flags)); } // Initialize Object diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index fd0cacdbf4d0f..ba482bae1b946 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -71,6 +71,7 @@ #endif #include "tailcallhelp.h" +#include "frozenobjectheap.h" // The Stack Overflow probe takes place in the COOPERATIVE_TRANSITION_BEGIN() macro // @@ -11938,7 +11939,7 @@ InfoAccessType CEEJitInfo::constructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd void** ptr = (void**)ConstructStringLiteral(scopeHnd, metaTok); // throws // If it's in the frozen segment we can pass the direct reference. - if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment((Object*)*ptr)) + if (SystemDomain::GetSegmentWithFrozenObjects()->IsInHeap((Object*)*ptr)) { *ppValue = *ptr; result = IAT_VALUE; From 641365a38873c222e63b8da0629b23d261d3e450 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 12:44:16 +0300 Subject: [PATCH 04/57] Add #include "common.h" --- src/coreclr/vm/frozenobjectheap.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 5a45897471c90..0040737da2d40 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -4,6 +4,8 @@ #ifndef _FROZENOBJECTHEAP_H #define _FROZENOBJECTHEAP_H +#include "common.h" + #define FOX_MAX_OBJECT_SIZE 2 * 1024 #define FOH_DEFAULT_SIZE 1 * 1024 * 1024 From 33a126b9553a96bc79433c9404561f9987bb2056 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 13:00:48 +0300 Subject: [PATCH 05/57] Fix build error --- src/coreclr/vm/appdomain.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 8b3a077b07209..f187d5ffd7a6c 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -34,7 +34,6 @@ #include "tieredcompilation.h" #include "codeversion.h" -#include "frozenobjectheap.h" class BaseDomain; class SystemDomain; @@ -42,6 +41,7 @@ class AppDomain; class CompilationDomain; class GlobalStringLiteralMap; class StringLiteralMap; +class FrozenObjectHeap; class MngStdInterfacesInfo; class DomainAssembly; class LoadLevelLimiter; From bb1b8359647cd9fcb4052bb7f1209ca9a635b434 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 13:41:45 +0300 Subject: [PATCH 06/57] Fix build --- src/coreclr/vm/appdomain.cpp | 1 + src/coreclr/vm/jitinterface.cpp | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 019a7de5c6122..6afb4016374f1 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -24,6 +24,7 @@ #include "assemblynative.hpp" #include "shimload.h" #include "stringliteralmap.h" +#include "frozenobjectheap.h" #include "codeman.h" #include "comcallablewrapper.h" #include "eventtrace.h" diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index ba482bae1b946..587d8e02652d7 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11938,8 +11938,9 @@ InfoAccessType CEEJitInfo::constructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd { void** ptr = (void**)ConstructStringLiteral(scopeHnd, metaTok); // throws + FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); // If it's in the frozen segment we can pass the direct reference. - if (SystemDomain::GetSegmentWithFrozenObjects()->IsInHeap((Object*)*ptr)) + if ((foh != nullptr) && foh->IsInHeap((Object*)*ptr)) { *ppValue = *ptr; result = IAT_VALUE; From 2ef204aebd2d3b23575f841931916b434dbb15be Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 16:45:52 +0300 Subject: [PATCH 07/57] Add lock --- src/coreclr/inc/crsttypes.h | 3 +- src/coreclr/vm/appdomain.cpp | 5 ++- src/coreclr/vm/appdomain.hpp | 1 + src/coreclr/vm/frozenobjectheap.cpp | 28 +++++++++---- src/coreclr/vm/frozenobjectheap.h | 5 ++- src/coreclr/vm/gchelpers.cpp | 62 ++++++++++++++++++++--------- src/coreclr/vm/gchelpers.h | 3 +- src/coreclr/vm/stringliteralmap.cpp | 1 - 8 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/coreclr/inc/crsttypes.h b/src/coreclr/inc/crsttypes.h index a1bab2ecb906c..a80c5468966fd 100644 --- a/src/coreclr/inc/crsttypes.h +++ b/src/coreclr/inc/crsttypes.h @@ -134,7 +134,8 @@ enum CrstType CrstUnwindInfoTableLock = 116, CrstVSDIndirectionCellLock = 117, CrstWrapperTemplate = 118, - kNumberOfCrstTypes = 119 + CrstFrozenObjectHeap = 119, + kNumberOfCrstTypes = 120 }; #endif // __CRST_TYPES_INCLUDED diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 6afb4016374f1..890c7bb91df23 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -116,6 +116,7 @@ int BaseDomain::m_iNumberOfProcessors = 0; // System Domain Statics GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; +CrstExplicitInit SystemDomain::m_FrozenObjectsCrst; DECLSPEC_ALIGN(16) static BYTE g_pSystemDomainMemory[sizeof(SystemDomain)]; @@ -1335,8 +1336,10 @@ void SystemDomain::LazyInitFrozenObjectsHeap() GCX_PREEMP(); + m_FrozenObjectsCrst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); + NewHolder pFoh(new FrozenObjectHeap()); - if (pFoh->Init() && InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + if (pFoh->Init(m_FrozenObjectsCrst) && InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) { pFoh.SuppressRelease(); } diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index f187d5ffd7a6c..9e8d92872a9d5 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2795,6 +2795,7 @@ class SystemDomain : public BaseDomain static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; static FrozenObjectHeap *m_FrozenObjects; + static CrstExplicitInit m_FrozenObjectsCrst; static ULONG s_dNumAppDomains; // Maintain a count of children app domains. diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index dc8f73b51da39..7a22a52f36b95 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -3,21 +3,29 @@ #include "frozenobjectheap.h" #include "memorypool.h" +#include "memorypool.h" -bool FrozenObjectHeap::Init(size_t fohSize) +bool FrozenObjectHeap::Init(CrstExplicitInit crst, size_t fohSize) { - _ASSERT(fohSize > (FOX_MAX_OBJECT_SIZE * 2)); + m_PageSize = GetOsPageSize(); + _ASSERT(fohSize >= m_PageSize); + + // TODO: Implement COMMIT on demand. + void* alloc = ClrVirtualAllocAligned(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); + ZeroMemory(alloc, fohSize); // will remove it. - // TODO: remove commit - void* alloc = ClrVirtualAlloc(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); if (alloc != nullptr) { m_SegmentHandle = GCInterface::RegisterFrozenSegment(alloc, fohSize); if (m_SegmentHandle != nullptr) { + m_Crst = crst; m_pStart = static_cast(alloc); m_pCurrent = m_pStart; + m_pCommited = m_pStart; m_Size = fohSize; + + ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; } } @@ -26,20 +34,26 @@ bool FrozenObjectHeap::Init(size_t fohSize) Object* FrozenObjectHeap::AllocateObject(size_t objectSize) { - if (objectSize > FOX_MAX_OBJECT_SIZE) + CrstHolder ch(&m_Crst); + + if (objectSize > m_PageSize) { - // object is to big + // Since FrozenObjectHeap is just an optimization, let's not fill it with large objects. return nullptr; } + _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); + _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); + uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); if ((obj + objectSize + OBJHEADER_SIZE) > (m_pStart + m_Size)) { // heap is full return nullptr; } + m_pCurrent = obj + OBJECT_BASESIZE + objectSize; - ZeroMemory(obj, objectSize); + ZeroMemory(obj, objectSize); // is it needed? return reinterpret_cast(obj + OBJECT_BASESIZE); } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 0040737da2d40..15dbea4d6be3c 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -12,7 +12,7 @@ class FrozenObjectHeap { public: - bool Init(size_t fohSize = FOH_DEFAULT_SIZE); + bool Init(CrstExplicitInit crst, size_t fohSize = FOH_DEFAULT_SIZE); Object* AllocateObject(size_t objectSize); @@ -23,8 +23,11 @@ class FrozenObjectHeap private: uint8_t* m_pStart; uint8_t* m_pCurrent; + uint8_t* m_pCommited; size_t m_Size; + size_t m_PageSize; void* m_SegmentHandle; + CrstExplicitInit m_Crst; }; #endif // _FROZENOBJECTHEAP_H diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index a2e1b4d6fda5d..50f4493fb127b 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -826,7 +826,7 @@ OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle elementType, BOOL bAll return AllocateSzArray(arrayType, (INT32) cElements, flags); } -STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap) +STRINGREF AllocateString(DWORD cchStringLength) { CONTRACTL { THROWS; @@ -853,35 +853,61 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap) _ASSERTE(totalSize > cchStringLength); GC_ALLOC_FLAGS flags = GC_ALLOC_NO_FLAGS; + + SetTypeHandleOnThreadForAlloc(TypeHandle(g_pStringClass)); + + if (totalSize >= g_pConfig->GetGCLOHThreshold()) + flags |= GC_ALLOC_LARGE_OBJECT_HEAP; + + StringObject* orString = (StringObject*)Alloc(totalSize, flags); + + // Initialize Object + orString->SetMethodTable(g_pStringClass); + orString->SetStringLength(cchStringLength); + + PublishObjectAndNotify(orString, flags); + return ObjectToSTRINGREF(orString); + +} + +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) +{ + CONTRACTL{ + THROWS; + GC_TRIGGERS; + MODE_COOPERATIVE; + } CONTRACTL_END; + + STRINGREF orStringRef; StringObject* orString = nullptr; - // Try to allocate the string in Frozen Object Heap. - if (preferFrozenObjHeap) + // Limit the maximum string size to <2GB to mitigate risk of security issues caused by 32-bit integer + // overflows in buffer size calculations. + // + // If the value below is changed, also change AllocateUtf8String. + if (cchStringLength > 0x3FFFFFDF) + ThrowOutOfMemory(); + + SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); + _ASSERTE(totalSize > cchStringLength); + + if (preferFrozenHeap) { FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); if (foh != nullptr) { orString = (StringObject*)foh->AllocateObject(totalSize); - // if FOH is null, full, or object is too big - fallback to normal allocation. + orString->SetMethodTable(g_pStringClass); + orString->SetStringLength(cchStringLength); + PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); + orStringRef = ObjectToSTRINGREF(orString); } } - if (orString == nullptr) { - SetTypeHandleOnThreadForAlloc(TypeHandle(g_pStringClass)); - - if (totalSize >= g_pConfig->GetGCLOHThreshold()) - flags |= GC_ALLOC_LARGE_OBJECT_HEAP; - - orString = (StringObject*)(Alloc(totalSize, flags)); + orStringRef = AllocateString(cchStringLength); } - - // Initialize Object - orString->SetMethodTable(g_pStringClass); - orString->SetStringLength(cchStringLength); - - PublishObjectAndNotify(orString, flags); - return ObjectToSTRINGREF(orString); + return orStringRef; } #ifdef FEATURE_COMINTEROP_UNMANAGED_ACTIVATION diff --git a/src/coreclr/vm/gchelpers.h b/src/coreclr/vm/gchelpers.h index 0233c97a6ac38..75d3788b79289 100644 --- a/src/coreclr/vm/gchelpers.h +++ b/src/coreclr/vm/gchelpers.h @@ -34,7 +34,8 @@ OBJECTREF AllocatePrimitiveArray(CorElementType type, DWORD cElements); OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle ElementType, BOOL bAllocateInPinnedHeap = FALSE); // Allocate a string -STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenObjHeap = false); +STRINGREF AllocateString(DWORD cchStringLength); +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap); OBJECTREF DupArrayForCloning(BASEARRAYREF pRef); diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 0df0936829f04..718459c3b575b 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -443,7 +443,6 @@ static void LogStringLiteral(__in_z const char* action, EEStringData *pStringDat } #endif - STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap) { CONTRACTL From a84a6eeaee43f43d114e07e606623b91613f50aa Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 17:07:46 +0300 Subject: [PATCH 08/57] Clean up --- src/coreclr/vm/frozenobjectheap.cpp | 5 ++++- src/coreclr/vm/frozenobjectheap.h | 2 ++ src/coreclr/vm/gchelpers.cpp | 15 ++++++++++----- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 7a22a52f36b95..43dd94b8e9f0d 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -12,7 +12,7 @@ bool FrozenObjectHeap::Init(CrstExplicitInit crst, size_t fohSize) // TODO: Implement COMMIT on demand. void* alloc = ClrVirtualAllocAligned(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); - ZeroMemory(alloc, fohSize); // will remove it. + ZeroMemory(alloc, fohSize); // will remove it, was just testing. if (alloc != nullptr) { @@ -25,6 +25,8 @@ bool FrozenObjectHeap::Init(CrstExplicitInit crst, size_t fohSize) m_pCommited = m_pStart; m_Size = fohSize; + INDEBUG(m_ObjectsCount = 0); + ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; } @@ -52,6 +54,7 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) return nullptr; } + INDEBUG(m_ObjectsCount++); m_pCurrent = obj + OBJECT_BASESIZE + objectSize; ZeroMemory(obj, objectSize); // is it needed? return reinterpret_cast(obj + OBJECT_BASESIZE); diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 15dbea4d6be3c..4d67208651d9f 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -28,6 +28,8 @@ class FrozenObjectHeap size_t m_PageSize; void* m_SegmentHandle; CrstExplicitInit m_Crst; + + INDEBUG(size_t m_ObjectsCount); }; #endif // _FROZENOBJECTHEAP_H diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 50f4493fb127b..f656b020292c2 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -888,7 +888,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) if (cchStringLength > 0x3FFFFFDF) ThrowOutOfMemory(); - SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); + const SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); _ASSERTE(totalSize > cchStringLength); if (preferFrozenHeap) @@ -897,10 +897,15 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) if (foh != nullptr) { orString = (StringObject*)foh->AllocateObject(totalSize); - orString->SetMethodTable(g_pStringClass); - orString->SetStringLength(cchStringLength); - PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); - orStringRef = ObjectToSTRINGREF(orString); + if (orString != nullptr) + { + orString->SetMethodTable(g_pStringClass); + orString->SetStringLength(cchStringLength); + + // Do we need to perform "PublishObject" here? + PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); + orStringRef = ObjectToSTRINGREF(orString); + } } } if (orString == nullptr) From 142295a3f645901f466ac7ff0216922febddf11a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 18:02:39 +0300 Subject: [PATCH 09/57] init memory on first allocation --- src/coreclr/vm/appdomain.cpp | 7 +-- src/coreclr/vm/appdomain.hpp | 1 - src/coreclr/vm/frozenobjectheap.cpp | 66 ++++++++++++++++++++++------- src/coreclr/vm/frozenobjectheap.h | 12 +++--- src/coreclr/vm/gchelpers.cpp | 1 - 5 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 890c7bb91df23..1d8ab6c7d4c41 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -116,7 +116,6 @@ int BaseDomain::m_iNumberOfProcessors = 0; // System Domain Statics GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; -CrstExplicitInit SystemDomain::m_FrozenObjectsCrst; DECLSPEC_ALIGN(16) static BYTE g_pSystemDomainMemory[sizeof(SystemDomain)]; @@ -1334,12 +1333,8 @@ void SystemDomain::LazyInitFrozenObjectsHeap() } CONTRACTL_END; - GCX_PREEMP(); - - m_FrozenObjectsCrst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); - NewHolder pFoh(new FrozenObjectHeap()); - if (pFoh->Init(m_FrozenObjectsCrst) && InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) { pFoh.SuppressRelease(); } diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 9e8d92872a9d5..f187d5ffd7a6c 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2795,7 +2795,6 @@ class SystemDomain : public BaseDomain static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; static FrozenObjectHeap *m_FrozenObjects; - static CrstExplicitInit m_FrozenObjectsCrst; static ULONG s_dNumAppDomains; // Maintain a count of children app domains. diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 43dd94b8e9f0d..c707bc5ed6ba8 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -3,23 +3,54 @@ #include "frozenobjectheap.h" #include "memorypool.h" -#include "memorypool.h" -bool FrozenObjectHeap::Init(CrstExplicitInit crst, size_t fohSize) +FrozenObjectHeap::FrozenObjectHeap(): + m_pStart(nullptr), + m_pCurrent(nullptr), + m_pCommited(nullptr), + m_Size(0), + m_PageSize(0), + m_SegmentHandle(nullptr), + m_ObjectsCount(0) +{ + m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); +} + +FrozenObjectHeap::~FrozenObjectHeap() +{ + if (m_SegmentHandle != nullptr) + { + GCHeapUtilities::GetGCHeap()->UnregisterFrozenSegment(m_SegmentHandle); + } + + if (m_pStart != nullptr) + { + ClrVirtualFree(m_pStart, 0, MEM_RELEASE); + } +} + +bool FrozenObjectHeap::Init(size_t fohSize) { m_PageSize = GetOsPageSize(); _ASSERT(fohSize >= m_PageSize); + _ASSERT(m_SegmentHandle == nullptr); + _ASSERT(m_pStart == nullptr); // TODO: Implement COMMIT on demand. void* alloc = ClrVirtualAllocAligned(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); - ZeroMemory(alloc, fohSize); // will remove it, was just testing. if (alloc != nullptr) { - m_SegmentHandle = GCInterface::RegisterFrozenSegment(alloc, fohSize); + segment_info seginfo{}; + seginfo.pvMem = m_pStart; + seginfo.ibFirstObject = sizeof(ObjHeader); + seginfo.ibAllocated = m_Size; + seginfo.ibCommit = seginfo.ibAllocated; + seginfo.ibReserved = seginfo.ibAllocated; + + m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&seginfo); if (m_SegmentHandle != nullptr) { - m_Crst = crst; m_pStart = static_cast(alloc); m_pCurrent = m_pStart; m_pCommited = m_pStart; @@ -30,6 +61,10 @@ bool FrozenObjectHeap::Init(CrstExplicitInit crst, size_t fohSize) ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; } + + // GC refused to register frozen segment + ClrVirtualFree(m_pStart, 0, MEM_RELEASE); + m_pStart = nullptr; } return false; } @@ -44,8 +79,16 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) return nullptr; } - _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); + if ((m_pStart == nullptr) && !Init()) + { + return nullptr; + } + + _ASSERT(m_pStart != nullptr); + _ASSERT(m_SegmentHandle != nullptr); + _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); + _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); if ((obj + objectSize + OBJHEADER_SIZE) > (m_pStart + m_Size)) @@ -62,7 +105,7 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) bool FrozenObjectHeap::IsInHeap(Object* object) { - auto ptr = reinterpret_cast(object); + const auto ptr = reinterpret_cast(object); if (ptr >= m_pStart && ptr < m_pCurrent) { _ASSERT(GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(object)); @@ -70,12 +113,3 @@ bool FrozenObjectHeap::IsInHeap(Object* object) } return false; } - -FrozenObjectHeap::~FrozenObjectHeap() -{ - if (m_SegmentHandle != nullptr) - { - GCInterface::UnregisterFrozenSegment(m_SegmentHandle); - ClrVirtualFree(m_pStart, 0, MEM_RELEASE); - } -} diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 4d67208651d9f..f54d8980d7af2 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -12,23 +12,21 @@ class FrozenObjectHeap { public: - bool Init(CrstExplicitInit crst, size_t fohSize = FOH_DEFAULT_SIZE); - + FrozenObjectHeap(); + ~FrozenObjectHeap(); Object* AllocateObject(size_t objectSize); - bool IsInHeap(Object* object); - ~FrozenObjectHeap(); - private: + bool Init(size_t fohSize = FOH_DEFAULT_SIZE); + uint8_t* m_pStart; uint8_t* m_pCurrent; uint8_t* m_pCommited; size_t m_Size; size_t m_PageSize; - void* m_SegmentHandle; + segment_handle m_SegmentHandle; CrstExplicitInit m_Crst; - INDEBUG(size_t m_ObjectsCount); }; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index f656b020292c2..bc2ed75bbca03 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -902,7 +902,6 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); - // Do we need to perform "PublishObject" here? PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); orStringRef = ObjectToSTRINGREF(orString); } From a446dd6a3f4b13c73d67d87d1c8a6ca5ecff294a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 18:20:40 +0300 Subject: [PATCH 10/57] Clean up --- src/coreclr/jit/emitxarch.cpp | 7 ----- src/coreclr/vm/frozenobjectheap.cpp | 45 +++++++++++++++-------------- src/coreclr/vm/frozenobjectheap.h | 7 ++--- src/coreclr/vm/gchelpers.cpp | 2 +- 4 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index a0a5e3283d4e9..fc32db2b5a385 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -12190,13 +12190,6 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id) assert(id->idGCref() == GCT_BYREF); #ifdef DEBUG - regMaskTP regMask; - regMask = genRegMask(reg1) | genRegMask(reg2); - - // r1/r2 could have been a GCREF as GCREF + int=BYREF - // or BYREF+/-int=BYREF - assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) || - ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))); #endif // Mark r1 as holding a byref emitGCregLiveUpd(GCT_BYREF, reg1, dst); diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index c707bc5ed6ba8..94f8ebecfde1d 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -9,10 +9,10 @@ FrozenObjectHeap::FrozenObjectHeap(): m_pCurrent(nullptr), m_pCommited(nullptr), m_Size(0), - m_PageSize(0), m_SegmentHandle(nullptr), m_ObjectsCount(0) { + m_PageSize = GetOsPageSize(); m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); } @@ -29,35 +29,33 @@ FrozenObjectHeap::~FrozenObjectHeap() } } -bool FrozenObjectHeap::Init(size_t fohSize) +bool FrozenObjectHeap::Initialize() { - m_PageSize = GetOsPageSize(); - _ASSERT(fohSize >= m_PageSize); + m_Size = m_PageSize * 1024; + + _ASSERT(m_PageSize > MIN_OBJECT_SIZE); _ASSERT(m_SegmentHandle == nullptr); _ASSERT(m_pStart == nullptr); // TODO: Implement COMMIT on demand. - void* alloc = ClrVirtualAllocAligned(nullptr, fohSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); - + void* alloc = ClrVirtualAllocAligned(nullptr, m_Size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); + ZeroMemory(alloc, m_Size); if (alloc != nullptr) { - segment_info seginfo{}; - seginfo.pvMem = m_pStart; - seginfo.ibFirstObject = sizeof(ObjHeader); - seginfo.ibAllocated = m_Size; - seginfo.ibCommit = seginfo.ibAllocated; - seginfo.ibReserved = seginfo.ibAllocated; - - m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&seginfo); + segment_info si{}; + si.pvMem = m_pStart; + si.ibFirstObject = sizeof(ObjHeader); + si.ibAllocated = m_Size; + si.ibCommit = m_Size; + si.ibReserved = m_Size; + + m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle != nullptr) { m_pStart = static_cast(alloc); m_pCurrent = m_pStart; m_pCommited = m_pStart; - m_Size = fohSize; - INDEBUG(m_ObjectsCount = 0); - ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; } @@ -79,9 +77,14 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) return nullptr; } - if ((m_pStart == nullptr) && !Init()) + if (m_pStart == nullptr) { - return nullptr; + // m_Size > 0 means we already tried to init and it failed. + // so bail out to avoid doing Alloc again. + if ((m_Size > 0) || !Initialize()) + { + return nullptr; + } } _ASSERT(m_pStart != nullptr); @@ -98,9 +101,9 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) } INDEBUG(m_ObjectsCount++); - m_pCurrent = obj + OBJECT_BASESIZE + objectSize; + m_pCurrent = obj + sizeof(ObjHeader) + objectSize; ZeroMemory(obj, objectSize); // is it needed? - return reinterpret_cast(obj + OBJECT_BASESIZE); + return reinterpret_cast(obj + sizeof(ObjHeader)); } bool FrozenObjectHeap::IsInHeap(Object* object) diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index f54d8980d7af2..65e0dd548e77d 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -5,9 +5,7 @@ #define _FROZENOBJECTHEAP_H #include "common.h" - -#define FOX_MAX_OBJECT_SIZE 2 * 1024 -#define FOH_DEFAULT_SIZE 1 * 1024 * 1024 +#include "gcinterface.h" class FrozenObjectHeap { @@ -18,7 +16,7 @@ class FrozenObjectHeap bool IsInHeap(Object* object); private: - bool Init(size_t fohSize = FOH_DEFAULT_SIZE); + bool Initialize(); uint8_t* m_pStart; uint8_t* m_pCurrent; @@ -27,6 +25,7 @@ class FrozenObjectHeap size_t m_PageSize; segment_handle m_SegmentHandle; CrstExplicitInit m_Crst; + bool m_FailedToInit; INDEBUG(size_t m_ObjectsCount); }; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index bc2ed75bbca03..31ece307e922e 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -902,7 +902,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); - PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); + // PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); orStringRef = ObjectToSTRINGREF(orString); } } From 41665c32514b28f8a8cba6d22d35afb20d0aa804 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Jul 2021 18:45:38 +0300 Subject: [PATCH 11/57] revert changes in emitxarch.cpp --- src/coreclr/jit/emitxarch.cpp | 7 +++++++ src/coreclr/vm/frozenobjectheap.cpp | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index fc32db2b5a385..a0a5e3283d4e9 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -12190,6 +12190,13 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id) assert(id->idGCref() == GCT_BYREF); #ifdef DEBUG + regMaskTP regMask; + regMask = genRegMask(reg1) | genRegMask(reg2); + + // r1/r2 could have been a GCREF as GCREF + int=BYREF + // or BYREF+/-int=BYREF + assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) || + ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))); #endif // Mark r1 as holding a byref emitGCregLiveUpd(GCT_BYREF, reg1, dst); diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 94f8ebecfde1d..2a2e5746d8007 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -39,7 +39,8 @@ bool FrozenObjectHeap::Initialize() // TODO: Implement COMMIT on demand. void* alloc = ClrVirtualAllocAligned(nullptr, m_Size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); - ZeroMemory(alloc, m_Size); + ZeroMemory(alloc, m_Size); // Will remove, was just testing. + if (alloc != nullptr) { segment_info si{}; From 67f32a742e3aeb673a6729b21b4341ba7883cf7c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 01:25:28 +0200 Subject: [PATCH 12/57] Update branch, fix asserts --- src/coreclr/vm/appdomain.hpp | 15 +++++++++++++++ src/coreclr/vm/frozenobjectheap.cpp | 15 ++++++++------- src/coreclr/vm/gchelpers.cpp | 2 -- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 48481dd1c0b3f..8c72eca6b8694 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -40,6 +40,7 @@ class SystemDomain; class AppDomain; class GlobalStringLiteralMap; class StringLiteralMap; +class FrozenObjectHeap; class MngStdInterfacesInfo; class DomainAssembly; class LoadLevelLimiter; @@ -2393,6 +2394,7 @@ class SystemDomain : public BaseDomain void Init(); void Stop(); static void LazyInitGlobalStringLiteralMap(); + static void LazyInitFrozenObjectsHeap(); //**************************************************************************************** // @@ -2465,6 +2467,18 @@ class SystemDomain : public BaseDomain _ASSERTE(m_pGlobalStringLiteralMap); return m_pGlobalStringLiteralMap; } + static FrozenObjectHeap* GetSegmentWithFrozenObjects() + { + WRAPPER_NO_CONTRACT; + +#ifdef FEATURE_BASICFREEZE + if (m_FrozenObjects == NULL) + { + SystemDomain::LazyInitFrozenObjectsHeap(); + } +#endif + return m_FrozenObjects; + } #endif // DACCESS_COMPILE #if defined(FEATURE_COMINTEROP_APARTMENT_SUPPORT) @@ -2634,6 +2648,7 @@ class SystemDomain : public BaseDomain static CrstStatic m_SystemDomainCrst; static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; + static FrozenObjectHeap *m_FrozenObjects; #endif // DACCESS_COMPILE public: diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 2a2e5746d8007..03f48a1955869 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -31,7 +31,7 @@ FrozenObjectHeap::~FrozenObjectHeap() bool FrozenObjectHeap::Initialize() { - m_Size = m_PageSize * 1024; + m_Size = m_PageSize * 1024; // e.g. 4Mb _ASSERT(m_PageSize > MIN_OBJECT_SIZE); _ASSERT(m_SegmentHandle == nullptr); @@ -43,8 +43,8 @@ bool FrozenObjectHeap::Initialize() if (alloc != nullptr) { - segment_info si{}; - si.pvMem = m_pStart; + segment_info si; + si.pvMem = alloc; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = m_Size; si.ibCommit = m_Size; @@ -61,7 +61,7 @@ bool FrozenObjectHeap::Initialize() return true; } - // GC refused to register frozen segment + // GC refused to register frozen segment (OOM?) ClrVirtualFree(m_pStart, 0, MEM_RELEASE); m_pStart = nullptr; } @@ -95,15 +95,16 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); - if ((obj + objectSize + OBJHEADER_SIZE) > (m_pStart + m_Size)) + if (objectSize > (size_t)(m_pStart + m_Size)) { // heap is full return nullptr; } INDEBUG(m_ObjectsCount++); - m_pCurrent = obj + sizeof(ObjHeader) + objectSize; - ZeroMemory(obj, objectSize); // is it needed? + m_pCurrent = obj + objectSize; + + // Skip object header (NOTE: objectSize already includes it) return reinterpret_cast(obj + sizeof(ObjHeader)); } diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index e866450c406ef..bd36136127f97 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -896,8 +896,6 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) { orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); - - // PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); orStringRef = ObjectToSTRINGREF(orString); } } From 9ecf7d731e4c25d89b8254a7b3685ca35433060a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 01:42:29 +0200 Subject: [PATCH 13/57] fix release build --- 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 03f48a1955869..613193659d1d9 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -9,8 +9,8 @@ FrozenObjectHeap::FrozenObjectHeap(): m_pCurrent(nullptr), m_pCommited(nullptr), m_Size(0), - m_SegmentHandle(nullptr), - m_ObjectsCount(0) + m_SegmentHandle(nullptr) + COMMA_INDEBUG(m_ObjectsCount(0)) { m_PageSize = GetOsPageSize(); m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); From 45a454a50a3421ecb4b221df266e2fb4080b6b55 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 02:24:43 +0200 Subject: [PATCH 14/57] Ignore collectible assemblies, fix build --- src/coreclr/vm/gchelpers.cpp | 2 +- src/coreclr/vm/stringliteralmap.cpp | 6 +++--- src/coreclr/vm/stringliteralmap.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index bd36136127f97..e7e2b226a2d48 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -873,7 +873,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) MODE_COOPERATIVE; } CONTRACTL_END; - STRINGREF orStringRef; + STRINGREF orStringRef = NULL; StringObject* orString = nullptr; // Limit the maximum string size to <2GB to mitigate risk of security issues caused by 32-bit integer diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index e31d577ee48a0..df770503293d5 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -165,7 +165,7 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA // someone beat us to inserting it. (m_StringToEntryHashTable->GetValue(pStringData, &Data)) // (Rather than waiting until after we look the string up in the global map) - StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetStringLiteral(pStringData, dwHash, bAddIfNotFound)); + StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetStringLiteral(pStringData, dwHash, bAddIfNotFound, bAppDomainWontUnload)); _ASSERTE(pEntry || !bAddIfNotFound); @@ -357,7 +357,7 @@ void GlobalStringLiteralMap::Init() ThrowOutOfMemory(); } -StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound) +StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload) { CONTRACTL { @@ -383,7 +383,7 @@ StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStri else { if (bAddIfNotFound) - pEntry = AddStringLiteral(pStringData, true); + pEntry = AddStringLiteral(pStringData, bAppDomainWontUnload); } return pEntry; diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index 69a2040663a36..a37aa1864cb7c 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -71,7 +71,7 @@ class GlobalStringLiteralMap void Init(); // Method to retrieve a string from the map. Takes a precomputed hash (for perf). - StringLiteralEntry *GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound); + StringLiteralEntry *GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload); // Method to explicitly intern a string object. Takes a precomputed hash (for perf). StringLiteralEntry *GetInternedString(STRINGREF *pString, DWORD dwHash, BOOL bAddIfNotFound); From 1d0a7106db948dd976ffd5858c89519c6eefce13 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 12:27:31 +0200 Subject: [PATCH 15/57] Fix failing tests, address feedback --- src/coreclr/jit/gentree.cpp | 6 ++++++ src/coreclr/vm/frozenobjectheap.cpp | 4 ++-- src/coreclr/vm/jithelpers.cpp | 17 +++++++++++++++-- src/coreclr/vm/jitinterface.cpp | 11 +++++------ src/coreclr/vm/jitinterface.h | 5 ++++- 5 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 65809969a120b..1dc50dcd8acb5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3695,6 +3695,12 @@ void Compiler::gtWalkOp(GenTree** op1WB, GenTree** op2WB, GenTree* base, bool co break; } + if (addOp1->IsCnsIntOrI() && addOp2->IsCnsIntOrI()) + { + // will be folded + break; + } + // mark it with GTF_ADDRMODE_NO_CSE add->gtFlags |= GTF_ADDRMODE_NO_CSE; diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 613193659d1d9..86d3fd2bf3102 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -94,8 +94,8 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); - uint8_t* obj = ALIGN_UP(m_pCurrent, DATA_ALIGNMENT); - if (objectSize > (size_t)(m_pStart + m_Size)) + uint8_t* obj = m_pCurrent; + if ((size_t)(m_pStart + m_Size) < (size_t)(obj + objectSize)) { // heap is full return nullptr; diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 9a7be8a9e7086..f902615d43833 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -23,6 +23,7 @@ #include "corprof.h" #include "eeprofinterfaces.h" #include "dynamicinterfacecastable.h" +#include "frozenobjectheap.h" #ifndef TARGET_UNIX // Included for referencing __report_gsfailure @@ -2420,7 +2421,7 @@ HCIMPL1(StringObject*, FramedAllocateString, DWORD stringLength) HCIMPLEND /*********************************************************************/ -OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken metaTok) +OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken metaTok, void** ppPinnedString) { CONTRACTL { THROWS; @@ -2431,7 +2432,19 @@ OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken meta _ASSERTE(TypeFromToken(metaTok) == mdtString); Module* module = GetModule(scopeHnd); - return module->ResolveStringRef(metaTok); + + // ResolveStringRef returns a small pinned object that points to StringObject + OBJECTHANDLE strObjHandle = module->ResolveStringRef(metaTok); + + // Let's see if that StringObject is pinned too (stored in Frozen Object Heap) + FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); + Object* strObj = *reinterpret_cast(strObjHandle); + if (ppPinnedString != nullptr && foh != nullptr && foh->IsInHeap(strObj)) + { + *ppPinnedString = strObj; + } + + return strObjHandle; } /*********************************************************************/ diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 809282162ee9b..39043e5fa02a4 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -64,7 +64,6 @@ #endif #include "tailcallhelp.h" -#include "frozenobjectheap.h" // The Stack Overflow probe takes place in the COOPERATIVE_TRANSITION_BEGIN() macro // @@ -11637,13 +11636,13 @@ InfoAccessType CEEJitInfo::constructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd } else { - void** ptr = (void**)ConstructStringLiteral(scopeHnd, metaTok); // throws + // If ConstructStringLiteral returns a pinned reference we can return it by value (IAT_VALUE) + void* ppPinnedString = nullptr; + void** ptr = (void**)ConstructStringLiteral(scopeHnd, metaTok, &ppPinnedString); - FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); - // If it's in the frozen segment we can pass the direct reference. - if ((foh != nullptr) && foh->IsInHeap((Object*)*ptr)) + if (ppPinnedString != nullptr) { - *ppValue = *ptr; + *ppValue = ppPinnedString; result = IAT_VALUE; } else diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index d0a415a1f5f1d..5dcdf4d786018 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -1088,7 +1088,10 @@ void DoGcStress (PT_CONTEXT regs, NativeCodeVersion nativeCodeVersion); EXTERN_C FCDECL2(LPVOID, ArrayStoreCheck, Object** pElement, PtrArray** pArray); -OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken metaTok); +// ppPinnedString: If the string is pinned (e.g. allocated in frozen heap), +// the pointer to the pinned string is returned in *ppPinnedPointer. ppPinnedPointer == nullptr +// means that the caller does not care whether the string is pinned or not. +OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken metaTok, void** ppPinnedString = nullptr); FCDECL2(Object*, JIT_Box, CORINFO_CLASS_HANDLE type, void* data); FCDECL0(VOID, JIT_PollGC); From 201565e32035deffb70fb3c5a22956cfa636195a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 13:09:01 +0200 Subject: [PATCH 16/57] Rename bAppDomainWontUnload to bIsCollectible --- src/coreclr/vm/dynamicmethod.cpp | 3 ++- src/coreclr/vm/loaderallocator.cpp | 6 +++--- src/coreclr/vm/object.cpp | 2 +- src/coreclr/vm/stringliteralmap.cpp | 31 ++++++++++++++++++----------- src/coreclr/vm/stringliteralmap.h | 10 +++++----- 5 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/coreclr/vm/dynamicmethod.cpp b/src/coreclr/vm/dynamicmethod.cpp index d837252b90495..a4227c4d00089 100644 --- a/src/coreclr/vm/dynamicmethod.cpp +++ b/src/coreclr/vm/dynamicmethod.cpp @@ -1275,7 +1275,8 @@ STRINGREF* LCGMethodResolver::GetOrInternString(STRINGREF *pProtectedStringRef) // lock the global string literal interning map CrstHolder gch(pStringLiteralMap->GetHashTableCrstGlobal()); - StringLiteralEntryHolder pEntry(pStringLiteralMap->GetInternedString(pProtectedStringRef, dwHash, /* bAddIfNotFound */ TRUE)); + StringLiteralEntryHolder pEntry(pStringLiteralMap->GetInternedString(pProtectedStringRef, dwHash, + /* bAddIfNotFound */ TRUE, /* bPreferFrozenObjectHeap */ FALSE)); DynamicStringLiteral* pStringLiteral = (DynamicStringLiteral*)m_jitTempData.New(sizeof(DynamicStringLiteral)); pStringLiteral->m_pEntry = pEntry.Extract(); diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index b7eebd07afecb..0e153dddaba91 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -1733,7 +1733,7 @@ STRINGREF *LoaderAllocator::GetStringObjRefPtrFromUnicodeString(EEStringData *pS LazyInitStringLiteralMap(); } _ASSERTE(m_pStringLiteralMap); - return m_pStringLiteralMap->GetStringLiteral(pStringData, TRUE, !CanUnload()); + return m_pStringLiteralMap->GetStringLiteral(pStringData, TRUE, CanUnload()); } //***************************************************************************** @@ -1791,7 +1791,7 @@ STRINGREF *LoaderAllocator::IsStringInterned(STRINGREF *pString) LazyInitStringLiteralMap(); } _ASSERTE(m_pStringLiteralMap); - return m_pStringLiteralMap->GetInternedString(pString, FALSE, !CanUnload()); + return m_pStringLiteralMap->GetInternedString(pString, FALSE, CanUnload()); } STRINGREF *LoaderAllocator::GetOrInternString(STRINGREF *pString) @@ -1810,7 +1810,7 @@ STRINGREF *LoaderAllocator::GetOrInternString(STRINGREF *pString) LazyInitStringLiteralMap(); } _ASSERTE(m_pStringLiteralMap); - return m_pStringLiteralMap->GetInternedString(pString, TRUE, !CanUnload()); + return m_pStringLiteralMap->GetInternedString(pString, TRUE, CanUnload()); } void AssemblyLoaderAllocator::RegisterHandleForCleanup(OBJECTHANDLE objHandle) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index a4e1ff2e084b2..a36b4fe9c86cf 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -527,7 +527,7 @@ VOID Object::ValidateInner(BOOL bDeep, BOOL bVerifyNextHeader, BOOL bVerifySyncB // noRangeChecks depends on initial values being FALSE BOOL bSmallObjectHeapPtr = FALSE, bLargeObjectHeapPtr = FALSE; - if (!noRangeChecks && !GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(this)) + if (!noRangeChecks) { bSmallObjectHeapPtr = GCHeapUtilities::GetGCHeap()->IsHeapPointer(this, true); if (!bSmallObjectHeapPtr) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index df770503293d5..ce0912795c016 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -143,7 +143,7 @@ StringLiteralMap::~StringLiteralMap() -STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload) +STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible) { CONTRACTL { @@ -165,7 +165,9 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA // someone beat us to inserting it. (m_StringToEntryHashTable->GetValue(pStringData, &Data)) // (Rather than waiting until after we look the string up in the global map) - StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetStringLiteral(pStringData, dwHash, bAddIfNotFound, bAppDomainWontUnload)); + // Don't use FOH for collectible modules to avoid potential memory leaks + const bool preferFrozenObjectHeap = !bIsCollectible; + StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetStringLiteral(pStringData, dwHash, bAddIfNotFound, preferFrozenObjectHeap)); _ASSERTE(pEntry || !bAddIfNotFound); @@ -177,7 +179,7 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA // TODO: except that by not inserting into our local table we always take the global map lock // and come into this path, when we could succeed at a lock free lookup above. - if (!bAppDomainWontUnload) + if (bIsCollectible) { // Make sure some other thread has not already added it. if (!m_StringToEntryHashTable->GetValue(pStringData, &Data)) @@ -209,7 +211,7 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA return NULL; } -STRINGREF *StringLiteralMap::GetInternedString(STRINGREF *pString, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload) +STRINGREF *StringLiteralMap::GetInternedString(STRINGREF *pString, BOOL bAddIfNotFound, BOOL bIsCollectible) { CONTRACTL { @@ -242,7 +244,10 @@ STRINGREF *StringLiteralMap::GetInternedString(STRINGREF *pString, BOOL bAddIfNo // (Rather than waiting until after we look the string up in the global map) // Retrieve the string literal from the global string literal map. - StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetInternedString(pString, dwHash, bAddIfNotFound)); + + // Don't use FOH for collectible modules to avoid potential memory leaks + const bool preferFrozenObjectHeap = !bIsCollectible; + StringLiteralEntryHolder pEntry(SystemDomain::GetGlobalStringLiteralMap()->GetInternedString(pString, dwHash, bAddIfNotFound, preferFrozenObjectHeap)); _ASSERTE(pEntry || !bAddIfNotFound); @@ -254,7 +259,7 @@ STRINGREF *StringLiteralMap::GetInternedString(STRINGREF *pString, BOOL bAddIfNo // TODO: except that by not inserting into our local table we always take the global map lock // and come into this path, when we could succeed at a lock free lookup above. - if (!bAppDomainWontUnload) + if (bIsCollectible) { // Since GlobalStringLiteralMap::GetInternedString() could have caused a GC, // we need to recreate the string data. @@ -357,7 +362,7 @@ void GlobalStringLiteralMap::Init() ThrowOutOfMemory(); } -StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload) +StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bPreferFrozenObjectHeap) { CONTRACTL { @@ -383,13 +388,15 @@ StringLiteralEntry *GlobalStringLiteralMap::GetStringLiteral(EEStringData *pStri else { if (bAddIfNotFound) - pEntry = AddStringLiteral(pStringData, bAppDomainWontUnload); + { + pEntry = AddStringLiteral(pStringData, bPreferFrozenObjectHeap); + } } return pEntry; } -StringLiteralEntry *GlobalStringLiteralMap::GetInternedString(STRINGREF *pString, DWORD dwHash, BOOL bAddIfNotFound) +StringLiteralEntry *GlobalStringLiteralMap::GetInternedString(STRINGREF *pString, DWORD dwHash, BOOL bAddIfNotFound, BOOL bPreferFrozenObjectHeap) { CONTRACTL { @@ -417,7 +424,7 @@ StringLiteralEntry *GlobalStringLiteralMap::GetInternedString(STRINGREF *pString else { if (bAddIfNotFound) - pEntry = AddInternedString(pString); + pEntry = AddInternedString(pString, bPreferFrozenObjectHeap); } return pEntry; @@ -507,7 +514,7 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri return pRet; } -StringLiteralEntry *GlobalStringLiteralMap::AddInternedString(STRINGREF *pString) +StringLiteralEntry *GlobalStringLiteralMap::AddInternedString(STRINGREF *pString, bool preferFrozenObjHeap) { CONTRACTL @@ -521,7 +528,7 @@ StringLiteralEntry *GlobalStringLiteralMap::AddInternedString(STRINGREF *pString CONTRACTL_END; EEStringData StringData = EEStringData((*pString)->GetStringLength(), (*pString)->GetBuffer()); - return AddStringLiteral(&StringData, true); + return AddStringLiteral(&StringData, preferFrozenObjHeap); } void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry) diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index a37aa1864cb7c..a7bfc7053faef 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -42,10 +42,10 @@ class StringLiteralMap } // Method to retrieve a string from the map. - STRINGREF *GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload); + STRINGREF *GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible); // Method to explicitly intern a string object. - STRINGREF *GetInternedString(STRINGREF *pString, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload); + STRINGREF *GetInternedString(STRINGREF *pString, BOOL bAddIfNotFound, BOOL bIsCollectible); private: // Hash tables that maps a Unicode string to a COM+ string handle. @@ -71,10 +71,10 @@ class GlobalStringLiteralMap void Init(); // Method to retrieve a string from the map. Takes a precomputed hash (for perf). - StringLiteralEntry *GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bAppDomainWontUnload); + StringLiteralEntry *GetStringLiteral(EEStringData *pStringData, DWORD dwHash, BOOL bAddIfNotFound, BOOL bPreferFrozenObjectHeap); // Method to explicitly intern a string object. Takes a precomputed hash (for perf). - StringLiteralEntry *GetInternedString(STRINGREF *pString, DWORD dwHash, BOOL bAddIfNotFound); + StringLiteralEntry *GetInternedString(STRINGREF *pString, DWORD dwHash, BOOL bAddIfNotFound, BOOL bPreferFrozenObjectHeap); // Method to calculate the hash DWORD GetHash(EEStringData* pData) @@ -95,7 +95,7 @@ class GlobalStringLiteralMap StringLiteralEntry *AddStringLiteral(EEStringData *pStringData, bool preferFrozenObjHeap); // Helper method to add an interned string. - StringLiteralEntry *AddInternedString(STRINGREF *pString); + StringLiteralEntry *AddInternedString(STRINGREF *pString, bool preferFrozenObjHeap); // Called by StringLiteralEntry when its RefCount falls to 0. void RemoveStringLiteralEntry(StringLiteralEntry *pEntry); From c7dad8590b2deeae916b893c4bbe1ac9c5d8ff2d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 13:39:00 +0200 Subject: [PATCH 17/57] Address feedback --- src/coreclr/vm/ceeload.cpp | 4 ++-- src/coreclr/vm/ceeload.h | 2 +- src/coreclr/vm/jithelpers.cpp | 13 +------------ src/coreclr/vm/loaderallocator.cpp | 4 ++-- src/coreclr/vm/loaderallocator.hpp | 2 +- src/coreclr/vm/stringliteralmap.cpp | 14 +++++++++++++- src/coreclr/vm/stringliteralmap.h | 2 +- 7 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 04e65cbfbdbe1..b3f8143300d41 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -2835,7 +2835,7 @@ void ModuleBase::InitializeStringData(DWORD token, EEStringData *pstrData, CQuic } -OBJECTHANDLE ModuleBase::ResolveStringRef(DWORD token) +OBJECTHANDLE ModuleBase::ResolveStringRef(DWORD token, void** ppPinnedString) { CONTRACTL { @@ -2864,7 +2864,7 @@ OBJECTHANDLE ModuleBase::ResolveStringRef(DWORD token) pLoaderAllocator = this->GetLoaderAllocator(); - string = (OBJECTHANDLE)pLoaderAllocator->GetStringObjRefPtrFromUnicodeString(&strData); + string = (OBJECTHANDLE)pLoaderAllocator->GetStringObjRefPtrFromUnicodeString(&strData, ppPinnedString); return string; } diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 16b3adac4b59f..d5e5395840c32 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -579,7 +579,7 @@ class ModuleBase } // Resolving - OBJECTHANDLE ResolveStringRef(DWORD Token); + OBJECTHANDLE ResolveStringRef(DWORD Token, void** ppPinnedString = nullptr); private: // string helper void InitializeStringData(DWORD token, EEStringData *pstrData, CQuickBytes *pqb); diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index f902615d43833..a3f11d9050fc0 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -23,7 +23,6 @@ #include "corprof.h" #include "eeprofinterfaces.h" #include "dynamicinterfacecastable.h" -#include "frozenobjectheap.h" #ifndef TARGET_UNIX // Included for referencing __report_gsfailure @@ -2434,17 +2433,7 @@ OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken meta Module* module = GetModule(scopeHnd); // ResolveStringRef returns a small pinned object that points to StringObject - OBJECTHANDLE strObjHandle = module->ResolveStringRef(metaTok); - - // Let's see if that StringObject is pinned too (stored in Frozen Object Heap) - FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); - Object* strObj = *reinterpret_cast(strObjHandle); - if (ppPinnedString != nullptr && foh != nullptr && foh->IsInHeap(strObj)) - { - *ppPinnedString = strObj; - } - - return strObjHandle; + return module->ResolveStringRef(metaTok, ppPinnedString); } /*********************************************************************/ diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index 0e153dddaba91..f723a3bca0d41 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -1717,7 +1717,7 @@ void AssemblyLoaderAllocator::RegisterBinder(CustomAssemblyBinder* binderToRelea m_binderToRelease = binderToRelease; } -STRINGREF *LoaderAllocator::GetStringObjRefPtrFromUnicodeString(EEStringData *pStringData) +STRINGREF *LoaderAllocator::GetStringObjRefPtrFromUnicodeString(EEStringData *pStringData, void** ppPinnedString) { CONTRACTL { @@ -1733,7 +1733,7 @@ STRINGREF *LoaderAllocator::GetStringObjRefPtrFromUnicodeString(EEStringData *pS LazyInitStringLiteralMap(); } _ASSERTE(m_pStringLiteralMap); - return m_pStringLiteralMap->GetStringLiteral(pStringData, TRUE, CanUnload()); + return m_pStringLiteralMap->GetStringLiteral(pStringData, TRUE, CanUnload(), ppPinnedString); } //***************************************************************************** diff --git a/src/coreclr/vm/loaderallocator.hpp b/src/coreclr/vm/loaderallocator.hpp index 90311d3a01ca1..a66f07f518723 100644 --- a/src/coreclr/vm/loaderallocator.hpp +++ b/src/coreclr/vm/loaderallocator.hpp @@ -590,7 +590,7 @@ class LoaderAllocator // Methods to retrieve a pointer to the COM+ string STRINGREF for a string constant. // If the string is not currently in the hash table it will be added and if the // copy string flag is set then the string will be copied before it is inserted. - STRINGREF *GetStringObjRefPtrFromUnicodeString(EEStringData *pStringData); + STRINGREF *GetStringObjRefPtrFromUnicodeString(EEStringData *pStringData, void** ppPinnedString = nullptr); void LazyInitStringLiteralMap(); STRINGREF *IsStringInterned(STRINGREF *pString); STRINGREF *GetOrInternString(STRINGREF *pString); diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index ce0912795c016..cd9c896b256f1 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -11,6 +11,7 @@ #include "common.h" #include "eeconfig.h" #include "stringliteralmap.h" +#include "frozenobjectheap.h" /* Thread safety in GlobalStringLiteralMap / StringLiteralMap @@ -143,7 +144,7 @@ StringLiteralMap::~StringLiteralMap() -STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible) +STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible, void** ppPinnedString) { CONTRACTL { @@ -203,6 +204,17 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA // Retrieve the string objectref from the string literal entry. pStrObj = pEntry->GetStringObject(); _ASSERTE(!bAddIfNotFound || pStrObj); + + FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); + if (pStrObj != nullptr && preferFrozenObjectHeap && ppPinnedString != nullptr && foh != nullptr) + { + Object* underlyingStrObj = *reinterpret_cast(pStrObj); + if (foh->IsInHeap(underlyingStrObj)) + { + *ppPinnedString = underlyingStrObj; + } + } + return pStrObj; } // If the bAddIfNotFound flag is set then we better have a string diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index a7bfc7053faef..e6d03d9d26adb 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -42,7 +42,7 @@ class StringLiteralMap } // Method to retrieve a string from the map. - STRINGREF *GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible); + STRINGREF *GetStringLiteral(EEStringData *pStringData, BOOL bAddIfNotFound, BOOL bIsCollectible, void** ppPinnedString = nullptr); // Method to explicitly intern a string object. STRINGREF *GetInternedString(STRINGREF *pString, BOOL bAddIfNotFound, BOOL bIsCollectible); From 4202d9ff1c4da102ec762e901ec312d9d867c96b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 14:28:59 +0200 Subject: [PATCH 18/57] Clean up --- src/coreclr/jit/assertionprop.cpp | 4 ---- src/coreclr/jit/gentree.cpp | 2 +- src/coreclr/vm/gchelpers.cpp | 5 ++--- src/coreclr/vm/jithelpers.cpp | 2 -- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index bf5056c478404..98523140e2c19 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3333,10 +3333,6 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, { return nullptr; } - if ((curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK) == GTF_ICON_STR_HDL) - { - return nullptr; - } // We assume that we do not try to do assertion prop on mismatched // accesses (note that we widen normalize-on-load local accesses diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1dc50dcd8acb5..5289da9c9c75e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3697,7 +3697,7 @@ void Compiler::gtWalkOp(GenTree** op1WB, GenTree** op2WB, GenTree* base, bool co if (addOp1->IsCnsIntOrI() && addOp2->IsCnsIntOrI()) { - // will be folded + // Might be folded break; } diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index e7e2b226a2d48..8a1390c1dc96e 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -821,7 +821,7 @@ OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle elementType, BOOL bAll return AllocateSzArray(arrayType, (INT32) cElements, flags); } -STRINGREF AllocateString(DWORD cchStringLength) +STRINGREF AllocateString( DWORD cchStringLength ) { CONTRACTL { THROWS; @@ -847,10 +847,9 @@ STRINGREF AllocateString(DWORD cchStringLength) SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); _ASSERTE(totalSize > cchStringLength); - GC_ALLOC_FLAGS flags = GC_ALLOC_NO_FLAGS; - SetTypeHandleOnThreadForAlloc(TypeHandle(g_pStringClass)); + GC_ALLOC_FLAGS flags = GC_ALLOC_NO_FLAGS; if (totalSize >= g_pConfig->GetGCLOHThreshold()) flags |= GC_ALLOC_LARGE_OBJECT_HEAP; diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index a3f11d9050fc0..e025138974c42 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -2431,8 +2431,6 @@ OBJECTHANDLE ConstructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken meta _ASSERTE(TypeFromToken(metaTok) == mdtString); Module* module = GetModule(scopeHnd); - - // ResolveStringRef returns a small pinned object that points to StringObject return module->ResolveStringRef(metaTok, ppPinnedString); } From a144453aef153e6673d78c92ba4c915e6b47d2f1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 19:46:14 +0200 Subject: [PATCH 19/57] Fix assert --- src/coreclr/jit/gentree.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 5289da9c9c75e..315e1bd771a8f 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3695,12 +3695,6 @@ void Compiler::gtWalkOp(GenTree** op1WB, GenTree** op2WB, GenTree* base, bool co break; } - if (addOp1->IsCnsIntOrI() && addOp2->IsCnsIntOrI()) - { - // Might be folded - break; - } - // mark it with GTF_ADDRMODE_NO_CSE add->gtFlags |= GTF_ADDRMODE_NO_CSE; @@ -3841,10 +3835,17 @@ bool Compiler::gtIsLikelyRegVar(GenTree* tree) // bool Compiler::gtCanSwapOrder(GenTree* firstNode, GenTree* secondNode) { - // Relative of order of global / side effects can't be swapped. - bool canSwap = true; + // Don't swap "CONST_HDL op CNS" + if (firstNode->IsIntegralConst() && secondNode->IsIntegralConst() && + varTypeIsGC(firstNode) && !varTypeIsGC(secondNode)) + { + canSwap = false; + } + + // Relative of order of global / side effects can't be swapped. + if (optValnumCSE_phase) { canSwap = optCSE_canSwap(firstNode, secondNode); From 2571c9c95b79ca43a59cb68118ce4225bde7aaa4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 29 Jul 2022 21:21:31 +0200 Subject: [PATCH 20/57] Fix more asserts not expecting TYP_REF constants being non zero --- src/coreclr/jit/assertionprop.cpp | 18 ++++++++++-------- src/coreclr/jit/gentree.cpp | 4 ++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 98523140e2c19..292c286ec7f9a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3069,11 +3069,9 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree) case TYP_REF: { - assert(vnStore->ConstantValue(vnCns) == 0); - // Support onle ref(ref(0)), do not support other forms (e.g byref(ref(0)). if (tree->TypeGet() == TYP_REF) { - conValTree = gtNewIconNode(0, TYP_REF); + conValTree = gtNewIconNode(vnStore->ConstantValue(vnCns), TYP_REF); } } break; @@ -4030,8 +4028,14 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen else if (op1->TypeGet() == TYP_REF) { // The only constant of TYP_REF that ValueNumbering supports is 'null' - assert(vnStore->ConstantValue(vnCns) == 0); - printf("null\n"); + if (vnStore->ConstantValue(vnCns) == 0) + { + printf("null\n"); + } + else + { + printf("%d (gcref)\n", static_cast(vnStore->ConstantValue(vnCns))); + } } else if (op1->TypeGet() == TYP_BYREF) { @@ -4084,9 +4088,7 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen } else if (op1->TypeGet() == TYP_REF) { - op1->BashToConst(0, TYP_REF); - // The only constant of TYP_REF that ValueNumbering supports is 'null' - noway_assert(vnStore->ConstantValue(vnCns) == 0); + op1->BashToConst(static_cast(vnStore->ConstantValue(vnCns)), TYP_REF); } else if (op1->TypeGet() == TYP_BYREF) { diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 315e1bd771a8f..24ea6ad3e3875 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3838,8 +3838,8 @@ bool Compiler::gtCanSwapOrder(GenTree* firstNode, GenTree* secondNode) bool canSwap = true; // Don't swap "CONST_HDL op CNS" - if (firstNode->IsIntegralConst() && secondNode->IsIntegralConst() && - varTypeIsGC(firstNode) && !varTypeIsGC(secondNode)) + if (firstNode->IsIntegralConst() && secondNode->IsIntegralConst() && varTypeIsGC(firstNode) && + !varTypeIsGC(secondNode)) { canSwap = false; } From 28e03312409eb93ce2ce7da261d1b8ad593ebaa7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 00:49:00 +0200 Subject: [PATCH 21/57] Test gc fix --- src/coreclr/gc/gc.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 5c5970d30ef2a..b2b7a796fbc3c 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -7753,6 +7753,14 @@ inline BOOL gc_heap::ephemeral_pointer_p (uint8_t* o) { #ifdef USE_REGIONS +#ifdef FEATURE_BASICFREEZE + if ((o >= g_gc_highest_address) || (o < g_gc_lowest_address)) + { + // objects in frozen segments are not ephemeral + return FALSE; + } +#endif + int gen_num = object_gennum ((uint8_t*)o); assert (gen_num >= 0); return (gen_num < max_generation); From 6625759d5b7288ddd9c426e712517b5a7122a557 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 11:30:50 +0200 Subject: [PATCH 22/57] move check to IsEphemeral --- src/coreclr/gc/gc.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index b2b7a796fbc3c..b9545186de1ef 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -7753,13 +7753,7 @@ inline BOOL gc_heap::ephemeral_pointer_p (uint8_t* o) { #ifdef USE_REGIONS -#ifdef FEATURE_BASICFREEZE - if ((o >= g_gc_highest_address) || (o < g_gc_lowest_address)) - { - // objects in frozen segments are not ephemeral - return FALSE; - } -#endif + int gen_num = object_gennum ((uint8_t*)o); assert (gen_num >= 0); @@ -44698,6 +44692,13 @@ unsigned int GCHeap::GetGenerationWithRange (Object* object, uint8_t** ppStart, bool GCHeap::IsEphemeral (Object* object) { uint8_t* o = (uint8_t*)object; +#if defined(FEATURE_BASICFREEZE) && defined(USE_REGIONS) + if (!is_in_heap_range(o)) + { + // objects in frozen segments are not ephemeral + return FALSE; + } +#endif gc_heap* hp = gc_heap::heap_of (o); return !!hp->ephemeral_pointer_p (o); } From 89df356ef01ede890be110b91bdd7e7520b2f688 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 13:52:24 +0200 Subject: [PATCH 23/57] Test commit (do I need to update "ibAllocated"?) --- src/coreclr/vm/frozenobjectheap.cpp | 5 +++-- src/coreclr/vm/gchelpers.cpp | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 86d3fd2bf3102..34462a9812d1f 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -13,7 +13,7 @@ FrozenObjectHeap::FrozenObjectHeap(): COMMA_INDEBUG(m_ObjectsCount(0)) { m_PageSize = GetOsPageSize(); - m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE); + m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); } FrozenObjectHeap::~FrozenObjectHeap() @@ -46,7 +46,7 @@ bool FrozenObjectHeap::Initialize() segment_info si; si.pvMem = alloc; si.ibFirstObject = sizeof(ObjHeader); - si.ibAllocated = m_Size; + si.ibAllocated = si.ibFirstObject; si.ibCommit = m_Size; si.ibReserved = m_Size; @@ -58,6 +58,7 @@ bool FrozenObjectHeap::Initialize() m_pCommited = m_pStart; INDEBUG(m_ObjectsCount = 0); ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); + //printf("\nFOH from %p to %p\n", m_pStart, m_pStart + m_Size); return true; } diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 897b5b0a668a4..3af466d5e14e8 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -895,6 +895,8 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) { orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); + _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); + //PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); orStringRef = ObjectToSTRINGREF(orString); } } From 3cb98e97196208a5099a94c1f5006f2e7246c614 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 15:34:43 +0200 Subject: [PATCH 24/57] Implement commit-on-demand --- src/coreclr/gc/gcee.cpp | 9 +++++ src/coreclr/gc/gcimpl.h | 1 + src/coreclr/gc/gcinterface.h | 3 ++ src/coreclr/vm/frozenobjectheap.cpp | 57 ++++++++++++++++++----------- src/coreclr/vm/frozenobjectheap.h | 3 +- src/coreclr/vm/stringliteralmap.cpp | 7 +--- 6 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/coreclr/gc/gcee.cpp b/src/coreclr/gc/gcee.cpp index 090ca285e572a..62be6ad7559c2 100644 --- a/src/coreclr/gc/gcee.cpp +++ b/src/coreclr/gc/gcee.cpp @@ -474,6 +474,15 @@ segment_handle GCHeap::RegisterFrozenSegment(segment_info *pseginfo) #endif // FEATURE_BASICFREEZE } +void GCHeap::UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited) +{ +#ifdef FEATURE_BASICFREEZE + heap_segment* heap_seg = reinterpret_cast(seg); + heap_segment_allocated (heap_seg) = allocated; + heap_segment_committed (heap_seg) = commited; +#endif // FEATURE_BASICFREEZE +} + void GCHeap::UnregisterFrozenSegment(segment_handle seg) { #ifdef FEATURE_BASICFREEZE diff --git a/src/coreclr/gc/gcimpl.h b/src/coreclr/gc/gcimpl.h index 67dc02943ffcd..1bde6af6b3bc7 100644 --- a/src/coreclr/gc/gcimpl.h +++ b/src/coreclr/gc/gcimpl.h @@ -245,6 +245,7 @@ class GCHeap : public IGCHeapInternal virtual segment_handle RegisterFrozenSegment(segment_info *pseginfo); virtual void UnregisterFrozenSegment(segment_handle seg); virtual bool IsInFrozenSegment(Object *object); + virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited); // Event control functions void ControlEvents(GCEventKeyword keyword, GCEventLevel level); diff --git a/src/coreclr/gc/gcinterface.h b/src/coreclr/gc/gcinterface.h index 8bdcf0c4aa22b..ef417ce6b118f 100644 --- a/src/coreclr/gc/gcinterface.h +++ b/src/coreclr/gc/gcinterface.h @@ -914,6 +914,9 @@ class IGCHeap { // Registers a frozen segment with the GC. virtual segment_handle RegisterFrozenSegment(segment_info *pseginfo) = 0; + // Updates given frozen segment + virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited) = 0; + // Unregisters a frozen segment. virtual void UnregisterFrozenSegment(segment_handle seg) = 0; diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 34462a9812d1f..963f5b0e63cd5 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -7,7 +7,7 @@ FrozenObjectHeap::FrozenObjectHeap(): m_pStart(nullptr), m_pCurrent(nullptr), - m_pCommited(nullptr), + m_SizeCommited(0), m_Size(0), m_SegmentHandle(nullptr) COMMA_INDEBUG(m_ObjectsCount(0)) @@ -16,6 +16,9 @@ FrozenObjectHeap::FrozenObjectHeap(): m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); } +#define FOH_RESERVE_PAGES 1024 // e.g. reserve 4Mb of virtual memory +#define FOH_COMMIT_PAGES 1 // e.g. commit 128Kb chunks + FrozenObjectHeap::~FrozenObjectHeap() { if (m_SegmentHandle != nullptr) @@ -31,15 +34,20 @@ FrozenObjectHeap::~FrozenObjectHeap() bool FrozenObjectHeap::Initialize() { - m_Size = m_PageSize * 1024; // e.g. 4Mb + m_Size = m_PageSize * FOH_RESERVE_PAGES; + _ASSERT(FOH_RESERVE_PAGES > FOH_COMMIT_PAGES); + _ASSERT(FOH_RESERVE_PAGES % FOH_COMMIT_PAGES == 0); _ASSERT(m_PageSize > MIN_OBJECT_SIZE); _ASSERT(m_SegmentHandle == nullptr); _ASSERT(m_pStart == nullptr); - // TODO: Implement COMMIT on demand. - void* alloc = ClrVirtualAllocAligned(nullptr, m_Size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE, m_PageSize); - ZeroMemory(alloc, m_Size); // Will remove, was just testing. + void* alloc = ClrVirtualAllocAligned(nullptr, m_Size, MEM_RESERVE, PAGE_READWRITE, m_PageSize); + if (alloc != nullptr) + { + // Commit FOH_COMMIT_PAGES pages in advance + alloc = ClrVirtualAllocAligned(alloc, m_PageSize * FOH_COMMIT_PAGES, MEM_COMMIT, PAGE_READWRITE, m_PageSize); + } if (alloc != nullptr) { @@ -47,7 +55,7 @@ bool FrozenObjectHeap::Initialize() si.pvMem = alloc; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = si.ibFirstObject; - si.ibCommit = m_Size; + si.ibCommit = m_PageSize * FOH_COMMIT_PAGES; si.ibReserved = m_Size; m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); @@ -55,10 +63,9 @@ bool FrozenObjectHeap::Initialize() { m_pStart = static_cast(alloc); m_pCurrent = m_pStart; - m_pCommited = m_pStart; + m_SizeCommited = si.ibCommit; INDEBUG(m_ObjectsCount = 0); ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); - //printf("\nFOH from %p to %p\n", m_pStart, m_pStart + m_Size); return true; } @@ -69,8 +76,12 @@ bool FrozenObjectHeap::Initialize() return false; } + Object* FrozenObjectHeap::AllocateObject(size_t objectSize) { + // NOTE: objectSize is expected be the full size including header + _ASSERT(objectSize >= MIN_OBJECT_SIZE); + CrstHolder ch(&m_Crst); if (objectSize > m_PageSize) @@ -91,9 +102,7 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) _ASSERT(m_pStart != nullptr); _ASSERT(m_SegmentHandle != nullptr); - _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); - _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT)); uint8_t* obj = m_pCurrent; if ((size_t)(m_pStart + m_Size) < (size_t)(obj + objectSize)) @@ -102,20 +111,24 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) return nullptr; } + // Check if we need to commit a new chunk + if ((size_t)(m_pStart + m_SizeCommited) < (size_t)(obj + objectSize)) + { + if (ClrVirtualAllocAligned(m_pCurrent, m_PageSize * FOH_COMMIT_PAGES, MEM_COMMIT, PAGE_READWRITE, m_PageSize) == nullptr) + { + // We failed to commit a new chunk of the reserved memory + return nullptr; + } + m_SizeCommited += m_PageSize * FOH_COMMIT_PAGES; + } + + ZeroMemory(obj, objectSize); INDEBUG(m_ObjectsCount++); m_pCurrent = obj + objectSize; - // Skip object header (NOTE: objectSize already includes it) - return reinterpret_cast(obj + sizeof(ObjHeader)); -} + // Notify GC that we bumped the pointer + GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommited); -bool FrozenObjectHeap::IsInHeap(Object* object) -{ - const auto ptr = reinterpret_cast(object); - if (ptr >= m_pStart && ptr < m_pCurrent) - { - _ASSERT(GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(object)); - return true; - } - return false; + // Skip object header + return reinterpret_cast(obj + sizeof(ObjHeader)); } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 65e0dd548e77d..bb91a25001e26 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -13,14 +13,13 @@ class FrozenObjectHeap FrozenObjectHeap(); ~FrozenObjectHeap(); Object* AllocateObject(size_t objectSize); - bool IsInHeap(Object* object); private: bool Initialize(); uint8_t* m_pStart; uint8_t* m_pCurrent; - uint8_t* m_pCommited; + size_t m_SizeCommited; size_t m_Size; size_t m_PageSize; segment_handle m_SegmentHandle; diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index cd9c896b256f1..57f0c4ef8f0cd 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -11,7 +11,6 @@ #include "common.h" #include "eeconfig.h" #include "stringliteralmap.h" -#include "frozenobjectheap.h" /* Thread safety in GlobalStringLiteralMap / StringLiteralMap @@ -205,16 +204,14 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA pStrObj = pEntry->GetStringObject(); _ASSERTE(!bAddIfNotFound || pStrObj); - FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); - if (pStrObj != nullptr && preferFrozenObjectHeap && ppPinnedString != nullptr && foh != nullptr) + if (pStrObj != nullptr && preferFrozenObjectHeap && ppPinnedString != nullptr) { Object* underlyingStrObj = *reinterpret_cast(pStrObj); - if (foh->IsInHeap(underlyingStrObj)) + if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(underlyingStrObj)) { *ppPinnedString = underlyingStrObj; } } - return pStrObj; } // If the bAddIfNotFound flag is set then we better have a string From c6df9edd270fb4d8b97bab02b8ceb858d331a4e6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 15:36:24 +0200 Subject: [PATCH 25/57] Clean up --- src/coreclr/gc/gc.cpp | 4 +--- src/coreclr/vm/gchelpers.cpp | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index b9545186de1ef..08469050bd7f3 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -7753,8 +7753,6 @@ inline BOOL gc_heap::ephemeral_pointer_p (uint8_t* o) { #ifdef USE_REGIONS - - int gen_num = object_gennum ((uint8_t*)o); assert (gen_num >= 0); return (gen_num < max_generation); @@ -44693,7 +44691,7 @@ bool GCHeap::IsEphemeral (Object* object) { uint8_t* o = (uint8_t*)object; #if defined(FEATURE_BASICFREEZE) && defined(USE_REGIONS) - if (!is_in_heap_range(o)) + if (!is_in_heap_range (o)) { // objects in frozen segments are not ephemeral return FALSE; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 3af466d5e14e8..6e8c30a27fe20 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -896,7 +896,6 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); - //PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS); orStringRef = ObjectToSTRINGREF(orString); } } From f50b4b9a07d65400efa7edbf37c78eeea8536346 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 16:08:15 +0200 Subject: [PATCH 26/57] bump FOH_COMMIT_PAGES --- 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 963f5b0e63cd5..dcec715e9ab35 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -17,7 +17,7 @@ FrozenObjectHeap::FrozenObjectHeap(): } #define FOH_RESERVE_PAGES 1024 // e.g. reserve 4Mb of virtual memory -#define FOH_COMMIT_PAGES 1 // e.g. commit 128Kb chunks +#define FOH_COMMIT_PAGES 32 // e.g. commit 128Kb chunks FrozenObjectHeap::~FrozenObjectHeap() { From 6ea2a502042f62bb3ae8066564678d0ae454d622 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 19:17:44 +0200 Subject: [PATCH 27/57] Address feedback, clean up --- src/coreclr/gc/gcee.cpp | 2 +- src/coreclr/inc/clrconfigvalues.h | 6 +++ src/coreclr/vm/frozenobjectheap.cpp | 69 ++++++++++++++++------------- src/coreclr/vm/frozenobjectheap.h | 7 ++- 4 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/coreclr/gc/gcee.cpp b/src/coreclr/gc/gcee.cpp index 62be6ad7559c2..a2dd58435d77e 100644 --- a/src/coreclr/gc/gcee.cpp +++ b/src/coreclr/gc/gcee.cpp @@ -478,8 +478,8 @@ void GCHeap::UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t { #ifdef FEATURE_BASICFREEZE heap_segment* heap_seg = reinterpret_cast(seg); - heap_segment_allocated (heap_seg) = allocated; heap_segment_committed (heap_seg) = commited; + heap_segment_allocated (heap_seg) = allocated; #endif // FEATURE_BASICFREEZE } diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 147d03f957cfc..8460f422bd7fe 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -403,6 +403,12 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_CodeHeapReserveForJumpStubs, W("CodeHeapReserv RETAIL_CONFIG_DWORD_INFO(INTERNAL_NGenReserveForJumpStubs, W("NGenReserveForJumpStubs"), 0, "Percentage of ngen image size to reserve for jump stubs") RETAIL_CONFIG_DWORD_INFO(INTERNAL_BreakOnOutOfMemoryWithinRange, W("BreakOnOutOfMemoryWithinRange"), 0, "Break before out of memory within range exception is thrown") +/// +/// Frozen segments (aka Frozen Object Heap) +/// +CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for frozen segments") // 4Mb +CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb + /// /// Log /// diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index dcec715e9ab35..7e03ce88db5b5 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -7,18 +7,15 @@ FrozenObjectHeap::FrozenObjectHeap(): m_pStart(nullptr), m_pCurrent(nullptr), - m_SizeCommited(0), - m_Size(0), + m_CommitChunkSize(0), + m_SizeCommitted(0), + m_SizeReserved(0), m_SegmentHandle(nullptr) COMMA_INDEBUG(m_ObjectsCount(0)) { - m_PageSize = GetOsPageSize(); m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); } -#define FOH_RESERVE_PAGES 1024 // e.g. reserve 4Mb of virtual memory -#define FOH_COMMIT_PAGES 32 // e.g. commit 128Kb chunks - FrozenObjectHeap::~FrozenObjectHeap() { if (m_SegmentHandle != nullptr) @@ -34,19 +31,26 @@ FrozenObjectHeap::~FrozenObjectHeap() bool FrozenObjectHeap::Initialize() { - m_Size = m_PageSize * FOH_RESERVE_PAGES; + // For internal testing + m_CommitChunkSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentCommitSize); + m_SizeReserved = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentReserveSize); + + if (m_SizeReserved == 0) + { + // A way to disable FrozenObjectHeap + return false; + } - _ASSERT(FOH_RESERVE_PAGES > FOH_COMMIT_PAGES); - _ASSERT(FOH_RESERVE_PAGES % FOH_COMMIT_PAGES == 0); - _ASSERT(m_PageSize > MIN_OBJECT_SIZE); + _ASSERT(m_SizeReserved > m_CommitChunkSize); + _ASSERT(m_SizeReserved % m_CommitChunkSize == 0); _ASSERT(m_SegmentHandle == nullptr); _ASSERT(m_pStart == nullptr); - void* alloc = ClrVirtualAllocAligned(nullptr, m_Size, MEM_RESERVE, PAGE_READWRITE, m_PageSize); + void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); if (alloc != nullptr) { - // Commit FOH_COMMIT_PAGES pages in advance - alloc = ClrVirtualAllocAligned(alloc, m_PageSize * FOH_COMMIT_PAGES, MEM_COMMIT, PAGE_READWRITE, m_PageSize); + // Commit FOH_COMMIT_SIZE chunk in advance + alloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); } if (alloc != nullptr) @@ -55,15 +59,15 @@ bool FrozenObjectHeap::Initialize() si.pvMem = alloc; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = si.ibFirstObject; - si.ibCommit = m_PageSize * FOH_COMMIT_PAGES; - si.ibReserved = m_Size; + si.ibCommit = m_CommitChunkSize; + si.ibReserved = m_SizeReserved; m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle != nullptr) { m_pStart = static_cast(alloc); m_pCurrent = m_pStart; - m_SizeCommited = si.ibCommit; + m_SizeCommitted = si.ibCommit; INDEBUG(m_ObjectsCount = 0); ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; @@ -84,50 +88,51 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) CrstHolder ch(&m_Crst); - if (objectSize > m_PageSize) - { - // Since FrozenObjectHeap is just an optimization, let's not fill it with large objects. - return nullptr; - } - if (m_pStart == nullptr) { - // m_Size > 0 means we already tried to init and it failed. + // m_SizeReserved > 0 means we already tried to init and it failed. // so bail out to avoid doing Alloc again. - if ((m_Size > 0) || !Initialize()) + if ((m_SizeReserved > 0) || !Initialize()) { return nullptr; } } + if (objectSize > m_CommitChunkSize) + { + // The current design doesn't allow object larger than FOH_COMMIT_CHUNK_SIZE and + // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects. + return nullptr; + } + _ASSERT(m_pStart != nullptr); _ASSERT(m_SegmentHandle != nullptr); _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); uint8_t* obj = m_pCurrent; - if ((size_t)(m_pStart + m_Size) < (size_t)(obj + objectSize)) + if ((size_t)(m_pStart + m_SizeReserved) < (size_t)(obj + objectSize)) { - // heap is full + // heap is full, caller is expected to switch to other heaps + // TODO: register a new frozen segment return nullptr; } // Check if we need to commit a new chunk - if ((size_t)(m_pStart + m_SizeCommited) < (size_t)(obj + objectSize)) + if ((size_t)(m_pStart + m_SizeCommitted) < (size_t)(obj + objectSize)) { - if (ClrVirtualAllocAligned(m_pCurrent, m_PageSize * FOH_COMMIT_PAGES, MEM_COMMIT, PAGE_READWRITE, m_PageSize) == nullptr) + if (ClrVirtualAlloc(m_pCurrent, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) { // We failed to commit a new chunk of the reserved memory return nullptr; } - m_SizeCommited += m_PageSize * FOH_COMMIT_PAGES; + m_SizeCommitted += m_CommitChunkSize; } - ZeroMemory(obj, objectSize); INDEBUG(m_ObjectsCount++); m_pCurrent = obj + objectSize; - // Notify GC that we bumped the pointer - GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommited); + // 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); // Skip object header return reinterpret_cast(obj + sizeof(ObjHeader)); diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index bb91a25001e26..6516e099243fe 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -19,12 +19,11 @@ class FrozenObjectHeap uint8_t* m_pStart; uint8_t* m_pCurrent; - size_t m_SizeCommited; - size_t m_Size; - size_t m_PageSize; + size_t m_CommitChunkSize; + size_t m_SizeCommitted; + size_t m_SizeReserved; segment_handle m_SegmentHandle; CrstExplicitInit m_Crst; - bool m_FailedToInit; INDEBUG(size_t m_ObjectsCount); }; From 15ca880e24d9b2a66e05ff234023e8ff040800cd Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 19:44:14 +0200 Subject: [PATCH 28/57] Fix build --- src/coreclr/inc/clrconfigvalues.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 8460f422bd7fe..791bf2c9e1c05 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -406,8 +406,8 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_BreakOnOutOfMemoryWithinRange, W("BreakOnOutOf /// /// Frozen segments (aka Frozen Object Heap) /// -CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for frozen segments") // 4Mb -CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for frozen segments") // 4Mb +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb /// /// Log From 9198043bf6efec44b23e599f67bd06df5ffa1fae Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 30 Jul 2022 20:59:28 +0200 Subject: [PATCH 29/57] Unix's ClrVirtualAlloc seems to be non-aligned --- 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 7e03ce88db5b5..4291d8ce07e8d 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -46,7 +46,7 @@ bool FrozenObjectHeap::Initialize() _ASSERT(m_SegmentHandle == nullptr); _ASSERT(m_pStart == nullptr); - void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); + void* alloc = ClrVirtualAllocAligned(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE, DATA_ALIGNMENT); if (alloc != nullptr) { // Commit FOH_COMMIT_SIZE chunk in advance From 7c98ef209ae18d0563ce947fbfac15d06f8ef442 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 31 Jul 2022 00:09:06 +0200 Subject: [PATCH 30/57] Oops, fix red CI --- src/coreclr/vm/frozenobjectheap.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 4291d8ce07e8d..bc7f42fb004e6 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -46,7 +46,7 @@ bool FrozenObjectHeap::Initialize() _ASSERT(m_SegmentHandle == nullptr); _ASSERT(m_pStart == nullptr); - void* alloc = ClrVirtualAllocAligned(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE, DATA_ALIGNMENT); + void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); if (alloc != nullptr) { // Commit FOH_COMMIT_SIZE chunk in advance @@ -55,6 +55,10 @@ bool FrozenObjectHeap::Initialize() if (alloc != nullptr) { + // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect + // DATA_ALIGNMENT alignment as well + _ASSERT(IS_ALIGNED(alloc, DATA_ALIGNMENT)); + segment_info si; si.pvMem = alloc; si.ibFirstObject = sizeof(ObjHeader); @@ -69,7 +73,6 @@ bool FrozenObjectHeap::Initialize() m_pCurrent = m_pStart; m_SizeCommitted = si.ibCommit; INDEBUG(m_ObjectsCount = 0); - ASSERT((intptr_t)m_pCurrent % DATA_ALIGNMENT == 0); return true; } @@ -120,7 +123,8 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) // Check if we need to commit a new chunk if ((size_t)(m_pStart + m_SizeCommitted) < (size_t)(obj + objectSize)) { - if (ClrVirtualAlloc(m_pCurrent, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) + _ASSERT(m_SizeCommitted + m_CommitChunkSize <= m_SizeReserved); + if (ClrVirtualAlloc(m_pStart + m_SizeCommitted, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) { // We failed to commit a new chunk of the reserved memory return nullptr; From af6e445dad16a97eb99a42f22bc3e92ed5aa192f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 31 Jul 2022 01:42:51 +0200 Subject: [PATCH 31/57] Multiple frozen segments --- src/coreclr/inc/clrconfigvalues.h | 11 +- src/coreclr/vm/appdomain.cpp | 8 +- src/coreclr/vm/appdomain.hpp | 10 +- src/coreclr/vm/frozenobjectheap.cpp | 174 +++++++++++++++------------- src/coreclr/vm/frozenobjectheap.h | 23 +++- src/coreclr/vm/gchelpers.cpp | 4 +- 6 files changed, 133 insertions(+), 97 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 791bf2c9e1c05..789fe062d8814 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -406,8 +406,15 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_BreakOnOutOfMemoryWithinRange, W("BreakOnOutOf /// /// Frozen segments (aka Frozen Object Heap) /// -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for frozen segments") // 4Mb -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb +#ifdef _DEBUG +#define FrozenSegmentReserveSize (0x4000) // 16Kb +#define FrozenSegmentCommitSize (0x1000) // 4Kb +#else +#define FrozenSegmentReserveSize (0x400000) // 4Mb +#define FrozenSegmentCommitSize (0x10000) // 64Kb +#endif +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), FrozenSegmentReserveSize, "Amount of memory to reserve for a frozen segment") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), FrozenSegmentCommitSize, "Amount of memory to commit on demand for frozen segments") /// /// Log diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index a54c59cf0d12b..a16d86c9a3a7b 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -101,8 +101,8 @@ CrstStatic BaseDomain::m_SpecialStaticsCrst; int BaseDomain::m_iNumberOfProcessors = 0; // System Domain Statics -GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; -FrozenObjectHeap* SystemDomain::m_FrozenObjects = NULL; +GlobalStringLiteralMap* SystemDomain::m_pGlobalStringLiteralMap = NULL; +FrozenObjectHeapManager* SystemDomain::m_FrozenObjectHeapManager = NULL; DECLSPEC_ALIGN(16) static BYTE g_pSystemDomainMemory[sizeof(SystemDomain)]; @@ -1209,8 +1209,8 @@ void SystemDomain::LazyInitFrozenObjectsHeap() } CONTRACTL_END; - NewHolder pFoh(new FrozenObjectHeap()); - if (InterlockedCompareExchangeT(&m_FrozenObjects, pFoh, nullptr) == nullptr) + NewHolder pFoh(new FrozenObjectHeapManager()); + if (InterlockedCompareExchangeT(&m_FrozenObjectHeapManager, pFoh, nullptr) == nullptr) { pFoh.SuppressRelease(); } diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 8c72eca6b8694..73b611dd29a48 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -40,7 +40,7 @@ class SystemDomain; class AppDomain; class GlobalStringLiteralMap; class StringLiteralMap; -class FrozenObjectHeap; +class FrozenObjectHeapManager; class MngStdInterfacesInfo; class DomainAssembly; class LoadLevelLimiter; @@ -2467,17 +2467,17 @@ class SystemDomain : public BaseDomain _ASSERTE(m_pGlobalStringLiteralMap); return m_pGlobalStringLiteralMap; } - static FrozenObjectHeap* GetSegmentWithFrozenObjects() + static FrozenObjectHeapManager* GetFrozenObjectHeapManager() { WRAPPER_NO_CONTRACT; #ifdef FEATURE_BASICFREEZE - if (m_FrozenObjects == NULL) + if (m_FrozenObjectHeapManager == NULL) { SystemDomain::LazyInitFrozenObjectsHeap(); } #endif - return m_FrozenObjects; + return m_FrozenObjectHeapManager; } #endif // DACCESS_COMPILE @@ -2648,7 +2648,7 @@ class SystemDomain : public BaseDomain static CrstStatic m_SystemDomainCrst; static GlobalStringLiteralMap *m_pGlobalStringLiteralMap; - static FrozenObjectHeap *m_FrozenObjects; + static FrozenObjectHeapManager *m_FrozenObjectHeapManager; #endif // DACCESS_COMPILE public: diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index bc7f42fb004e6..0577ebf09461b 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -4,135 +4,151 @@ #include "frozenobjectheap.h" #include "memorypool.h" -FrozenObjectHeap::FrozenObjectHeap(): - m_pStart(nullptr), - m_pCurrent(nullptr), - m_CommitChunkSize(0), - m_SizeCommitted(0), - m_SizeReserved(0), - m_SegmentHandle(nullptr) - COMMA_INDEBUG(m_ObjectsCount(0)) + +FrozenObjectHeapManager::FrozenObjectHeapManager(): + m_CurrentHeap(nullptr) { m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); + m_HeapCommitChunkSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentCommitSize); + m_HeapSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentReserveSize); } -FrozenObjectHeap::~FrozenObjectHeap() +// Allocates an object of the give size (including header) on a frozen segment. +// May return nullptr in the following cases: +// 1) DOTNET_FrozenSegmentReserveSize is 0 (disabled) +// 2) Object is too large (large than DOTNET_FrozenSegmentCommitSize) +// in such cases caller is responsible to find a more appropriate heap to allocate it +Object* FrozenObjectHeapManager::AllocateObject(size_t objectSize) { - if (m_SegmentHandle != nullptr) + CONTRACTL { - GCHeapUtilities::GetGCHeap()->UnregisterFrozenSegment(m_SegmentHandle); + THROWS; + MODE_COOPERATIVE; } + CONTRACTL_END + + CrstHolder ch(&m_Crst); - if (m_pStart != nullptr) + // Quick way to disable Frozen Heaps + if (m_HeapSize == 0) { - ClrVirtualFree(m_pStart, 0, MEM_RELEASE); + return nullptr; } -} -bool FrozenObjectHeap::Initialize() -{ - // For internal testing - m_CommitChunkSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentCommitSize); - m_SizeReserved = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentReserveSize); + _ASSERT(m_HeapCommitChunkSize >= MIN_OBJECT_SIZE); + _ASSERT(m_HeapSize > m_HeapCommitChunkSize); + _ASSERT(m_HeapSize % m_HeapCommitChunkSize == 0); - if (m_SizeReserved == 0) + // NOTE: objectSize is expected be the full size including header + _ASSERT(objectSize >= MIN_OBJECT_SIZE); + + if (objectSize > m_HeapCommitChunkSize) { - // A way to disable FrozenObjectHeap - return false; + // The current design doesn't allow object larger than DOTNET_FrozenSegmentCommitSize and + // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects. + return nullptr; } - _ASSERT(m_SizeReserved > m_CommitChunkSize); - _ASSERT(m_SizeReserved % m_CommitChunkSize == 0); - _ASSERT(m_SegmentHandle == nullptr); - _ASSERT(m_pStart == nullptr); - - void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); - if (alloc != nullptr) + if (m_CurrentHeap == nullptr) { - // Commit FOH_COMMIT_SIZE chunk in advance - alloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); + // Create the first heap on first allocation + m_CurrentHeap = new FrozenObjectHeap(m_HeapSize, m_HeapCommitChunkSize); + m_FrozenHeaps.Append(m_CurrentHeap); + _ASSERT(m_CurrentHeap != nullptr); } - if (alloc != nullptr) + Object* obj = m_CurrentHeap->AllocateObject(objectSize); + + // The only case where it might be null is when the current heap is full and we need + // to create a new one + if (obj == nullptr) { - // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect - // DATA_ALIGNMENT alignment as well - _ASSERT(IS_ALIGNED(alloc, DATA_ALIGNMENT)); - - segment_info si; - si.pvMem = alloc; - si.ibFirstObject = sizeof(ObjHeader); - si.ibAllocated = si.ibFirstObject; - si.ibCommit = m_CommitChunkSize; - si.ibReserved = m_SizeReserved; - - m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); - if (m_SegmentHandle != nullptr) - { - m_pStart = static_cast(alloc); - m_pCurrent = m_pStart; - m_SizeCommitted = si.ibCommit; - INDEBUG(m_ObjectsCount = 0); - return true; - } + m_CurrentHeap = new FrozenObjectHeap(m_HeapSize, m_HeapCommitChunkSize); + m_FrozenHeaps.Append(m_CurrentHeap); + + // Try again + obj = m_CurrentHeap->AllocateObject(objectSize); - // GC refused to register frozen segment (OOM?) - ClrVirtualFree(m_pStart, 0, MEM_RELEASE); - m_pStart = nullptr; + // This time it's not expected to be null + _ASSERT(obj != nullptr); } - return false; + return obj; } -Object* FrozenObjectHeap::AllocateObject(size_t objectSize) +FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): + m_pStart(nullptr), + m_pCurrent(nullptr), + m_CommitChunkSize(0), + m_SizeCommitted(0), + m_SegmentHandle(nullptr) + COMMA_INDEBUG(m_ObjectsCount(0)) { - // NOTE: objectSize is expected be the full size including header - _ASSERT(objectSize >= MIN_OBJECT_SIZE); + m_SizeReserved = reserveSize; + m_CommitChunkSize = commitChunkSize; - CrstHolder ch(&m_Crst); + void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); + if (alloc == nullptr) + { + ThrowOutOfMemory(); + } - if (m_pStart == nullptr) + // Commit a chunk in advance + alloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); + if (alloc == nullptr) { - // m_SizeReserved > 0 means we already tried to init and it failed. - // so bail out to avoid doing Alloc again. - if ((m_SizeReserved > 0) || !Initialize()) - { - return nullptr; - } + ThrowOutOfMemory(); } - if (objectSize > m_CommitChunkSize) + // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect + // DATA_ALIGNMENT alignment as well + _ASSERT(IS_ALIGNED(alloc, DATA_ALIGNMENT)); + + segment_info si; + si.pvMem = alloc; + si.ibFirstObject = sizeof(ObjHeader); + si.ibAllocated = si.ibFirstObject; + si.ibCommit = m_CommitChunkSize; + si.ibReserved = m_SizeReserved; + + m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); + if (m_SegmentHandle == nullptr) { - // The current design doesn't allow object larger than FOH_COMMIT_CHUNK_SIZE and - // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects. - return nullptr; + ThrowOutOfMemory(); } - _ASSERT(m_pStart != nullptr); - _ASSERT(m_SegmentHandle != nullptr); + m_pStart = static_cast(alloc); + m_pCurrent = m_pStart; + m_SizeCommitted = si.ibCommit; + INDEBUG(m_ObjectsCount = 0); + return; +} + +Object* FrozenObjectHeap::AllocateObject(size_t objectSize) +{ + _ASSERT(m_pStart != nullptr && m_SizeReserved > 0 && m_SegmentHandle != nullptr); // Expected to be inited _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); uint8_t* obj = m_pCurrent; - if ((size_t)(m_pStart + m_SizeReserved) < (size_t)(obj + objectSize)) + if (reinterpret_cast(m_pStart + m_SizeReserved) < reinterpret_cast(obj + objectSize)) { - // heap is full, caller is expected to switch to other heaps - // TODO: register a new frozen segment + // heap is full return nullptr; } // Check if we need to commit a new chunk - if ((size_t)(m_pStart + m_SizeCommitted) < (size_t)(obj + objectSize)) + if (reinterpret_cast(m_pStart + m_SizeCommitted) < reinterpret_cast(obj + objectSize)) { _ASSERT(m_SizeCommitted + m_CommitChunkSize <= m_SizeReserved); if (ClrVirtualAlloc(m_pStart + m_SizeCommitted, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) { - // We failed to commit a new chunk of the reserved memory - return nullptr; + ThrowOutOfMemory(); } m_SizeCommitted += m_CommitChunkSize; } INDEBUG(m_ObjectsCount++); + m_pCurrent = obj + objectSize; // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 6516e099243fe..371e104caf5dc 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -6,24 +6,37 @@ #include "common.h" #include "gcinterface.h" +#include -class FrozenObjectHeap +class FrozenObjectHeap; + +class FrozenObjectHeapManager { public: - FrozenObjectHeap(); - ~FrozenObjectHeap(); + FrozenObjectHeapManager(); Object* AllocateObject(size_t objectSize); private: - bool Initialize(); + CrstExplicitInit m_Crst; + SArray m_FrozenHeaps; + FrozenObjectHeap* m_CurrentHeap; + size_t m_HeapCommitChunkSize; + size_t m_HeapSize; +}; +class FrozenObjectHeap +{ +public: + FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize); + Object* AllocateObject(size_t objectSize); + +private: uint8_t* m_pStart; uint8_t* m_pCurrent; size_t m_CommitChunkSize; size_t m_SizeCommitted; size_t m_SizeReserved; segment_handle m_SegmentHandle; - CrstExplicitInit m_Crst; INDEBUG(size_t m_ObjectsCount); }; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 6e8c30a27fe20..efba4a57f7606 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -887,10 +887,10 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) if (preferFrozenHeap) { - FrozenObjectHeap* foh = SystemDomain::GetSegmentWithFrozenObjects(); + FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); if (foh != nullptr) { - orString = (StringObject*)foh->AllocateObject(totalSize); + orString = static_cast(foh->AllocateObject(totalSize)); if (orString != nullptr) { orString->SetMethodTable(g_pStringClass); From 4ef34dd194a7756e40b9297a19bf422702504676 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 31 Jul 2022 10:01:51 +0200 Subject: [PATCH 32/57] Clean up --- src/coreclr/inc/clrconfigvalues.h | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 789fe062d8814..cfcc5ebc32fd2 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -406,15 +406,8 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_BreakOnOutOfMemoryWithinRange, W("BreakOnOutOf /// /// Frozen segments (aka Frozen Object Heap) /// -#ifdef _DEBUG -#define FrozenSegmentReserveSize (0x4000) // 16Kb -#define FrozenSegmentCommitSize (0x1000) // 4Kb -#else -#define FrozenSegmentReserveSize (0x400000) // 4Mb -#define FrozenSegmentCommitSize (0x10000) // 64Kb -#endif -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), FrozenSegmentReserveSize, "Amount of memory to reserve for a frozen segment") -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), FrozenSegmentCommitSize, "Amount of memory to commit on demand for frozen segments") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for a frozen segment") // 4Mb +RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb /// /// Log From 722eb8217b2c86be973b8a0f9eb5592771d8f1d7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 31 Jul 2022 13:25:27 +0200 Subject: [PATCH 33/57] fix typo --- src/coreclr/gc/gcee.cpp | 4 ++-- src/coreclr/gc/gcimpl.h | 2 +- src/coreclr/gc/gcinterface.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/gc/gcee.cpp b/src/coreclr/gc/gcee.cpp index a2dd58435d77e..c9ed484078151 100644 --- a/src/coreclr/gc/gcee.cpp +++ b/src/coreclr/gc/gcee.cpp @@ -474,11 +474,11 @@ segment_handle GCHeap::RegisterFrozenSegment(segment_info *pseginfo) #endif // FEATURE_BASICFREEZE } -void GCHeap::UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited) +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) = commited; + heap_segment_committed (heap_seg) = committed; heap_segment_allocated (heap_seg) = allocated; #endif // FEATURE_BASICFREEZE } diff --git a/src/coreclr/gc/gcimpl.h b/src/coreclr/gc/gcimpl.h index 1bde6af6b3bc7..23ddcbc67e73a 100644 --- a/src/coreclr/gc/gcimpl.h +++ b/src/coreclr/gc/gcimpl.h @@ -245,7 +245,7 @@ class GCHeap : public IGCHeapInternal virtual segment_handle RegisterFrozenSegment(segment_info *pseginfo); virtual void UnregisterFrozenSegment(segment_handle seg); virtual bool IsInFrozenSegment(Object *object); - virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited); + virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed); // Event control functions void ControlEvents(GCEventKeyword keyword, GCEventLevel level); diff --git a/src/coreclr/gc/gcinterface.h b/src/coreclr/gc/gcinterface.h index ef417ce6b118f..6ad0936f5cf2a 100644 --- a/src/coreclr/gc/gcinterface.h +++ b/src/coreclr/gc/gcinterface.h @@ -915,7 +915,7 @@ class IGCHeap { virtual segment_handle RegisterFrozenSegment(segment_info *pseginfo) = 0; // Updates given frozen segment - virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* commited) = 0; + virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed) = 0; // Unregisters a frozen segment. virtual void UnregisterFrozenSegment(segment_handle seg) = 0; From 0ae890a6086c77cdc1e7c8509e6e64d97a86468d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 31 Jul 2022 19:37:51 +0200 Subject: [PATCH 34/57] Fix assert in assertionprop.cpp --- src/coreclr/jit/assertionprop.cpp | 6 ++++-- src/coreclr/vm/frozenobjectheap.cpp | 15 +++++++++------ src/coreclr/vm/frozenobjectheap.h | 4 ++-- src/coreclr/vm/gchelpers.cpp | 3 +-- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 292c286ec7f9a..7501e2a7e76d1 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2580,8 +2580,10 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) GenTree* objectNode = call->gtArgs.GetArgByIndex(1)->GetNode(); GenTree* methodTableNode = call->gtArgs.GetArgByIndex(0)->GetNode(); - assert(objectNode->TypeGet() == TYP_REF); - assert(methodTableNode->TypeGet() == TYP_I_IMPL); + // objectNode can be TYP_I_IMPL in case if it's a constant handle + // (e.g. a string literal from frozen segments) + assert(objectNode->TypeIs(TYP_REF, TYP_I_IMPL)); + assert(methodTableNode->TypeIs(TYP_I_IMPL)); // Reverse the assertion assert((assertionKind == OAK_EQUAL) || (assertionKind == OAK_NOT_EQUAL)); diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 0577ebf09461b..f96b2c2f103bb 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -18,7 +18,7 @@ FrozenObjectHeapManager::FrozenObjectHeapManager(): // 1) DOTNET_FrozenSegmentReserveSize is 0 (disabled) // 2) Object is too large (large than DOTNET_FrozenSegmentCommitSize) // in such cases caller is responsible to find a more appropriate heap to allocate it -Object* FrozenObjectHeapManager::AllocateObject(size_t objectSize) +Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t objectSize) { CONTRACTL { @@ -35,6 +35,7 @@ Object* FrozenObjectHeapManager::AllocateObject(size_t objectSize) return nullptr; } + _ASSERT(type != nullptr); _ASSERT(m_HeapCommitChunkSize >= MIN_OBJECT_SIZE); _ASSERT(m_HeapSize > m_HeapCommitChunkSize); _ASSERT(m_HeapSize % m_HeapCommitChunkSize == 0); @@ -57,7 +58,7 @@ Object* FrozenObjectHeapManager::AllocateObject(size_t objectSize) _ASSERT(m_CurrentHeap != nullptr); } - Object* obj = m_CurrentHeap->AllocateObject(objectSize); + Object* obj = m_CurrentHeap->AllocateObject(type, objectSize); // The only case where it might be null is when the current heap is full and we need // to create a new one @@ -67,7 +68,7 @@ Object* FrozenObjectHeapManager::AllocateObject(size_t objectSize) m_FrozenHeaps.Append(m_CurrentHeap); // Try again - obj = m_CurrentHeap->AllocateObject(objectSize); + obj = m_CurrentHeap->AllocateObject(type, objectSize); // This time it's not expected to be null _ASSERT(obj != nullptr); @@ -124,7 +125,7 @@ FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): return; } -Object* FrozenObjectHeap::AllocateObject(size_t objectSize) +Object* FrozenObjectHeap::AllocateObject(PTR_MethodTable type, size_t objectSize) { _ASSERT(m_pStart != nullptr && m_SizeReserved > 0 && m_SegmentHandle != nullptr); // Expected to be inited _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); @@ -151,9 +152,11 @@ Object* FrozenObjectHeap::AllocateObject(size_t objectSize) m_pCurrent = obj + objectSize; + Object* object = reinterpret_cast(obj + sizeof(ObjHeader)); + object->SetMethodTable(type); + // 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); - // Skip object header - return reinterpret_cast(obj + sizeof(ObjHeader)); + return object; } diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 371e104caf5dc..3247b1b5bf663 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -14,7 +14,7 @@ class FrozenObjectHeapManager { public: FrozenObjectHeapManager(); - Object* AllocateObject(size_t objectSize); + Object* AllocateObject(PTR_MethodTable type, size_t objectSize); private: CrstExplicitInit m_Crst; @@ -28,7 +28,7 @@ class FrozenObjectHeap { public: FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize); - Object* AllocateObject(size_t objectSize); + Object* AllocateObject(PTR_MethodTable type, size_t objectSize); private: uint8_t* m_pStart; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index efba4a57f7606..e3877568e5ee0 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -890,10 +890,9 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); if (foh != nullptr) { - orString = static_cast(foh->AllocateObject(totalSize)); + orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); if (orString != nullptr) { - orString->SetMethodTable(g_pStringClass); orString->SetStringLength(cchStringLength); _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); orStringRef = ObjectToSTRINGREF(orString); From e59d27adccf56416de7ceeacd672d39dbf9cb834 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 18 Aug 2022 14:16:06 +0200 Subject: [PATCH 35/57] Address feedback --- src/coreclr/gc/gcee.cpp | 18 +++++++++--------- src/coreclr/gc/gcinterface.h | 6 +++--- src/coreclr/jit/codegencommon.cpp | 2 -- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/coreclr/gc/gcee.cpp b/src/coreclr/gc/gcee.cpp index c9ed484078151..470d334e2fee5 100644 --- a/src/coreclr/gc/gcee.cpp +++ b/src/coreclr/gc/gcee.cpp @@ -474,15 +474,6 @@ segment_handle GCHeap::RegisterFrozenSegment(segment_info *pseginfo) #endif // FEATURE_BASICFREEZE } -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; -#endif // FEATURE_BASICFREEZE -} - void GCHeap::UnregisterFrozenSegment(segment_handle seg) { #ifdef FEATURE_BASICFREEZE @@ -514,6 +505,15 @@ bool GCHeap::IsInFrozenSegment(Object *object) #endif } +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; +#endif // FEATURE_BASICFREEZE +} + bool GCHeap::RuntimeStructuresValid() { return GCScan::GetGcRuntimeStructuresValid(); diff --git a/src/coreclr/gc/gcinterface.h b/src/coreclr/gc/gcinterface.h index 6ad0936f5cf2a..73672e1413183 100644 --- a/src/coreclr/gc/gcinterface.h +++ b/src/coreclr/gc/gcinterface.h @@ -914,9 +914,6 @@ class IGCHeap { // Registers a frozen segment with the GC. virtual segment_handle RegisterFrozenSegment(segment_info *pseginfo) = 0; - // Updates given frozen segment - virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed) = 0; - // Unregisters a frozen segment. virtual void UnregisterFrozenSegment(segment_handle seg) = 0; @@ -949,6 +946,9 @@ class IGCHeap { // Gets all the names and values of the GC configurations. virtual void EnumerateConfigurationValues(void* context, ConfigurationValueFunc configurationValueFunc) = 0; + + // Updates given frozen segment + virtual void UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed) = 0; }; #ifdef WRITE_BARRIER_CHECK diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 8a68a9707ba28..84ea5b4c10342 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1374,8 +1374,6 @@ bool CodeGen::genCreateAddrMode( if (varTypeIsGC(rv2->TypeGet())) { - noway_assert(rv1 && !varTypeIsGC(rv1->TypeGet())); - tmp = rv1; rv1 = rv2; rv2 = tmp; From b6fae80861c9862a71bb7aa778cd7910362d9f99 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 1 Sep 2022 11:21:35 +0200 Subject: [PATCH 36/57] Address feedback --- src/coreclr/inc/corinfo.h | 1 + src/coreclr/vm/gchelpers.cpp | 21 +++++++-------------- src/coreclr/vm/object.h | 7 +++++++ src/coreclr/vm/stringliteralmap.cpp | 2 +- src/coreclr/vm/syncblk.h | 4 +++- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 9a91fc92ae8a9..6f4bdb8d9a604 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -1931,6 +1931,7 @@ struct CORINFO_VarArgInfo #define SIZEOF__CORINFO_Object TARGET_POINTER_SIZE /* methTable */ #define CORINFO_Array_MaxLength 0x7FFFFFC7 +#define CORINFO_String_MaxLength 0x3FFFFFDF #define OFFSETOF__CORINFO_Array__length SIZEOF__CORINFO_Object #ifdef TARGET_64BIT diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index e3877568e5ee0..5a8d40fad686a 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -839,9 +839,7 @@ STRINGREF AllocateString( DWORD cchStringLength ) // Limit the maximum string size to <2GB to mitigate risk of security issues caused by 32-bit integer // overflows in buffer size calculations. - // - // If the value below is changed, also change AllocateUtf8String. - if (cchStringLength > 0x3FFFFFDF) + if (cchStringLength > CORINFO_String_MaxLength) ThrowOutOfMemory(); SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); @@ -877,9 +875,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) // Limit the maximum string size to <2GB to mitigate risk of security issues caused by 32-bit integer // overflows in buffer size calculations. - // - // If the value below is changed, also change AllocateUtf8String. - if (cchStringLength > 0x3FFFFFDF) + if (cchStringLength > CORINFO_String_MaxLength) ThrowOutOfMemory(); const SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); @@ -888,15 +884,12 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) if (preferFrozenHeap) { FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); - if (foh != nullptr) + orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); + if (orString != nullptr) { - orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); - if (orString != nullptr) - { - orString->SetStringLength(cchStringLength); - _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); - orStringRef = ObjectToSTRINGREF(orString); - } + orString->SetStringLength(cchStringLength); + _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); + orStringRef = ObjectToSTRINGREF(orString); } } if (orString == nullptr) diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 4c731c4a240f4..a88a4735aa239 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -219,6 +219,13 @@ class Object return dac_cast(this); } + // Is object located in a frozen segment? + bool IsFrozen() + { + WRAPPER_NO_CONTRACT; + return (GetHeader()->GetBits() & BIT_SBLK_FROZEN) != 0; + } + #ifdef _DEBUG // TRUE if the header has a real SyncBlockIndex (i.e. it has an entry in the // SyncTable, though it doesn't necessarily have an entry in the SyncBlockCache) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 87b918c10bda6..83d24aa5b0857 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -207,7 +207,7 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA if (pStrObj != nullptr && preferFrozenObjectHeap && ppPinnedString != nullptr) { Object* underlyingStrObj = *reinterpret_cast(pStrObj); - if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(underlyingStrObj)) + if (underlyingStrObj->IsFrozen()) { *ppPinnedString = underlyingStrObj; } diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index dbad515e8bb30..fe8f56a15bcc2 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -92,7 +92,9 @@ typedef DPTR(EnCSyncBlockInfo) PTR_EnCSyncBlockInfo; // reducing the mask. We use the very high bit, in _DEBUG, to be sure we never forget // to mask the Value to obtain the Index -#define BIT_SBLK_UNUSED 0x80000000 +// This bit indicates that an object (only applicable to String objects at the moment) +// is located in a frozen segment +#define BIT_SBLK_FROZEN 0x80000000 #define BIT_SBLK_FINALIZER_RUN 0x40000000 #define BIT_SBLK_GC_RESERVE 0x20000000 From d20cc47ba1110665c223f104a90cc1ea77f9ebf6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 2 Sep 2022 16:02:11 +0200 Subject: [PATCH 37/57] Address feedback --- src/coreclr/inc/crsttypes.h | 2 ++ src/coreclr/vm/appdomain.hpp | 5 +-- src/coreclr/vm/frozenobjectheap.cpp | 4 +++ src/coreclr/vm/gchelpers.cpp | 6 +++- src/coreclr/vm/gchelpers.h | 2 +- src/coreclr/vm/object.h | 7 ----- src/coreclr/vm/stringliteralmap.cpp | 26 +++++++-------- src/coreclr/vm/stringliteralmap.h | 49 ++++++++++++++++++++++------- src/coreclr/vm/syncblk.h | 8 +---- 9 files changed, 63 insertions(+), 46 deletions(-) diff --git a/src/coreclr/inc/crsttypes.h b/src/coreclr/inc/crsttypes.h index 3b9f455190668..cc3d34ab58e2b 100644 --- a/src/coreclr/inc/crsttypes.h +++ b/src/coreclr/inc/crsttypes.h @@ -259,6 +259,7 @@ int g_rgCrstLevelMap[] = 3, // CrstUnwindInfoTableLock 4, // CrstVSDIndirectionCellLock 3, // CrstWrapperTemplate + 0, // CrstFrozenObjectHeap }; // An array mapping CrstType to a stringized name. @@ -380,6 +381,7 @@ LPCSTR g_rgCrstNameMap[] = "CrstUnwindInfoTableLock", "CrstVSDIndirectionCellLock", "CrstWrapperTemplate", + "CrstFrozenObjectHeap" }; // Define a special level constant for unordered locks. diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 2897817e39a3f..d56157f8e6ee6 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -2465,13 +2465,10 @@ class SystemDomain : public BaseDomain static FrozenObjectHeapManager* GetFrozenObjectHeapManager() { WRAPPER_NO_CONTRACT; - -#ifdef FEATURE_BASICFREEZE if (m_FrozenObjectHeapManager == NULL) { - SystemDomain::LazyInitFrozenObjectsHeap(); + LazyInitFrozenObjectsHeap(); } -#endif return m_FrozenObjectHeapManager; } #endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index f96b2c2f103bb..74da0778f48ad 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -27,6 +27,10 @@ Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t obj } CONTRACTL_END +#ifndef FEATURE_BASICFREEZE + return nullptr; +#endif + CrstHolder ch(&m_Crst); // Quick way to disable Frozen Heaps diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 5a8d40fad686a..25a2194b01d9e 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -862,7 +862,7 @@ STRINGREF AllocateString( DWORD cchStringLength ) } -STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIsFrozen) { CONTRACTL{ THROWS; @@ -870,6 +870,8 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) MODE_COOPERATIVE; } CONTRACTL_END; + _ASSERT(pIsFrozen != nullptr); + STRINGREF orStringRef = NULL; StringObject* orString = nullptr; @@ -890,11 +892,13 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap) orString->SetStringLength(cchStringLength); _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0')); orStringRef = ObjectToSTRINGREF(orString); + *pIsFrozen = true; } } if (orString == nullptr) { orStringRef = AllocateString(cchStringLength); + *pIsFrozen = false; } return orStringRef; } diff --git a/src/coreclr/vm/gchelpers.h b/src/coreclr/vm/gchelpers.h index 75d3788b79289..d861f33a34f75 100644 --- a/src/coreclr/vm/gchelpers.h +++ b/src/coreclr/vm/gchelpers.h @@ -35,7 +35,7 @@ OBJECTREF AllocateObjectArray(DWORD cElements, TypeHandle ElementType, BOOL bAll // Allocate a string STRINGREF AllocateString(DWORD cchStringLength); -STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap); +STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIsFrozen); OBJECTREF DupArrayForCloning(BASEARRAYREF pRef); diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index a88a4735aa239..4c731c4a240f4 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -219,13 +219,6 @@ class Object return dac_cast(this); } - // Is object located in a frozen segment? - bool IsFrozen() - { - WRAPPER_NO_CONTRACT; - return (GetHeader()->GetBits() & BIT_SBLK_FROZEN) != 0; - } - #ifdef _DEBUG // TRUE if the header has a real SyncBlockIndex (i.e. it has an entry in the // SyncTable, though it doesn't necessarily have an entry in the SyncBlockCache) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 83d24aa5b0857..929b0708612af 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -204,13 +204,10 @@ STRINGREF *StringLiteralMap::GetStringLiteral(EEStringData *pStringData, BOOL bA pStrObj = pEntry->GetStringObject(); _ASSERTE(!bAddIfNotFound || pStrObj); - if (pStrObj != nullptr && preferFrozenObjectHeap && ppPinnedString != nullptr) + + if (pStrObj != nullptr && ppPinnedString != nullptr && preferFrozenObjectHeap && pEntry->IsStringFrozen()) { - Object* underlyingStrObj = *reinterpret_cast(pStrObj); - if (underlyingStrObj->IsFrozen()) - { - *ppPinnedString = underlyingStrObj; - } + *ppPinnedString = *reinterpret_cast(pStrObj); } return pStrObj; } @@ -455,7 +452,7 @@ static void LogStringLiteral(_In_z_ const char* action, EEStringData *pStringDat } #endif -STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap) +STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap, bool* pIsFrozen) { CONTRACTL { @@ -468,7 +465,7 @@ STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHe // Create the COM+ string object. DWORD cCount = pStringData->GetCharCount(); - STRINGREF strObj = AllocateString(cCount, preferFrozenObjHeap); + STRINGREF strObj = AllocateString(cCount, preferFrozenObjHeap, pIsFrozen); GCPROTECT_BEGIN(strObj) { @@ -500,15 +497,16 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri { PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); + // Create the COM+ string object. - STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap); + bool isFrozen = false; + STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap, &isFrozen); // Allocate a handle for the string. SetObjectReference(pStrObj[0], (OBJECTREF) strObj); - // Allocate the StringLiteralEntry. - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0])); + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0], isFrozen)); pStrObj.SuppressRelease(); // Insert the handle to the string into the hash table. m_StringToEntryHashTable->InsertValue(pStringData, (LPVOID)pEntry, FALSE); @@ -583,7 +581,7 @@ void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry // release method of the StringLiteralEntry. } -StringLiteralEntry *StringLiteralEntry::AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj) +StringLiteralEntry *StringLiteralEntry::AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen) { CONTRACTL { @@ -615,7 +613,7 @@ StringLiteralEntry *StringLiteralEntry::AllocateEntry(EEStringData *pStringData, } _ASSERTE (pMem && "Unable to allocate String literal Entry"); - return new (pMem) StringLiteralEntry (pStringData, pStringObj); + return new (pMem) StringLiteralEntry (pStringData, pStringObj, isFrozen); } void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) @@ -628,7 +626,7 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) } CONTRACTL_END; - _ASSERTE (VolatileLoad(&pEntry->m_dwRefCount) == 0); + _ASSERTE (pEntry->GetRefCount() == 0); #ifdef _DEBUG memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index e6d03d9d26adb..f1908a58f0a40 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -22,7 +22,7 @@ class StringLiteralEntry; // Allocate 16 entries (approx size sizeof(StringLiteralEntry)*16) #define MAX_ENTRIES_PER_CHUNK 16 -STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap); +STRINGREF AllocateStringObject(EEStringData *pStringData, bool preferFrozenObjHeap, bool* pIsFrozen); // Loader allocator specific string literal map. class StringLiteralMap @@ -123,13 +123,21 @@ class StringLiteralEntryArray; // Ref counted entry representing a string literal. class StringLiteralEntry { + #define SLE_IS_FROZEN (1u << 31) + #define SLE_IS_OVERFLOWN (1u << 30) + #define SLE_REFCOUNT_MASK (SLE_IS_FROZEN | SLE_IS_OVERFLOWN) + private: - StringLiteralEntry(EEStringData *pStringData, STRINGREF *pStringObj) + StringLiteralEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen) : m_pStringObj(pStringObj), m_dwRefCount(1) #ifdef _DEBUG , m_bDeleted(FALSE) #endif { + if (isFrozen) + { + SetStringFrozen(); + } LIMITED_METHOD_CONTRACT; } protected: @@ -152,7 +160,7 @@ class StringLiteralEntry NOTHROW; GC_NOTRIGGER; PRECONDITION(CheckPointer(this)); - PRECONDITION((LONG)VolatileLoad(&m_dwRefCount) > 0); + PRECONDITION(GetRefCount() > 0); PRECONDITION(SystemDomain::GetGlobalStringLiteralMapNoCreate()->m_HashTableCrstGlobal.OwnedByCurrentThread()); } CONTRACTL_END; @@ -160,9 +168,11 @@ class StringLiteralEntry _ASSERTE (!m_bDeleted); // We will keep the item alive forever if the refcount overflowed - if ((LONG)VolatileLoad(&m_dwRefCount) < 0) + if (IsRefCountOverflowed()) return; + // the following increment may set SLE_IS_OVERFLOWN bit on it's own if we have more than + // 1073741823 strings in the map VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) + 1); } #ifndef DACCESS_COMPILE @@ -192,17 +202,17 @@ class StringLiteralEntry NOTHROW; GC_NOTRIGGER; PRECONDITION(CheckPointer(this)); - PRECONDITION(VolatileLoad(&m_dwRefCount) > 0); + PRECONDITION(GetRefCount() > 0); PRECONDITION(SystemDomain::GetGlobalStringLiteralMapNoCreate()->m_HashTableCrstGlobal.OwnedByCurrentThread()); } CONTRACTL_END; // We will keep the item alive forever if the refcount overflowed - if ((LONG)VolatileLoad(&m_dwRefCount) < 0) + if (IsRefCountOverflowed()) return; VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) - 1); - if (VolatileLoad(&m_dwRefCount) == 0) + if (GetRefCount() == 0) { _ASSERTE(SystemDomain::GetGlobalStringLiteralMapNoCreate()); SystemDomain::GetGlobalStringLiteralMapNoCreate()->RemoveStringLiteralEntry(this); @@ -212,7 +222,7 @@ class StringLiteralEntry } #endif // DACCESS_COMPILE - LONG GetRefCount() + DWORD GetRefCount() { CONTRACTL { @@ -224,7 +234,7 @@ class StringLiteralEntry _ASSERTE (!m_bDeleted); - return (VolatileLoad(&m_dwRefCount)); + return VolatileLoad(&m_dwRefCount) & SLE_REFCOUNT_MASK; } STRINGREF* GetStringObject() @@ -259,15 +269,30 @@ class StringLiteralEntry pStringData->SetStringBuffer (thisChars); } - static StringLiteralEntry *AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj); + static StringLiteralEntry *AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen); static void DeleteEntry (StringLiteralEntry *pEntry); + bool IsStringFrozen() + { + return VolatileLoad(&m_dwRefCount) & SLE_IS_FROZEN; + } + + void SetStringFrozen() + { + VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) | SLE_IS_FROZEN); + } + + bool IsRefCountOverflowed() + { + return VolatileLoad(&m_dwRefCount) & SLE_IS_OVERFLOWN; + } + private: STRINGREF* m_pStringObj; union { - DWORD m_dwRefCount; - StringLiteralEntry *m_pNext; + DWORD m_dwRefCount; + StringLiteralEntry *m_pNext; }; #ifdef _DEBUG diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index fe8f56a15bcc2..17d8074c810bf 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -74,13 +74,9 @@ class AppDomain; #ifdef EnC_SUPPORTED class EnCSyncBlockInfo; typedef DPTR(EnCSyncBlockInfo) PTR_EnCSyncBlockInfo; - #endif // EnC_SUPPORTED #include "eventstore.hpp" - -#include "eventstore.hpp" - #include "synch.h" // At a negative offset from each Object is an ObjHeader. The 'size' of the @@ -92,9 +88,7 @@ typedef DPTR(EnCSyncBlockInfo) PTR_EnCSyncBlockInfo; // reducing the mask. We use the very high bit, in _DEBUG, to be sure we never forget // to mask the Value to obtain the Index -// This bit indicates that an object (only applicable to String objects at the moment) -// is located in a frozen segment -#define BIT_SBLK_FROZEN 0x80000000 +#define BIT_SBLK_UNUSED 0x80000000 #define BIT_SBLK_FINALIZER_RUN 0x40000000 #define BIT_SBLK_GC_RESERVE 0x20000000 From 31b5fdc7ead87e4debcc0fd3b7df8525a5b6ed17 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 3 Sep 2022 14:47:14 +0200 Subject: [PATCH 38/57] Fix potential issues --- src/coreclr/vm/stringliteralmap.h | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index f1908a58f0a40..322a022cc90e5 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -124,8 +124,8 @@ class StringLiteralEntryArray; class StringLiteralEntry { #define SLE_IS_FROZEN (1u << 31) - #define SLE_IS_OVERFLOWN (1u << 30) - #define SLE_REFCOUNT_MASK (SLE_IS_FROZEN | SLE_IS_OVERFLOWN) + #define SLE_IS_OVERFLOWED (1u << 30) + #define SLE_REFCOUNT_MASK (SLE_IS_FROZEN | SLE_IS_OVERFLOWED) private: StringLiteralEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen) @@ -160,7 +160,7 @@ class StringLiteralEntry NOTHROW; GC_NOTRIGGER; PRECONDITION(CheckPointer(this)); - PRECONDITION(GetRefCount() > 0); + PRECONDITION(GetRefCount() != 0); PRECONDITION(SystemDomain::GetGlobalStringLiteralMapNoCreate()->m_HashTableCrstGlobal.OwnedByCurrentThread()); } CONTRACTL_END; @@ -171,9 +171,17 @@ class StringLiteralEntry if (IsRefCountOverflowed()) return; - // the following increment may set SLE_IS_OVERFLOWN bit on it's own if we have more than - // 1073741823 strings in the map - VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) + 1); + const DWORD refCount = GetRefCount() + 1; + if (refCount & SLE_IS_OVERFLOWED) + { + // We just overflowed m_dwRefCount (1073741823 + 1), make refCount = 1 and leave a mark SLE_IS_OVERFLOWED + // so the object will be always alive now + VolatileStore(&m_dwRefCount, (DWORD)(SLE_IS_OVERFLOWED | 1)); + } + else + { + VolatileStore(&m_dwRefCount, refCount); + } } #ifndef DACCESS_COMPILE FORCEINLINE static void StaticRelease(StringLiteralEntry* pEntry) @@ -234,7 +242,7 @@ class StringLiteralEntry _ASSERTE (!m_bDeleted); - return VolatileLoad(&m_dwRefCount) & SLE_REFCOUNT_MASK; + return VolatileLoad(&m_dwRefCount) & ~SLE_REFCOUNT_MASK; } STRINGREF* GetStringObject() @@ -284,7 +292,7 @@ class StringLiteralEntry bool IsRefCountOverflowed() { - return VolatileLoad(&m_dwRefCount) & SLE_IS_OVERFLOWN; + return VolatileLoad(&m_dwRefCount) & SLE_IS_OVERFLOWED; } private: From 1b1ab4c02a5e527fd9565628349d55230d5695c3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 3 Sep 2022 14:51:15 +0200 Subject: [PATCH 39/57] Fix potential issues --- src/coreclr/vm/stringliteralmap.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index 322a022cc90e5..2c95048ce006c 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -174,9 +174,7 @@ class StringLiteralEntry const DWORD refCount = GetRefCount() + 1; if (refCount & SLE_IS_OVERFLOWED) { - // We just overflowed m_dwRefCount (1073741823 + 1), make refCount = 1 and leave a mark SLE_IS_OVERFLOWED - // so the object will be always alive now - VolatileStore(&m_dwRefCount, (DWORD)(SLE_IS_OVERFLOWED | 1)); + SetRefCountOverflowed(); } else { @@ -295,6 +293,11 @@ class StringLiteralEntry return VolatileLoad(&m_dwRefCount) & SLE_IS_OVERFLOWED; } + void SetRefCountOverflowed() + { + VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) | SLE_IS_OVERFLOWED); + } + private: STRINGREF* m_pStringObj; union From 88ad320b13db5edb6952daa613bfdfac6bd1befa Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Sep 2022 21:39:02 +0200 Subject: [PATCH 40/57] Address feedback --- src/coreclr/vm/stringliteralmap.cpp | 49 ++++++++++++++++++++--------- src/coreclr/vm/stringliteralmap.h | 32 ++++++++++++++----- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 929b0708612af..465d6f2525c9d 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -495,28 +495,37 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri StringLiteralEntry *pRet; - { - PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); - // Create the COM+ string object. bool isFrozen = false; STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap, &isFrozen); - // Allocate a handle for the string. - SetObjectReference(pStrObj[0], (OBJECTREF) strObj); + if (isFrozen) + { + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateFrozenEntry(pStringData, strObj)); + m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); + pEntry.SuppressRelease(); + pRet = pEntry; + } + else + { + // If it's not frozen we need to gc allocate a pinned handle to the non-pinned string object + PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable, 1); - // Allocate the StringLiteralEntry. - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0], isFrozen)); - pStrObj.SuppressRelease(); - // Insert the handle to the string into the hash table. - m_StringToEntryHashTable->InsertValue(pStringData, (LPVOID)pEntry, FALSE); - pEntry.SuppressRelease(); - pRet = pEntry; + // Allocate a handle for the string. + SetObjectReference(pStrObj[0], strObj); + + // Allocate the StringLiteralEntry. + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0])); + pStrObj.SuppressRelease(); + // Insert the handle to the string into the hash table. + m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); + pEntry.SuppressRelease(); + pRet = pEntry; + } #ifdef LOGGING LogStringLiteral("added", pStringData); #endif - } return pRet; } @@ -581,7 +590,7 @@ void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry // release method of the StringLiteralEntry. } -StringLiteralEntry *StringLiteralEntry::AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen) +void* StringLiteralEntry::AllocateEntryInternal() { CONTRACTL { @@ -613,7 +622,17 @@ StringLiteralEntry *StringLiteralEntry::AllocateEntry(EEStringData *pStringData, } _ASSERTE (pMem && "Unable to allocate String literal Entry"); - return new (pMem) StringLiteralEntry (pStringData, pStringObj, isFrozen); + return pMem; +} + +StringLiteralEntry* StringLiteralEntry::AllocateEntry(EEStringData* pStringData, STRINGREF* pStringObj) +{ + return new (AllocateEntryInternal()) StringLiteralEntry(pStringData, pStringObj); +} + +StringLiteralEntry* StringLiteralEntry::AllocateFrozenEntry(EEStringData* pStringData, STRINGREF pFrozenStringObj) +{ + return new (AllocateEntryInternal()) StringLiteralEntry(pStringData, pFrozenStringObj); } void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index 2c95048ce006c..10769eb89a0af 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -128,18 +128,25 @@ class StringLiteralEntry #define SLE_REFCOUNT_MASK (SLE_IS_FROZEN | SLE_IS_OVERFLOWED) private: - StringLiteralEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen) + StringLiteralEntry(EEStringData *pStringData, STRINGREF *pStringObj) : m_pStringObj(pStringObj), m_dwRefCount(1) #ifdef _DEBUG , m_bDeleted(FALSE) #endif { - if (isFrozen) - { - SetStringFrozen(); - } LIMITED_METHOD_CONTRACT; } + + StringLiteralEntry(EEStringData *pStringData, STRINGREF frozenStringObj) + : m_FrozenStringObj(frozenStringObj), m_dwRefCount(1) +#ifdef _DEBUG + , m_bDeleted(FALSE) +#endif + { + SetStringFrozen(); + LIMITED_METHOD_CONTRACT; + } + protected: ~StringLiteralEntry() { @@ -252,7 +259,7 @@ class StringLiteralEntry PRECONDITION(CheckPointer(this)); } CONTRACTL_END; - return m_pStringObj; + return IsStringFrozen() ? &m_FrozenStringObj : m_pStringObj; } void GetStringData(EEStringData *pStringData) @@ -275,7 +282,12 @@ class StringLiteralEntry pStringData->SetStringBuffer (thisChars); } - static StringLiteralEntry *AllocateEntry(EEStringData *pStringData, STRINGREF *pStringObj, bool isFrozen); +private: + static void* AllocateEntryInternal(); + +public: + static StringLiteralEntry *AllocateEntry(EEStringData *pStringData, STRINGREF* pStringObj); + static StringLiteralEntry* AllocateFrozenEntry(EEStringData* pStringData, STRINGREF pFrozenStringObj); static void DeleteEntry (StringLiteralEntry *pEntry); bool IsStringFrozen() @@ -299,7 +311,11 @@ class StringLiteralEntry } private: - STRINGREF* m_pStringObj; + union + { + STRINGREF* m_pStringObj; + STRINGREF m_FrozenStringObj; + }; union { DWORD m_dwRefCount; From 943a23581ea9caab5749b8ad76ddb8afc7f278cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 4 Sep 2022 22:00:10 +0200 Subject: [PATCH 41/57] Address feedback --- src/coreclr/vm/stringliteralmap.cpp | 5 ++++- src/coreclr/vm/stringliteralmap.h | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 465d6f2525c9d..e9d912e633800 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -648,7 +648,10 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) _ASSERTE (pEntry->GetRefCount() == 0); #ifdef _DEBUG - memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); + if (!pEntry->IsStringFrozen()) + { + memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); + } pEntry->m_bDeleted = TRUE; #endif diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index 10769eb89a0af..aa401c323cc7c 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -277,7 +277,7 @@ class StringLiteralEntry WCHAR *thisChars; int thisLength; - ObjectToSTRINGREF(*(StringObject**)m_pStringObj)->RefInterpretGetStringValuesDangerousForGC(&thisChars, &thisLength); + ObjectToSTRINGREF(*GetStringObject())->RefInterpretGetStringValuesDangerousForGC(&thisChars, &thisLength); pStringData->SetCharCount (thisLength); // thisLength is in WCHARs and that's what EEStringData's char count wants pStringData->SetStringBuffer (thisChars); } From a3bfc8c7ecb5807adf42bb1bcdbe41867be659ab Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Sep 2022 11:16:36 +0200 Subject: [PATCH 42/57] Check if this fixes all asserts --- src/coreclr/vm/gchelpers.cpp | 2 +- src/coreclr/vm/stringliteralmap.cpp | 52 +++++++++++++++-------------- src/coreclr/vm/stringliteralmap.h | 32 ++++++------------ src/coreclr/vm/threads.h | 3 ++ 4 files changed, 41 insertions(+), 48 deletions(-) diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 25a2194b01d9e..46c7b45da38fb 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -883,7 +883,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIs const SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); _ASSERTE(totalSize > cchStringLength); - if (preferFrozenHeap) + if (preferFrozenHeap && cchStringLength > 0) { FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index e9d912e633800..5a5e545ef2e05 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -495,33 +495,37 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri StringLiteralEntry *pRet; - // Create the COM+ string object. bool isFrozen = false; STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap, &isFrozen); - - if (isFrozen) - { - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateFrozenEntry(pStringData, strObj)); - m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); - pEntry.SuppressRelease(); - pRet = pEntry; - } - else + GCPROTECT_BEGIN(strObj) { - // If it's not frozen we need to gc allocate a pinned handle to the non-pinned string object - PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable, 1); + if (isFrozen) + { + _ASSERT(preferFrozenObjHeap); + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateFrozenEntry(pStringData, strObj)); + m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); + pEntry.SuppressRelease(); + pRet = pEntry; + _ASSERT(pRet->IsStringFrozen()); + } + else + { + // If it's not frozen we need to gc allocate a pinned handle to the non-pinned string object + PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); - // Allocate a handle for the string. - SetObjectReference(pStrObj[0], strObj); + // Allocate a handle for the string. + SetObjectReference(pStrObj[0], strObj); - // Allocate the StringLiteralEntry. - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0])); - pStrObj.SuppressRelease(); - // Insert the handle to the string into the hash table. - m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); - pEntry.SuppressRelease(); - pRet = pEntry; + // Allocate the StringLiteralEntry. + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateEntry(pStringData, (STRINGREF*)pStrObj[0])); + pStrObj.SuppressRelease(); + // Insert the handle to the string into the hash table. + m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); + pEntry.SuppressRelease(); + pRet = pEntry; + } } + GCPROTECT_END(); #ifdef LOGGING LogStringLiteral("added", pStringData); @@ -646,12 +650,10 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) CONTRACTL_END; _ASSERTE (pEntry->GetRefCount() == 0); + _ASSERTE (!pEntry->IsStringFrozen()); #ifdef _DEBUG - if (!pEntry->IsStringFrozen()) - { - memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); - } + memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); pEntry->m_bDeleted = TRUE; #endif diff --git a/src/coreclr/vm/stringliteralmap.h b/src/coreclr/vm/stringliteralmap.h index aa401c323cc7c..510b66b32bd68 100644 --- a/src/coreclr/vm/stringliteralmap.h +++ b/src/coreclr/vm/stringliteralmap.h @@ -138,12 +138,11 @@ class StringLiteralEntry } StringLiteralEntry(EEStringData *pStringData, STRINGREF frozenStringObj) - : m_FrozenStringObj(frozenStringObj), m_dwRefCount(1) + : m_FrozenStringObj(frozenStringObj), m_dwRefCount(1 | SLE_IS_FROZEN) #ifdef _DEBUG , m_bDeleted(FALSE) #endif { - SetStringFrozen(); LIMITED_METHOD_CONTRACT; } @@ -174,18 +173,16 @@ class StringLiteralEntry _ASSERTE (!m_bDeleted); - // We will keep the item alive forever if the refcount overflowed - if (IsRefCountOverflowed()) + if (IsAlwaysAlive()) return; - const DWORD refCount = GetRefCount() + 1; - if (refCount & SLE_IS_OVERFLOWED) + if ((GetRefCount() + 1) & SLE_IS_OVERFLOWED) { - SetRefCountOverflowed(); + VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) | SLE_IS_OVERFLOWED); } else { - VolatileStore(&m_dwRefCount, refCount); + VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) + 1); } } #ifndef DACCESS_COMPILE @@ -220,8 +217,7 @@ class StringLiteralEntry } CONTRACTL_END; - // We will keep the item alive forever if the refcount overflowed - if (IsRefCountOverflowed()) + if (IsAlwaysAlive()) return; VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) - 1); @@ -295,19 +291,11 @@ class StringLiteralEntry return VolatileLoad(&m_dwRefCount) & SLE_IS_FROZEN; } - void SetStringFrozen() + bool IsAlwaysAlive() { - VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) | SLE_IS_FROZEN); - } - - bool IsRefCountOverflowed() - { - return VolatileLoad(&m_dwRefCount) & SLE_IS_OVERFLOWED; - } - - void SetRefCountOverflowed() - { - VolatileStore(&m_dwRefCount, VolatileLoad(&m_dwRefCount) | SLE_IS_OVERFLOWED); + // If string literal is either frozen or its counter overflowed + // we'll keep it always alive + return VolatileLoad(&m_dwRefCount) & (SLE_IS_OVERFLOWED | SLE_IS_FROZEN); } private: diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index b51804a471705..54da8e94405fc 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3738,6 +3738,9 @@ class Thread // If the pointer lives in the GC heap, than it is protected, and thus valid. if (dac_cast(g_lowest_address) <= val && val < dac_cast(g_highest_address)) return(true); + // Same for frozen segments + if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(*(Object**)ref)) + return(true); return(false); } From 1be12c502ad74a8ad47d44b45b4b12e99f1fc081 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Sep 2022 11:42:17 +0200 Subject: [PATCH 43/57] Handle string.Empty (it used to be ignored) --- src/coreclr/vm/gchelpers.cpp | 2 +- src/coreclr/vm/jitinterface.cpp | 16 ++++++++++++++-- src/coreclr/vm/object.cpp | 7 +++++-- src/coreclr/vm/object.h | 15 ++++++++++++--- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 46c7b45da38fb..25a2194b01d9e 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -883,7 +883,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIs const SIZE_T totalSize = PtrAlign(StringObject::GetSize(cchStringLength)); _ASSERTE(totalSize > cchStringLength); - if (preferFrozenHeap && cchStringLength > 0) + if (preferFrozenHeap) { FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 055a876cb7eb8..bc146b76ef911 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11652,7 +11652,19 @@ InfoAccessType CEEJitInfo::emptyStringLiteral(void ** ppValue) InfoAccessType result = IAT_PVALUE; JIT_TO_EE_TRANSITION(); - *ppValue = StringObject::GetEmptyStringRefPtr(); + void* pinnedStr = nullptr; + void* pinnedStrHandlePtr = StringObject::GetEmptyStringRefPtr(&pinnedStr); + + if (pinnedStr != nullptr) + { + *ppValue = pinnedStr; + result = IAT_VALUE; + } + else + { + *ppValue = pinnedStr; + } + EE_TO_JIT_TRANSITION(); return result; @@ -13338,7 +13350,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, if (rid == 0) { // Empty string - result = (size_t)StringObject::GetEmptyStringRefPtr(); + result = (size_t)StringObject::GetEmptyStringRefPtr(nullptr); } else { diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index a36b4fe9c86cf..a6a7b20850c49 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -817,7 +817,8 @@ STRINGREF StringObject::NewString(LPCUTF8 psz, int cBytes) // STATIC MEMBER VARIABLES // // -STRINGREF* StringObject::EmptyStringRefPtr=NULL; +STRINGREF* StringObject::EmptyStringRefPtr = NULL; +bool StringObject::EmptyStringIsFrozen = false; //The special string helpers are used as flag bits for weird strings that have bytes //after the terminating 0. The only case where we use this right now is the VB BSTR as @@ -854,7 +855,9 @@ STRINGREF* StringObject::InitEmptyStringRefPtr() { GCX_COOP(); EEStringData data(0, W(""), TRUE); - EmptyStringRefPtr = SystemDomain::System()->DefaultDomain()->GetLoaderAllocator()->GetStringObjRefPtrFromUnicodeString(&data); + void* pinnedStr = nullptr; + EmptyStringRefPtr = SystemDomain::System()->DefaultDomain()->GetLoaderAllocator()->GetStringObjRefPtrFromUnicodeString(&data, &pinnedStr); + EmptyStringIsFrozen = pinnedStr != nullptr; return EmptyStringRefPtr; } diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 4c731c4a240f4..1b7f140aae89f 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -928,7 +928,7 @@ class StringObject : public Object static STRINGREF NewString(LPCUTF8 psz, int cBytes); static STRINGREF GetEmptyString(); - static STRINGREF* GetEmptyStringRefPtr(); + static STRINGREF* GetEmptyStringRefPtr(void** pinnedString); static STRINGREF* InitEmptyStringRefPtr(); @@ -962,6 +962,7 @@ class StringObject : public Object private: static STRINGREF* EmptyStringRefPtr; + static bool EmptyStringIsFrozen; }; /*================================GetEmptyString================================ @@ -990,19 +991,27 @@ inline STRINGREF StringObject::GetEmptyString() { return *refptr; } -inline STRINGREF* StringObject::GetEmptyStringRefPtr() { +inline STRINGREF* StringObject::GetEmptyStringRefPtr(void** pinnedString) { CONTRACTL { THROWS; MODE_ANY; GC_TRIGGERS; } CONTRACTL_END; + STRINGREF* refptr = EmptyStringRefPtr; //If we've never gotten a reference to the EmptyString, we need to go get one. - if (refptr==NULL) { + if (refptr == nullptr) + { refptr = InitEmptyStringRefPtr(); } + + if (EmptyStringIsFrozen && pinnedString != nullptr) + { + *pinnedString = *(void**)refptr; + } + //We've already have a reference to the EmptyString, so we can just return it. return refptr; } From 5c2a2220f7756983f30bd4a232dd12acb27c1f72 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 5 Sep 2022 16:51:32 +0200 Subject: [PATCH 44/57] Apply suggestions from code review Co-authored-by: Jan Kotas --- src/coreclr/vm/frozenobjectheap.cpp | 2 ++ src/coreclr/vm/frozenobjectheap.h | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 74da0778f48ad..18804470b516b 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#include "common.h" + #include "frozenobjectheap.h" #include "memorypool.h" diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 3247b1b5bf663..e9a89644b868e 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -4,7 +4,6 @@ #ifndef _FROZENOBJECTHEAP_H #define _FROZENOBJECTHEAP_H -#include "common.h" #include "gcinterface.h" #include From 19d1734b5f21e7867f868057c0b5c9e65cd065b3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Sep 2022 17:21:07 +0200 Subject: [PATCH 45/57] Address feedback --- src/coreclr/vm/frozenobjectheap.cpp | 3 --- src/coreclr/vm/stringliteralmap.cpp | 33 ++++++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 18804470b516b..2e1e4a950151c 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -2,10 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. #include "common.h" - #include "frozenobjectheap.h" -#include "memorypool.h" - FrozenObjectHeapManager::FrozenObjectHeapManager(): m_CurrentHeap(nullptr) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 5a5e545ef2e05..dbaadb5fa0a77 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -497,18 +497,18 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri bool isFrozen = false; STRINGREF strObj = AllocateStringObject(pStringData, preferFrozenObjHeap, &isFrozen); - GCPROTECT_BEGIN(strObj) + if (isFrozen) { - if (isFrozen) - { - _ASSERT(preferFrozenObjHeap); - StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateFrozenEntry(pStringData, strObj)); - m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); - pEntry.SuppressRelease(); - pRet = pEntry; - _ASSERT(pRet->IsStringFrozen()); - } - else + _ASSERT(preferFrozenObjHeap); + StringLiteralEntryHolder pEntry(StringLiteralEntry::AllocateFrozenEntry(pStringData, strObj)); + m_StringToEntryHashTable->InsertValue(pStringData, pEntry, FALSE); + pEntry.SuppressRelease(); + pRet = pEntry; + _ASSERT(pRet->IsStringFrozen()); + } + else + { + GCPROTECT_BEGIN(strObj) { // If it's not frozen we need to gc allocate a pinned handle to the non-pinned string object PinnedHeapHandleBlockHolder pStrObj(&m_PinnedHeapHandleTable,1); @@ -524,8 +524,8 @@ StringLiteralEntry *GlobalStringLiteralMap::AddStringLiteral(EEStringData *pStri pEntry.SuppressRelease(); pRet = pEntry; } + GCPROTECT_END(); } - GCPROTECT_END(); #ifdef LOGGING LogStringLiteral("added", pStringData); @@ -585,9 +585,12 @@ void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry } #endif - // Release the object handle that the entry was using. - STRINGREF *pObjRef = pEntry->GetStringObject(); - m_PinnedHeapHandleTable.ReleaseHandles((OBJECTREF*)pObjRef, 1); + if (pEntry->IsStringFrozen()) + { + // Release the object handle that the entry was using. + STRINGREF *pObjRef = pEntry->GetStringObject(); + m_PinnedHeapHandleTable.ReleaseHandles((OBJECTREF*)pObjRef, 1); + } } // We do not delete the StringLiteralEntry itself that will be done in the From 4762bcca396cb34b2b6fb3ddce4bb9991f2ea69e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Sep 2022 17:26:41 +0200 Subject: [PATCH 46/57] Address feedback --- src/coreclr/vm/stringliteralmap.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index dbaadb5fa0a77..1da1ef5361c74 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -652,13 +652,20 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) } CONTRACTL_END; - _ASSERTE (pEntry->GetRefCount() == 0); - _ASSERTE (!pEntry->IsStringFrozen()); - + if (pEntry->IsStringFrozen()) + { +#ifdef _DEBUG + pEntry->m_bDeleted = TRUE; +#endif + } + else + { + _ASSERTE (pEntry->GetRefCount() == 0); #ifdef _DEBUG - memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); - pEntry->m_bDeleted = TRUE; + memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); + pEntry->m_bDeleted = TRUE; #endif + } // The free list needs protection from the m_HashTableCrstGlobal pEntry->m_pNext = s_FreeEntryList; From 2e03cc28f58d551530409953fd790ec54c9f4f7d Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 5 Sep 2022 17:34:59 +0200 Subject: [PATCH 47/57] Update src/coreclr/vm/stringliteralmap.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/stringliteralmap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index 1da1ef5361c74..af294c5110235 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -585,7 +585,7 @@ void GlobalStringLiteralMap::RemoveStringLiteralEntry(StringLiteralEntry *pEntry } #endif - if (pEntry->IsStringFrozen()) + if (!pEntry->IsStringFrozen()) { // Release the object handle that the entry was using. STRINGREF *pObjRef = pEntry->GetStringObject(); From 2f00f717a20d2df1c74aed69807ef4f601c32208 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 5 Sep 2022 17:35:28 +0200 Subject: [PATCH 48/57] Update src/coreclr/vm/stringliteralmap.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/stringliteralmap.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index af294c5110235..b8a485b4f915f 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -655,6 +655,7 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) if (pEntry->IsStringFrozen()) { #ifdef _DEBUG + memset (&pEntry->m_FrozenStringObj, 0xc, sizeof(pEntry->m_FrozenStringObj)); pEntry->m_bDeleted = TRUE; #endif } From ddfbf73e3570d8944d0279ba0c9c5c695464e2f1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Sep 2022 18:07:42 +0200 Subject: [PATCH 49/57] fix compilation errors --- src/coreclr/vm/stringliteralmap.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/stringliteralmap.cpp b/src/coreclr/vm/stringliteralmap.cpp index b8a485b4f915f..4cca97a22b523 100644 --- a/src/coreclr/vm/stringliteralmap.cpp +++ b/src/coreclr/vm/stringliteralmap.cpp @@ -652,21 +652,11 @@ void StringLiteralEntry::DeleteEntry (StringLiteralEntry *pEntry) } CONTRACTL_END; - if (pEntry->IsStringFrozen()) - { + _ASSERTE (pEntry->IsStringFrozen() || pEntry->GetRefCount() == 0); #ifdef _DEBUG - memset (&pEntry->m_FrozenStringObj, 0xc, sizeof(pEntry->m_FrozenStringObj)); - pEntry->m_bDeleted = TRUE; + memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); + pEntry->m_bDeleted = TRUE; #endif - } - else - { - _ASSERTE (pEntry->GetRefCount() == 0); -#ifdef _DEBUG - memset (&pEntry->m_pStringObj, 0xc, sizeof(pEntry->m_pStringObj)); - pEntry->m_bDeleted = TRUE; -#endif - } // The free list needs protection from the m_HashTableCrstGlobal pEntry->m_pNext = s_FreeEntryList; From 802106e522fd6325bc7e0bd27eafa7a0e76666bf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 7 Sep 2022 13:02:20 +0200 Subject: [PATCH 50/57] Address Jan's feedback --- src/coreclr/vm/frozenobjectheap.cpp | 27 ++++++++++++++++----------- src/coreclr/vm/frozenobjectheap.h | 4 ++-- src/coreclr/vm/gchelpers.cpp | 2 +- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 2e1e4a950151c..c307326257693 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -17,7 +17,7 @@ FrozenObjectHeapManager::FrozenObjectHeapManager(): // 1) DOTNET_FrozenSegmentReserveSize is 0 (disabled) // 2) Object is too large (large than DOTNET_FrozenSegmentCommitSize) // in such cases caller is responsible to find a more appropriate heap to allocate it -Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t objectSize) +Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t objectSize) { CONTRACTL { @@ -27,8 +27,9 @@ Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t obj CONTRACTL_END #ifndef FEATURE_BASICFREEZE + // GC is required to support frozen segments return nullptr; -#endif +#else // FEATURE_BASICFREEZE CrstHolder ch(&m_Crst); @@ -61,9 +62,9 @@ Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t obj _ASSERT(m_CurrentHeap != nullptr); } - Object* obj = m_CurrentHeap->AllocateObject(type, objectSize); + Object* obj = m_CurrentHeap->TryAllocateObject(type, objectSize); - // The only case where it might be null is when the current heap is full and we need + // The only case where it can be null is when the current heap is full and we need // to create a new one if (obj == nullptr) { @@ -71,12 +72,13 @@ Object* FrozenObjectHeapManager::AllocateObject(PTR_MethodTable type, size_t obj m_FrozenHeaps.Append(m_CurrentHeap); // Try again - obj = m_CurrentHeap->AllocateObject(type, objectSize); + obj = m_CurrentHeap->TryAllocateObject(type, objectSize); // This time it's not expected to be null _ASSERT(obj != nullptr); } return obj; +#endif // !FEATURE_BASICFREEZE } @@ -98,18 +100,19 @@ FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): } // Commit a chunk in advance - alloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); - if (alloc == nullptr) + void* committedAlloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); + if (committedAlloc == nullptr) { + ClrVirtualFree(alloc, 0, MEM_RELEASE); ThrowOutOfMemory(); } // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect // DATA_ALIGNMENT alignment as well - _ASSERT(IS_ALIGNED(alloc, DATA_ALIGNMENT)); + _ASSERT(IS_ALIGNED(committedAlloc, DATA_ALIGNMENT)); segment_info si; - si.pvMem = alloc; + si.pvMem = committedAlloc; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = si.ibFirstObject; si.ibCommit = m_CommitChunkSize; @@ -118,17 +121,18 @@ FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle == nullptr) { + ClrVirtualFree(alloc, 0, MEM_RELEASE); ThrowOutOfMemory(); } - m_pStart = static_cast(alloc); + m_pStart = static_cast(committedAlloc); m_pCurrent = m_pStart; m_SizeCommitted = si.ibCommit; INDEBUG(m_ObjectsCount = 0); return; } -Object* FrozenObjectHeap::AllocateObject(PTR_MethodTable type, size_t objectSize) +Object* FrozenObjectHeap::TryAllocateObject(PTR_MethodTable type, size_t objectSize) { _ASSERT(m_pStart != nullptr && m_SizeReserved > 0 && m_SegmentHandle != nullptr); // Expected to be inited _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); @@ -146,6 +150,7 @@ Object* FrozenObjectHeap::AllocateObject(PTR_MethodTable type, size_t objectSize _ASSERT(m_SizeCommitted + m_CommitChunkSize <= m_SizeReserved); if (ClrVirtualAlloc(m_pStart + m_SizeCommitted, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) { + ClrVirtualFree(m_pStart, 0, MEM_RELEASE); ThrowOutOfMemory(); } m_SizeCommitted += m_CommitChunkSize; diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index e9a89644b868e..8e3078b63cb61 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -13,7 +13,7 @@ class FrozenObjectHeapManager { public: FrozenObjectHeapManager(); - Object* AllocateObject(PTR_MethodTable type, size_t objectSize); + Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); private: CrstExplicitInit m_Crst; @@ -27,7 +27,7 @@ class FrozenObjectHeap { public: FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize); - Object* AllocateObject(PTR_MethodTable type, size_t objectSize); + Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); private: uint8_t* m_pStart; diff --git a/src/coreclr/vm/gchelpers.cpp b/src/coreclr/vm/gchelpers.cpp index 25a2194b01d9e..e09f4c9345cd6 100644 --- a/src/coreclr/vm/gchelpers.cpp +++ b/src/coreclr/vm/gchelpers.cpp @@ -886,7 +886,7 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIs if (preferFrozenHeap) { FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager(); - orString = static_cast(foh->AllocateObject(g_pStringClass, totalSize)); + orString = static_cast(foh->TryAllocateObject(g_pStringClass, totalSize)); if (orString != nullptr) { orString->SetStringLength(cchStringLength); From bfe96e7fdee59fa8a528d45e8b6e8d7b544fb4ad Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 8 Sep 2022 01:34:53 +0200 Subject: [PATCH 51/57] Update src/coreclr/jit/gentree.cpp Co-authored-by: Andy Ayers --- src/coreclr/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 7f001c8db5f1d..13106f68df9ed 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3838,7 +3838,7 @@ bool Compiler::gtCanSwapOrder(GenTree* firstNode, GenTree* secondNode) bool canSwap = true; // Don't swap "CONST_HDL op CNS" - if (firstNode->IsIntegralConst() && secondNode->IsIntegralConst() && varTypeIsGC(firstNode) && + if (firstNode->IsIconHandle() && secondNode->IsIntegralConst() && !varTypeIsGC(secondNode)) { canSwap = false; From 2e10c9c6ae8907b28486c6b31f1f2bc3ea15b149 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 8 Sep 2022 01:36:37 +0200 Subject: [PATCH 52/57] Update gentree.cpp --- src/coreclr/jit/gentree.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 13106f68df9ed..19e84db4dc9d0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3838,8 +3838,7 @@ bool Compiler::gtCanSwapOrder(GenTree* firstNode, GenTree* secondNode) bool canSwap = true; // Don't swap "CONST_HDL op CNS" - if (firstNode->IsIconHandle() && secondNode->IsIntegralConst() && - !varTypeIsGC(secondNode)) + if (firstNode->IsIconHandle() && secondNode->IsIntegralConst()) { canSwap = false; } From bac0389da6313b7804723f588c3d45bc54ece876 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 05:39:07 +0200 Subject: [PATCH 53/57] Address Maoni's feedback: 1) Rename FrozenObjectHeap to FrozenObjectSegment since it represents a single segment 2) Remove unnecessary config knobs and leave only turn off/on one --- src/coreclr/inc/clrconfigvalues.h | 3 +- src/coreclr/vm/frozenobjectheap.cpp | 72 ++++++++++++++--------------- src/coreclr/vm/frozenobjectheap.h | 29 ++++++++---- 3 files changed, 57 insertions(+), 47 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index caef86369fa9f..a600138a65dc2 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -408,8 +408,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_BreakOnOutOfMemoryWithinRange, W("BreakOnOutOf /// /// Frozen segments (aka Frozen Object Heap) /// -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentReserveSize, W("FrozenSegmentReserveSize"), 0x400000, "Amount of memory to reserve for a frozen segment") // 4Mb -RETAIL_CONFIG_DWORD_INFO(INTERNAL_FrozenSegmentCommitSize, W("FrozenSegmentCommitSize"), 0x10000, "Amount of memory to commit on demand for frozen segments") // 64Kb +RETAIL_CONFIG_DWORD_INFO(INTERNAL_UseFrozenObjectHeap, W("UseFrozenObjectHeap"), 1, "Use frozen object heap for certain types of objects (e.g. string literals) as an optimization.") /// /// Log diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index c307326257693..fa02d1128c9b5 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -4,18 +4,22 @@ #include "common.h" #include "frozenobjectheap.h" +// Size to reserve for a frozen segment +#define FOH_SEGMENT_SIZE (4 * 1024 * 1024) +// Size to commit on demand in that reserved space +#define FOH_COMMIT_SIZE (64 * 1024) + FrozenObjectHeapManager::FrozenObjectHeapManager(): - m_CurrentHeap(nullptr) + m_CurrentSegment(nullptr) { m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); - m_HeapCommitChunkSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentCommitSize); - m_HeapSize = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_FrozenSegmentReserveSize); + m_Enabled = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_UseFrozenObjectHeap) != 0; } // Allocates an object of the give size (including header) on a frozen segment. // May return nullptr in the following cases: -// 1) DOTNET_FrozenSegmentReserveSize is 0 (disabled) -// 2) Object is too large (large than DOTNET_FrozenSegmentCommitSize) +// 1) DOTNET_UseFrozenObjectHeap is 0 (disabled) +// 2) Object is too large (large 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) { @@ -33,46 +37,46 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t CrstHolder ch(&m_Crst); - // Quick way to disable Frozen Heaps - if (m_HeapSize == 0) + if (!m_Enabled) { + // Disabled via DOTNET_UseFrozenObjectHeap=0 return nullptr; } _ASSERT(type != nullptr); - _ASSERT(m_HeapCommitChunkSize >= MIN_OBJECT_SIZE); - _ASSERT(m_HeapSize > m_HeapCommitChunkSize); - _ASSERT(m_HeapSize % m_HeapCommitChunkSize == 0); + _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE); + _ASSERT(FOH_SEGMENT_SIZE > FOH_COMMIT_SIZE); + _ASSERT(FOH_SEGMENT_SIZE % FOH_COMMIT_SIZE == 0); // NOTE: objectSize is expected be the full size including header _ASSERT(objectSize >= MIN_OBJECT_SIZE); - if (objectSize > m_HeapCommitChunkSize) + if (objectSize > FOH_COMMIT_SIZE) { - // The current design doesn't allow object larger than DOTNET_FrozenSegmentCommitSize and + // 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; } - if (m_CurrentHeap == nullptr) + if (m_CurrentSegment == nullptr) { // Create the first heap on first allocation - m_CurrentHeap = new FrozenObjectHeap(m_HeapSize, m_HeapCommitChunkSize); - m_FrozenHeaps.Append(m_CurrentHeap); - _ASSERT(m_CurrentHeap != nullptr); + m_CurrentSegment = new FrozenObjectSegment(); + m_FrozenSegments.Append(m_CurrentSegment); + _ASSERT(m_CurrentSegment != nullptr); } - Object* obj = m_CurrentHeap->TryAllocateObject(type, objectSize); + Object* obj = m_CurrentSegment->TryAllocateObject(type, objectSize); // The only case where it can be null is when the current heap is full and we need // to create a new one if (obj == nullptr) { - m_CurrentHeap = new FrozenObjectHeap(m_HeapSize, m_HeapCommitChunkSize); - m_FrozenHeaps.Append(m_CurrentHeap); + m_CurrentSegment = new FrozenObjectSegment(); + m_FrozenSegments.Append(m_CurrentSegment); // Try again - obj = m_CurrentHeap->TryAllocateObject(type, objectSize); + obj = m_CurrentSegment->TryAllocateObject(type, objectSize); // This time it's not expected to be null _ASSERT(obj != nullptr); @@ -82,25 +86,21 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t } -FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): +FrozenObjectSegment::FrozenObjectSegment(): m_pStart(nullptr), m_pCurrent(nullptr), - m_CommitChunkSize(0), m_SizeCommitted(0), m_SegmentHandle(nullptr) COMMA_INDEBUG(m_ObjectsCount(0)) { - m_SizeReserved = reserveSize; - m_CommitChunkSize = commitChunkSize; - - void* alloc = ClrVirtualAlloc(nullptr, m_SizeReserved, MEM_RESERVE, PAGE_READWRITE); + void* alloc = ClrVirtualAlloc(nullptr, FOH_SEGMENT_SIZE, MEM_RESERVE, PAGE_READWRITE); if (alloc == nullptr) { ThrowOutOfMemory(); } // Commit a chunk in advance - void* committedAlloc = ClrVirtualAlloc(alloc, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE); + void* committedAlloc = ClrVirtualAlloc(alloc, FOH_COMMIT_SIZE, MEM_COMMIT, PAGE_READWRITE); if (committedAlloc == nullptr) { ClrVirtualFree(alloc, 0, MEM_RELEASE); @@ -115,8 +115,8 @@ FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): si.pvMem = committedAlloc; si.ibFirstObject = sizeof(ObjHeader); si.ibAllocated = si.ibFirstObject; - si.ibCommit = m_CommitChunkSize; - si.ibReserved = m_SizeReserved; + si.ibCommit = FOH_COMMIT_SIZE; + si.ibReserved = FOH_SEGMENT_SIZE; m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si); if (m_SegmentHandle == nullptr) @@ -132,28 +132,28 @@ FrozenObjectHeap::FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize): return; } -Object* FrozenObjectHeap::TryAllocateObject(PTR_MethodTable type, size_t objectSize) +Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t objectSize) { - _ASSERT(m_pStart != nullptr && m_SizeReserved > 0 && m_SegmentHandle != nullptr); // Expected to be inited + _ASSERT(m_pStart != nullptr && FOH_SEGMENT_SIZE > 0 && m_SegmentHandle != nullptr); // Expected to be inited _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT)); uint8_t* obj = m_pCurrent; - if (reinterpret_cast(m_pStart + m_SizeReserved) < reinterpret_cast(obj + objectSize)) + if (reinterpret_cast(m_pStart + FOH_SEGMENT_SIZE) < reinterpret_cast(obj + objectSize)) { - // heap is full + // Segment is full return nullptr; } // Check if we need to commit a new chunk if (reinterpret_cast(m_pStart + m_SizeCommitted) < reinterpret_cast(obj + objectSize)) { - _ASSERT(m_SizeCommitted + m_CommitChunkSize <= m_SizeReserved); - if (ClrVirtualAlloc(m_pStart + m_SizeCommitted, m_CommitChunkSize, MEM_COMMIT, PAGE_READWRITE) == nullptr) + _ASSERT(m_SizeCommitted + FOH_COMMIT_SIZE <= FOH_SEGMENT_SIZE); + if (ClrVirtualAlloc(m_pStart + m_SizeCommitted, FOH_COMMIT_SIZE, MEM_COMMIT, PAGE_READWRITE) == nullptr) { ClrVirtualFree(m_pStart, 0, MEM_RELEASE); ThrowOutOfMemory(); } - m_SizeCommitted += m_CommitChunkSize; + m_SizeCommitted += FOH_COMMIT_SIZE; } INDEBUG(m_ObjectsCount++); diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 8e3078b63cb61..9987584d53097 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -7,7 +7,21 @@ #include "gcinterface.h" #include -class FrozenObjectHeap; +// FrozenObjectHeapManager provides a simple API to allocate objects on GC's Frozen Segments, it can be used as +// an optimization to put certain types of objects there and rely on them to be effectively pinned so, for instance, +// jit can bake direct addresses of them in codegen and avoid extra indirect loads. +// +// Example: a string literal allocated on a normal heap looks like this in JIT for x64: +// +// mov rax, 0xD1FFAB1E ; pinned handle +// mov rax, gword ptr [rax] ; actual string object +// +// and here is the same literal but allocated on a frozen segment: +// +// mov rax, 0xD1FFAB1E ; actual string object +// + +class FrozenObjectSegment; class FrozenObjectHeapManager { @@ -17,24 +31,21 @@ class FrozenObjectHeapManager private: CrstExplicitInit m_Crst; - SArray m_FrozenHeaps; - FrozenObjectHeap* m_CurrentHeap; - size_t m_HeapCommitChunkSize; - size_t m_HeapSize; + SArray m_FrozenSegments; + FrozenObjectSegment* m_CurrentSegment; + bool m_Enabled; }; -class FrozenObjectHeap +class FrozenObjectSegment { public: - FrozenObjectHeap(size_t reserveSize, size_t commitChunkSize); + FrozenObjectSegment(); Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); private: uint8_t* m_pStart; uint8_t* m_pCurrent; - size_t m_CommitChunkSize; size_t m_SizeCommitted; - size_t m_SizeReserved; segment_handle m_SegmentHandle; INDEBUG(size_t m_ObjectsCount); }; From 01b57310369e6dc5591d5d1ba58f581b4a1d411b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 05:43:20 +0200 Subject: [PATCH 54/57] heap -> segment in comments --- 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 fa02d1128c9b5..7272a2d5e7dc3 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -60,7 +60,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t if (m_CurrentSegment == nullptr) { - // Create the first heap on first allocation + // Create the first segment on first allocation m_CurrentSegment = new FrozenObjectSegment(); m_FrozenSegments.Append(m_CurrentSegment); _ASSERT(m_CurrentSegment != nullptr); @@ -68,7 +68,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t Object* obj = m_CurrentSegment->TryAllocateObject(type, objectSize); - // The only case where it can be null is when the current heap is full and we need + // The only case where it can be null is when the current segment is full and we need // to create a new one if (obj == nullptr) { From 8d139c9e5ab25d616208eb9b3efc9ea15e8deb4b Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 13 Sep 2022 05:58:48 +0200 Subject: [PATCH 55/57] Update src/coreclr/vm/frozenobjectheap.h Co-authored-by: Jan Kotas --- src/coreclr/vm/frozenobjectheap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frozenobjectheap.h b/src/coreclr/vm/frozenobjectheap.h index 9987584d53097..703c3aaa6fc45 100644 --- a/src/coreclr/vm/frozenobjectheap.h +++ b/src/coreclr/vm/frozenobjectheap.h @@ -30,7 +30,7 @@ class FrozenObjectHeapManager Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize); private: - CrstExplicitInit m_Crst; + Crst m_Crst; SArray m_FrozenSegments; FrozenObjectSegment* m_CurrentSegment; bool m_Enabled; From b6deb3c8b30a52e3eaa6306b040fe4952888b9ec Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 06:01:17 +0200 Subject: [PATCH 56/57] Address Jan's feedback around m_Crst creation --- src/coreclr/vm/frozenobjectheap.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/frozenobjectheap.cpp b/src/coreclr/vm/frozenobjectheap.cpp index 7272a2d5e7dc3..292346db76564 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -10,10 +10,10 @@ #define FOH_COMMIT_SIZE (64 * 1024) FrozenObjectHeapManager::FrozenObjectHeapManager(): - m_CurrentSegment(nullptr) + m_CurrentSegment(nullptr), + m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), + m_Enabled(CLRConfig::GetConfigValue(CLRConfig::INTERNAL_UseFrozenObjectHeap) != 0) { - m_Crst.Init(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC); - m_Enabled = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_UseFrozenObjectHeap) != 0; } // Allocates an object of the give size (including header) on a frozen segment. From 1ba9d976e586ae6fe3185bb6f22acf77b55f55fb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 06:04:02 +0200 Subject: [PATCH 57/57] Fix order of initialization --- 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 292346db76564..cedade43fa125 100644 --- a/src/coreclr/vm/frozenobjectheap.cpp +++ b/src/coreclr/vm/frozenobjectheap.cpp @@ -10,8 +10,8 @@ #define FOH_COMMIT_SIZE (64 * 1024) FrozenObjectHeapManager::FrozenObjectHeapManager(): - m_CurrentSegment(nullptr), m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC), + m_CurrentSegment(nullptr), m_Enabled(CLRConfig::GetConfigValue(CLRConfig::INTERNAL_UseFrozenObjectHeap) != 0) { }