From 8ae6508d4537afec3a201eb2ff77b050c709666c Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 9 Jan 2024 09:56:16 -0800 Subject: [PATCH 1/3] Remove the concept of packable fields from EEClass - EEClass has long had a concept of packable rarely accessed fields - When we had NGEN we had logic to make these fields take very small amounts of space, but now that we no longer have NGEN, this code is not actually used in practice - The unpacked form of these fields used 4 bytes per field, even though many of them were logically 2 byte fields - The NumBoxed fields were just unnecessary, especially if we have a HasBoxedThreadStatics flag, which this change adds - Move the HasBoxed flags to the WFLAGS_LOW_ENUM Overall this saves 24 bytes per EEClass and moves all these fields onto a normal structure. --- src/coreclr/vm/array.cpp | 2 +- src/coreclr/vm/class.cpp | 70 +----- src/coreclr/vm/class.h | 154 +++--------- src/coreclr/vm/methodtable.cpp | 16 -- src/coreclr/vm/methodtable.h | 25 +- src/coreclr/vm/methodtablebuilder.cpp | 20 +- src/coreclr/vm/methodtablebuilder.h | 2 - src/coreclr/vm/packedfields.inl | 342 -------------------------- src/coreclr/vm/threadstatics.cpp | 4 +- 9 files changed, 65 insertions(+), 570 deletions(-) delete mode 100644 src/coreclr/vm/packedfields.inl diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index 46ea5958f6151..c19c9cb9f311f 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -324,7 +324,7 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy // Allocate ArrayClass (including space for packed fields), MethodTable, and class name in one alloc. // Remember to pad allocation size for ArrayClass portion to ensure MethodTable is pointer aligned. - cbArrayClass = ALIGN_UP(sizeof(ArrayClass) + sizeof(EEClassPackedFields), sizeof(void*)); + cbArrayClass = ALIGN_UP(sizeof(ArrayClass), sizeof(void*)); } // ArrayClass already includes one void* diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 0062632396c95..abc3ac561f3e4 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -30,20 +30,6 @@ #ifndef DACCESS_COMPILE -//******************************************************************************* -EEClass::EEClass(DWORD cbFixedEEClassFields) -{ - LIMITED_METHOD_CONTRACT; - - // Cache size of fixed fields (this instance also contains a set of packed fields whose final size isn't - // determined until the end of class loading). We store the size into a spare byte made available by - // compiler field alignment, so we need to ensure we never allocate a flavor of EEClass more than 255 - // bytes long. - _ASSERTE(cbFixedEEClassFields <= 0xff); - m_cbFixedEEClassFields = (BYTE)cbFixedEEClassFields; - - // All other members are initialized to zero -} //******************************************************************************* void *EEClass::operator new( @@ -59,12 +45,7 @@ void *EEClass::operator new( } CONTRACTL_END; - // EEClass (or sub-type) is always followed immediately by an EEClassPackedFields structure. This is - // maximally sized at runtime but in the ngen scenario will be optimized into a smaller structure (which - // is why it must go after all the fixed sized fields). - S_SIZE_T safeSize = S_SIZE_T(size) + S_SIZE_T(sizeof(EEClassPackedFields)); - - void *p = pamTracker->Track(pHeap->AllocMem(safeSize)); + void *p = pamTracker->Track(pHeap->AllocMem(S_SIZE_T(size))); // No need to memset since this memory came from VirtualAlloc'ed memory // memset (p, 0, size); @@ -209,7 +190,7 @@ EEClass::CreateMinimalClass(LoaderHeap *pHeap, AllocMemTracker *pamTracker) } CONTRACTL_END; - return new (pHeap, pamTracker) EEClass(sizeof(EEClass)); + return new (pHeap, pamTracker) EEClass(); } @@ -3282,11 +3263,6 @@ EEClass::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, MethodTable * pMT) DAC_ENUM_DTHIS(); EMEM_OUT(("MEM: %p EEClass\n", dac_cast(this))); - // The DAC_ENUM_DTHIS above won't have reported the packed fields tacked on the end of this instance (they - // aren't part of the static class definition because the fields are variably sized and thus have to come - // right at the end of the structure, even for sub-types such as LayoutEEClass or DelegateEEClass). - DacEnumMemoryRegion(dac_cast(GetPackedFields()), sizeof(EEClassPackedFields)); - if (HasOptionalFields()) DacEnumMemoryRegion(dac_cast(GetOptionalFields()), sizeof(EEClassOptionalFields)); @@ -3319,45 +3295,3 @@ EEClass::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, MethodTable * pMT) #endif // DACCESS_COMPILE -// Get pointer to the packed fields structure attached to this instance. -PTR_EEClassPackedFields EEClass::GetPackedFields() -{ - LIMITED_METHOD_DAC_CONTRACT; - - return dac_cast(PTR_HOST_TO_TADDR(this) + m_cbFixedEEClassFields); -} - -// Get the value of the given field. Works regardless of whether the field is currently in its packed or -// unpacked state. -DWORD EEClass::GetPackableField(EEClassFieldId eField) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - SUPPORTS_DAC; - } - CONTRACTL_END; - - return m_fFieldsArePacked ? - GetPackedFields()->GetPackedField(eField) : - GetPackedFields()->GetUnpackedField(eField); -} - -// Set the value of the given field. The field *must* be in the unpacked state for this to be legal (in -// practice all packable fields must be initialized during class construction and from then on remain -// immutable). -void EEClass::SetPackableField(EEClassFieldId eField, DWORD dwValue) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - } - CONTRACTL_END; - - _ASSERTE(!m_fFieldsArePacked); - GetPackedFields()->SetUnpackedField(eField, dwValue); -} diff --git a/src/coreclr/vm/class.h b/src/coreclr/vm/class.h index 8d5785dd48a64..75003bae383d5 100644 --- a/src/coreclr/vm/class.h +++ b/src/coreclr/vm/class.h @@ -14,7 +14,7 @@ // // Given that the data itself is touched infrequently, we can trade off space reduction against cpu-usage to // good effect here. A fair amount of work has gone into reducing the size of each EEClass instance (see -// EEClassOptionalFields and EEClassPackedFields) at the expense of somewhat more convoluted runtime access. +// EEClassOptionalFields) at the expense of somewhat more convoluted runtime access. // // Please consider this (and measure the impact of your changes against startup scenarios) before adding // fields to EEClass or otherwise increasing its size. @@ -46,7 +46,6 @@ #include "typectxt.h" #include "iterator_util.h" -#include "packedfields.inl" #include "array.h" VOID DECLSPEC_NORETURN RealCOMPlusThrowHR(HRESULT hr); @@ -617,49 +616,6 @@ class EEClassOptionalFields } }; typedef DPTR(EEClassOptionalFields) PTR_EEClassOptionalFields; - -// -// Another mechanism used to reduce the size of the average EEClass instance is the notion of packed fields. -// This is based on the observation that EEClass has a large number of integer fields that typically contain -// small values and that are fixed once class layout has completed. We can compact these fields by discarding -// the leading zero bits (and for small values there'll be a lot of these) and packing the significant data -// into compact bitfields. This is a dynamic operation (the exact packing used depends on the exact data -// stored in the fields). -// -// The PackedDWORDFields<> class (defined in PackedFields.inl) encapsulates this. It takes one template -// parameter, the number of fields to pack, and provides operations to get and set those fields until we're -// happy with the values, at which point it will compact them for us. -// -// The packed fields themselves are stored at the end of the EEClass instance (or the LayoutEEClass or the -// DelegateEEClass etc.) so we can take advantage of the variable sized nature of the fields. We gain nothing for -// runtime allocated EEClasses (we have to allocate a maximally sized structure for the packed fields because -// we can't tell at the beginning of EEClass layout what the field values will be). But in the ngen scenario -// we can compact the fields just prior to saving and only store the portion of the EEClass that is relvant, -// helping us with our goal of packing all the EEClass instances together as tightly as possible. -// -// Since each packed field is now accessed via an array-like index, we give each of those indices a name with -// the enum below to make the code more readable. -// - -enum EEClassFieldId -{ - EEClass_Field_NumInstanceFields = 0, - EEClass_Field_NumMethods, - EEClass_Field_NumStaticFields, - EEClass_Field_NumHandleStatics, - EEClass_Field_NumBoxedStatics, - EEClass_Field_NonGCStaticFieldBytes, - EEClass_Field_NumThreadStaticFields, - EEClass_Field_NumHandleThreadStatics, - EEClass_Field_NumBoxedThreadStatics, - EEClass_Field_NonGCThreadStaticFieldBytes, - EEClass_Field_NumNonVirtualSlots, - EEClass_Field_COUNT -}; - -typedef PackedDWORDFields EEClassPackedFields; -typedef DPTR(EEClassPackedFields) PTR_EEClassPackedFields; - //@GENERICS: // For most types there is a one-to-one mapping between MethodTable* and EEClass* // However this is not the case for instantiated types where code and representation @@ -911,13 +867,13 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! { LIMITED_METHOD_CONTRACT; SUPPORTS_DAC; - return (WORD)GetPackableField(EEClass_Field_NumInstanceFields); + return m_NumInstanceFields; } inline void SetNumInstanceFields (WORD wNumInstanceFields) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumInstanceFields, wNumInstanceFields); + m_NumInstanceFields = wNumInstanceFields; } /* @@ -928,25 +884,25 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! { LIMITED_METHOD_CONTRACT; SUPPORTS_DAC; - return (WORD)GetPackableField(EEClass_Field_NumStaticFields); + return m_NumStaticFields; } inline void SetNumStaticFields (WORD wNumStaticFields) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumStaticFields, wNumStaticFields); + m_NumStaticFields = wNumStaticFields; } inline WORD GetNumThreadStaticFields() { LIMITED_METHOD_CONTRACT; SUPPORTS_DAC; - return (WORD)GetPackableField(EEClass_Field_NumThreadStaticFields); + return m_NumThreadStaticFields; } inline void SetNumThreadStaticFields (WORD wNumThreadStaticFields) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumThreadStaticFields, wNumThreadStaticFields); + m_NumThreadStaticFields = wNumThreadStaticFields; } // Statics are stored in a big chunk inside the module @@ -973,34 +929,34 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! inline DWORD GetNonGCRegularStaticFieldBytes() { LIMITED_METHOD_CONTRACT; - return GetPackableField(EEClass_Field_NonGCStaticFieldBytes); + return m_NonGCStaticFieldBytes; } inline void SetNonGCRegularStaticFieldBytes (DWORD cbNonGCStaticFieldBytes) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NonGCStaticFieldBytes, cbNonGCStaticFieldBytes); + m_NonGCStaticFieldBytes = cbNonGCStaticFieldBytes; } inline DWORD GetNonGCThreadStaticFieldBytes() { LIMITED_METHOD_CONTRACT; - return GetPackableField(EEClass_Field_NonGCThreadStaticFieldBytes); + return m_NonGCThreadStaticFieldBytes; } inline void SetNonGCThreadStaticFieldBytes (DWORD cbNonGCStaticFieldBytes) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NonGCThreadStaticFieldBytes, cbNonGCStaticFieldBytes); + m_NonGCThreadStaticFieldBytes = cbNonGCStaticFieldBytes; } inline WORD GetNumNonVirtualSlots() { LIMITED_METHOD_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumNonVirtualSlots); + return m_NumNonVirtualSlots; } inline void SetNumNonVirtualSlots(WORD wNumNonVirtualSlots) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumNonVirtualSlots, wNumNonVirtualSlots); + m_NumNonVirtualSlots = wNumNonVirtualSlots; } inline BOOL IsEquivalentType() @@ -1023,12 +979,12 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! inline WORD GetNumHandleRegularStatics () { LIMITED_METHOD_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumHandleStatics); + return m_NumHandleStatics; } inline void SetNumHandleRegularStatics (WORD wNumHandleRegularStatics) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumHandleStatics, wNumHandleRegularStatics); + m_NumHandleStatics = wNumHandleRegularStatics; } /* @@ -1037,40 +993,12 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! inline WORD GetNumHandleThreadStatics () { LIMITED_METHOD_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumHandleThreadStatics); + return m_NumHandleThreadStatics; } inline void SetNumHandleThreadStatics (WORD wNumHandleThreadStatics) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumHandleThreadStatics, wNumHandleThreadStatics); - } - - /* - * Number of boxed statics allocated - */ - inline WORD GetNumBoxedRegularStatics () - { - LIMITED_METHOD_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumBoxedStatics); - } - inline void SetNumBoxedRegularStatics (WORD wNumBoxedRegularStatics) - { - LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumBoxedStatics, wNumBoxedRegularStatics); - } - - /* - * Number of boxed statics allocated for ThreadStatics - */ - inline WORD GetNumBoxedThreadStatics () - { - LIMITED_METHOD_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumBoxedThreadStatics); - } - inline void SetNumBoxedThreadStatics (WORD wNumBoxedThreadStatics) - { - LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumBoxedThreadStatics, wNumBoxedThreadStatics); + m_NumHandleThreadStatics = wNumHandleThreadStatics; } /* @@ -1124,12 +1052,12 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! inline WORD GetNumMethods() { LIMITED_METHOD_DAC_CONTRACT; - return (WORD)GetPackableField(EEClass_Field_NumMethods); + return m_NumMethods; } inline void SetNumMethods (WORD wNumMethods) { LIMITED_METHOD_CONTRACT; - SetPackableField(EEClass_Field_NumMethods, wNumMethods); + m_NumMethods = wNumMethods; } /* @@ -1689,9 +1617,6 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! // - Any field that has a default value for the vast majority of EEClass instances // should be stored in the EEClassOptionalFields (see header comment) // - // - Any field that is nearly always a small positive integer and is infrequently - // accessed should be in the EEClassPackedFields (see header comment) - // // If none of these categories apply - such as for always-meaningful pointer members or // sets of flags - a full field is used. Please avoid adding such members if possible. //------------------------------------------------------------- @@ -1825,11 +1750,18 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! // NOTE: Following BYTE fields are laid out together so they'll fit within the same DWORD for efficient // structure packing. BYTE m_NormType; - BYTE m_fFieldsArePacked; // TRUE iff fields pointed to by GetPackedFields() are in packed state - BYTE m_cbFixedEEClassFields; // Count of bytes of normal fields of this instance (EEClass, - // LayoutEEClass etc.). Doesn't count bytes of "packed" fields BYTE m_cbBaseSizePadding; // How many bytes of padding are included in BaseSize + WORD m_NumInstanceFields; + WORD m_NumMethods; + WORD m_NumStaticFields; + WORD m_NumHandleStatics; + WORD m_NumThreadStaticFields; + WORD m_NumHandleThreadStatics; + WORD m_NumNonVirtualSlots; + DWORD m_NonGCStaticFieldBytes; + DWORD m_NonGCThreadStaticFieldBytes; + public: // EEClass optional field support. Whether a particular EEClass instance has optional fields is determined // at class load time. The entire EEClassOptionalFields structure is allocated if the EEClass has need of @@ -1857,23 +1789,6 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! return m_rpOptionalFields; } -private: - // - // Support for packed fields. - // - - // Get pointer to the packed fields structure attached to this instance. - PTR_EEClassPackedFields GetPackedFields(); - - // Get the value of the given field. Works regardless of whether the field is currently in its packed or - // unpacked state. - DWORD GetPackableField(EEClassFieldId eField); - - // Set the value of the given field. The field *must* be in the unpacked state for this to be legal (in - // practice all packable fields must be initialized during class construction and from then on remain - // immutable). - void SetPackableField(EEClassFieldId eField, DWORD dwValue); - //------------------------------------------------------------- // END CONCRETE DATA LAYOUT //------------------------------------------------------------- @@ -1888,7 +1803,10 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! /* * Constructor: prevent any other class from doing a new() */ - EEClass(DWORD cbFixedEEClassFields); + EEClass() + { + LIMITED_METHOD_CONTRACT; + } /* * Destructor: prevent any other class from deleting @@ -1965,7 +1883,7 @@ class LayoutEEClass : public EEClass Volatile m_nativeLayoutInfo; #ifndef DACCESS_COMPILE - LayoutEEClass() : EEClass(sizeof(LayoutEEClass)) + LayoutEEClass() : EEClass() { LIMITED_METHOD_CONTRACT; m_nativeLayoutInfo = NULL; @@ -2013,7 +1931,7 @@ class DelegateEEClass : public EEClass } #ifndef DACCESS_COMPILE - DelegateEEClass() : EEClass(sizeof(DelegateEEClass)) + DelegateEEClass() : EEClass() { LIMITED_METHOD_CONTRACT; // Note: Memory allocated on loader heap is zero filled @@ -2035,7 +1953,7 @@ class ArrayClass : public EEClass friend MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementType arrayKind, unsigned Rank, AllocMemTracker *pamTracker); #ifndef DACCESS_COMPILE - ArrayClass() : EEClass(sizeof(ArrayClass)) { LIMITED_METHOD_CONTRACT; } + ArrayClass() { LIMITED_METHOD_CONTRACT; } #else friend class NativeImageDumper; #endif diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 32fb17a629d80..78e785ab19bcb 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -7326,22 +7326,6 @@ WORD MethodTable::GetNumHandleRegularStatics() return GetClass()->GetNumHandleRegularStatics(); } -//========================================================================================== -WORD MethodTable::GetNumBoxedRegularStatics() -{ - LIMITED_METHOD_CONTRACT; - - return GetClass()->GetNumBoxedRegularStatics(); -} - -//========================================================================================== -WORD MethodTable::GetNumBoxedThreadStatics () -{ - LIMITED_METHOD_CONTRACT; - - return GetClass()->GetNumBoxedThreadStatics(); -} - #ifdef _DEBUG //========================================================================================== // Returns true if pointer to the parent method table has been initialized/restored already. diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 9c06faf6c4323..a77d6a2635b7e 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2296,6 +2296,7 @@ class MethodTable inline void SetHasBoxedRegularStatics() { LIMITED_METHOD_CONTRACT; + _ASSERTE(!HasComponentSize()); SetFlag(enum_flag_HasBoxedRegularStatics); } @@ -2305,6 +2306,19 @@ class MethodTable return GetFlag(enum_flag_HasBoxedRegularStatics); } + inline void SetHasBoxedThreadStatics() + { + LIMITED_METHOD_CONTRACT; + _ASSERTE(!HasComponentSize()); + SetFlag(enum_flag_HasBoxedThreadStatics); + } + + inline DWORD HasBoxedThreadStatics() + { + LIMITED_METHOD_CONTRACT; + return GetFlag(enum_flag_HasBoxedThreadStatics); + } + DWORD HasFixedAddressVTStatics(); // Indicates if the MethodTable only contains abstract methods @@ -2340,9 +2354,6 @@ class MethodTable WORD GetNumHandleRegularStatics(); - WORD GetNumBoxedRegularStatics (); - WORD GetNumBoxedThreadStatics (); - //------------------------------------------------------------------- // DYNAMIC ID // @@ -3329,11 +3340,11 @@ public : enum_flag_IsByRefLike = 0x00001000, - enum_flag_NotInPZM = 0x00002000, // True if this type is not in its PreferredZapModule + enum_flag_HasBoxedRegularStatics = 0x00002000, + enum_flag_HasBoxedThreadStatics = 0x00004000, // In a perfect world we would fill these flags using other flags that we already have // which have a constant value for something which has a component size. - enum_flag_UNUSED_ComponentSize_6 = 0x00004000, enum_flag_UNUSED_ComponentSize_7 = 0x00008000, #define SET_FALSE(flag) ((flag) & 0) @@ -3346,7 +3357,8 @@ public : // case where this MethodTable is for a String or Array enum_flag_StringArrayValues = SET_FALSE(enum_flag_HasCriticalFinalizer) | SET_TRUE(enum_flag_StaticsMask_NonDynamic) | - SET_FALSE(enum_flag_NotInPZM) | + SET_FALSE(enum_flag_HasBoxedRegularStatics) | + SET_FALSE(enum_flag_HasBoxedThreadStatics) | SET_TRUE(enum_flag_GenericsMask_NonGeneric) | SET_FALSE(enum_flag_HasVariance) | SET_FALSE(enum_flag_HasDefaultCtor) | @@ -3461,7 +3473,6 @@ public : enum_flag_RequiresAlign8 = 0x1000, // Type requires 8-byte alignment (only set on platforms that require this and don't get it implicitly) #endif - enum_flag_HasBoxedRegularStatics = 0x2000, // GetNumBoxedRegularStatics() != 0 enum_flag_HasSingleNonVirtualSlot = 0x4000, diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 279aac68723fc..71d4d6a9d2bdc 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -115,7 +115,7 @@ MethodTableBuilder::CreateClass( Module *pModule, } else { - pEEClass = new (pAllocator->GetLowFrequencyHeap(), pamTracker) EEClass(sizeof(EEClass)); + pEEClass = new (pAllocator->GetLowFrequencyHeap(), pamTracker) EEClass(); } DWORD dwAttrClass = 0; @@ -1838,6 +1838,11 @@ MethodTableBuilder::BuildMethodTableThrowing( pMT->SetHasBoxedRegularStatics(); } + if (bmtFP->NumThreadStaticGCBoxedFields != 0) + { + pMT->SetHasBoxedThreadStatics(); + } + if (bmtFP->fIsByRefLikeType) { pMT->SetIsByRefLike(); @@ -7866,12 +7871,6 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() } SetNumHandleRegularStatics(static_cast(dwNumHandleStatics)); - if (!FitsIn(bmtFP->NumRegularStaticGCBoxedFields)) - { // Overflow. - BuildMethodTableThrowException(IDS_EE_TOOMANYFIELDS); - } - SetNumBoxedRegularStatics(static_cast(bmtFP->NumRegularStaticGCBoxedFields)); - // Tell the module to give us the offsets we'll be using and commit space for us // if necessary DWORD dwNonGCOffset, dwGCOffset; @@ -7986,13 +7985,6 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() SetNumHandleThreadStatics(static_cast(dwNumHandleStatics)); - if (!FitsIn(bmtFP->NumThreadStaticGCBoxedFields)) - { // Overflow. - BuildMethodTableThrowException(IDS_EE_TOOMANYFIELDS); - } - - SetNumBoxedThreadStatics(static_cast(bmtFP->NumThreadStaticGCBoxedFields)); - // Tell the module to give us the offsets we'll be using and commit space for us // if necessary DWORD dwNonGCOffset, dwGCOffset; diff --git a/src/coreclr/vm/methodtablebuilder.h b/src/coreclr/vm/methodtablebuilder.h index 29889045b4479..9e3db269dac82 100644 --- a/src/coreclr/vm/methodtablebuilder.h +++ b/src/coreclr/vm/methodtablebuilder.h @@ -209,8 +209,6 @@ class MethodTableBuilder void SetModuleDynamicID(DWORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetModuleDynamicID(x); } void SetNumHandleRegularStatics(WORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetNumHandleRegularStatics(x); } void SetNumHandleThreadStatics(WORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetNumHandleThreadStatics(x); } - void SetNumBoxedRegularStatics(WORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetNumBoxedRegularStatics(x); } - void SetNumBoxedThreadStatics(WORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetNumBoxedThreadStatics(x); } void SetAlign8Candidate() { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetAlign8Candidate(); } void SetHasOverlaidFields() { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetHasOverlaidFields(); } void SetNonGCRegularStaticFieldBytes(DWORD x) { WRAPPER_NO_CONTRACT; GetHalfBakedClass()->SetNonGCRegularStaticFieldBytes(x); } diff --git a/src/coreclr/vm/packedfields.inl b/src/coreclr/vm/packedfields.inl deleted file mode 100644 index be5eff1da820c..0000000000000 --- a/src/coreclr/vm/packedfields.inl +++ /dev/null @@ -1,342 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// - -// - -// -// Provides a mechanism to store an array of DWORD-typed fields in a space-efficient manner. There are some -// caveats: -// 1) Fields can be written and read in an uncompressed form (a simple array of DWORD values) until the -// PackFields() method is invoked. Once this method has been invoked (and returned true) fields have been -// compacted and must not be modified again. That is, the primary usage of this class is to store a set of -// initialized-once fields. -// 2) The compaction algorithm relies on the fields containing small values (such as counts). Avoid storing -// fields that have special sentinel values (such as all bits set) which will frequently set high order -// bits. -// 3) An instance of PackedDWORDFields will take up a fixed quantity of memory equivalent to an array of -// DWORD fields. If PackFields() returns true then the fields values frozen at the time of the call have -// been compressed into a fewer number of bytes in-place. This smaller size will always be a multiple of -// sizeof(DWORD) in length and is reported by GetPackedSize(). If a PackedDWORDFields structure is being -// declared as a field inside another structure it is typically wise to place the field last to take -// advantage of this size reduction (e.g. when saving the outer structure into an ngen image). If -// PackFields() returns false then the fields remain unpacked and unchanged. -// 4) The caller retains the responsibility of recording whether an instance of PackedDWORDFields is in the -// packed or unpacked state. This is important since incorrect behavior will result if the wrong methods -// are used for the current state (e.g. calling GetUnpackedField() on a packed instance). This is not done -// automatically since there are no bits free to store the state. However, under a debug build correct -// usage will be checked (at the expensive of extra storage space). -// 5) The space saving made come at a runtime CPU cost to access the fields. Do not use this mechanism to -// compact fields that must be read on a performance critical path. If unsure, measure the performance of -// this solution before committing to it. -// -// ============================================================================ - -// Describe an array of FIELD_COUNT DWORDs. Each entry is addressed via a zero-based index and is expected to -// frequently contain a small integer and remain frozen after initialization. -template -class PackedDWORDFields -{ - // Some constants to make the code a little more readable. - enum Constants - { - kMaxLengthBits = 5, // Number of bits needed to express the maximum length of a field (32-bits) - kBitsPerDWORD = 32, // Number of bits in a DWORD - }; - -public: - // Fields are all initialized to zero. - PackedDWORDFields() - { - LIMITED_METHOD_DAC_CONTRACT; - - memset(m_rgUnpackedFields, 0, sizeof(m_rgUnpackedFields)); -#ifdef _DEBUG - memset(m_rgDebugUnpackedFields, 0, sizeof(m_rgDebugUnpackedFields)); - m_fFieldsPacked = false; -#endif // _DEBUG - } - - // Get the value of the given field when the structure is in its unpacked state. - DWORD GetUnpackedField(DWORD dwFieldIndex) - { - LIMITED_METHOD_DAC_CONTRACT; - - _ASSERTE(dwFieldIndex < FIELD_COUNT); - _ASSERTE(!m_fFieldsPacked); - - return m_rgUnpackedFields[dwFieldIndex]; - } - - // Set the value of the given field when the structure is in its unpacked state. Setting field values - // multiple times is allowed but only until a successful call to PackFields is made. - void SetUnpackedField(DWORD dwFieldIndex, DWORD dwValue) - { - LIMITED_METHOD_CONTRACT; - - _ASSERTE(dwFieldIndex < FIELD_COUNT); - _ASSERTE(!m_fFieldsPacked); - - m_rgUnpackedFields[dwFieldIndex] = dwValue; - -#ifdef _DEBUG - m_rgDebugUnpackedFields[dwFieldIndex] = dwValue; -#endif // _DEBUG - } - - // Attempt to optimize the set of fields given their current values. Returns false if compaction wouldn't - // achieve any space savings (in this case the structure remains in the unpacked state and the caller can - // continue to use the *UnpackedField methods above or even re-attempt PackFields() with different field - // values). If true is returned the data has been compacted into a smaller amount of space (this will - // always be a multiple of sizeof(DWORD) in size). This size can be queried using GetPackedSize() below. - // Once PackFields() has returned true fields can no longer be modified and field values must be retrieved - // via GetPackedField() rather than GetUnpackedField(). - bool PackFields() - { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - } - CONTRACTL_END; - - // Can't re-pack a packed structure. - _ASSERTE(!m_fFieldsPacked); - - // First compute the number of bits of space we'd need for a packed representation. Do this before - // making any changes since sometimes we'd end up expanding the data instead and in this case we wish - // to return false and make no updates to the structure. - - // There's a fixed overhead of kMaxLengthBits for each field (we store the packed fields as a - // bit-stream that alternates between a field length (of kMaxLengthBits) followed by a variable length - // bitfield containing the field value. - DWORD dwTotalPackedBits = FIELD_COUNT * kMaxLengthBits; - - // For each field calculate excatly how many bits we'd need to store the field value and add this to - // the total. - for (DWORD i = 0; i < FIELD_COUNT; i++) - dwTotalPackedBits += BitsRequired(m_rgUnpackedFields[i]); - - // Now we have the total is it smaller than a simple array of DWORDs? - if (dwTotalPackedBits >= (FIELD_COUNT * kBitsPerDWORD)) - return false; - - // Compaction will save us space. We're committed to implementing that compaction now. - - // Work from a copy of the unpacked fields since we're about to start modifying the space in which - // they're currently stored. - DWORD rgUnpackedFields[FIELD_COUNT]; - memcpy(rgUnpackedFields, m_rgUnpackedFields, sizeof(rgUnpackedFields)); - - // Start writing a stream of bits. For each field write a fixed sized header describing the number of - // bits required to express the field followed by the field value itself. - DWORD dwOffset = 0; - for (DWORD i = 0; i < FIELD_COUNT; i++) - { - // Find the minimal number of bits required to encode the current field's value. - DWORD dwFieldLength = BitsRequired(rgUnpackedFields[i]); - _ASSERTE(dwFieldLength > 0 && dwFieldLength <= kBitsPerDWORD); - - // Write the size field. Note that we store the size biased by one. That is, a field length of one - // is encoded as zero. We do this so we can express a range of field sizes from 1 through 32, - // emcompassing the worst case scenario (a full 32 bits). It comes at the cost of not being able - // to encode zero-valued fields with zero bits. Is this is deemed an important optimization in the - // future we could always given up on a simple linear mapping of the size field and use a lookup - // table to map values encoded into the real sizes. Experiments with EEClass packed fields over - // CoreLib show that this currently doesn't yield us much benefit, primarily due to the DWORD - // round-up size semantic, which implies we'd need a lot more optimization than this to reduce the - // average structure size below the next DWORD threshold. - BitVectorSet(dwOffset, kMaxLengthBits, dwFieldLength - 1); - dwOffset += kMaxLengthBits; - - // Write the field value itself. - BitVectorSet(dwOffset, dwFieldLength, rgUnpackedFields[i]); - dwOffset += dwFieldLength; - } - -#ifdef _DEBUG - m_fFieldsPacked = true; -#endif // _DEBUG - - // Compaction was successful. - return true; - } - - // Return the size in bytes of a compacted structure (it is illegal to call this on an uncompacted - // structure). - DWORD GetPackedSize() - { - LIMITED_METHOD_DAC_CONTRACT; - - _ASSERTE(m_fFieldsPacked); - - // Walk the field stream reading header (which are fixed size) and then using the value of the headers - // to skip the field value. - DWORD cBits = 0; - for (DWORD i = 0; i < FIELD_COUNT; i++) - cBits += kMaxLengthBits + BitVectorGet(cBits, kMaxLengthBits) + 1; // +1 since size is [1,32] not [0,31] - - // Compute the number of DWORDs needed to store the bits of the encoding. - // static_cast would not be necessary if ALIGN_UP were templated like FitsIn. - DWORD cDWORDs = static_cast(ALIGN_UP(cBits, kBitsPerDWORD)) / kBitsPerDWORD; - - // Return the total structure size. - return offsetof(PackedDWORDFields, m_rgPackedFields) + (cDWORDs * sizeof(DWORD)); - } - - // Get the value of a packed field. Illegal to call this on an uncompacted structure. - DWORD GetPackedField(DWORD dwFieldIndex) - { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - SUPPORTS_DAC; - } - CONTRACTL_END; - - _ASSERTE(dwFieldIndex < FIELD_COUNT); - _ASSERTE(m_fFieldsPacked); - - // Walk past all the predecessor fields. - DWORD dwOffset = 0; - for (DWORD i = 0; i < dwFieldIndex; i++) - dwOffset += kMaxLengthBits + BitVectorGet(dwOffset, kMaxLengthBits) + 1; // +1 since size is [1,32] not [0,31] - - // The next kMaxLengthBits bits contain the length in bits of the field we want (-1 due to the way we - // encode the length). - DWORD dwFieldLength = BitVectorGet(dwOffset, kMaxLengthBits) + 1; - dwOffset += kMaxLengthBits; - - // Grab the field value. - DWORD dwReturn = BitVectorGet(dwOffset, dwFieldLength); - - // On debug builds ensure the encoded field value is the same as the original unpacked version. - _ASSERTE(dwReturn == m_rgDebugUnpackedFields[dwFieldIndex]); - return dwReturn; - } - -private: - // Return the minimum number of bits required to encode a DWORD value by stripping out the - // most-significant leading zero bits). Returns a value between 1 and 32 inclusive (we never encode - // anything with zero bits). - DWORD BitsRequired(DWORD dwValue) - { - LIMITED_METHOD_CONTRACT; - - // Starting with a bit-mask of the most significant bit and iterating over masks for successively less - // significant bits, stop as soon as the mask co-incides with a set bit in the value. Simultaneously - // we're counting down the bits required to express the range of values implied by seeing the - // corresponding bit set in the value (e.g. when we're testing the high bit we know we'd need 32-bits - // to encode the range of values that have this bit set). Stop when we get to one bit (we never return - // 0 bits required, even for an input value of 0). - DWORD dwMask = 0x80000000; - DWORD cBits = 32; - while (cBits > 1) - { - if (dwValue & dwMask) - return cBits; - - dwMask >>= 1; - cBits--; - } - - return 1; - } - - // Set the dwLength bits at m_rgPackedFields + dwOffset bits to the value dwValue. - void BitVectorSet(DWORD dwOffset, DWORD dwLength, DWORD dwValue) - { - LIMITED_METHOD_CONTRACT; - - _ASSERTE(dwLength > 0 && dwLength <= kBitsPerDWORD); // Can set at most one DWORD at a time - _ASSERTE((dwLength == kBitsPerDWORD) || (dwValue < (1U << dwLength))); // Value had better fit in the given length - - // Calculate the start and end naturally aligned DWORDs into which the value will go. - DWORD dwStartBlock = dwOffset / kBitsPerDWORD; - DWORD dwEndBlock = (dwOffset + dwLength - 1) / kBitsPerDWORD; - if (dwStartBlock == dwEndBlock) - { - // Easy case: the new value fits entirely within one aligned DWORD. Compute the number of bits - // we'll need to shift the input value (to the left) and a mask of the bits that will be set in - // the destination DWORD. - DWORD dwValueShift = dwOffset % kBitsPerDWORD; - DWORD dwValueMask = ((1U << dwLength) - 1) << dwValueShift; - - m_rgPackedFields[dwStartBlock] &= ~dwValueMask; // Zero the target bits - m_rgPackedFields[dwStartBlock] |= dwValue << dwValueShift; // Or in the new value (suitably shifted) - } - else - { - // Hard case: the new value is split across two DWORDs (two DWORDs is the max as the new value can - // be at most DWORD-sized itself). For simplicity we'll simply break this into two separate - // non-spanning sets. We can revisit this in the future if the perf is a problem. - DWORD dwInitialBits = kBitsPerDWORD - (dwOffset % kBitsPerDWORD); // Number of bits to set in the first DWORD - DWORD dwInitialMask = (1U << dwInitialBits) - 1; // Mask covering those value bits - - // Set the portion of the value residing in the first DWORD. - BitVectorSet(dwOffset, dwInitialBits, dwValue & dwInitialMask); - - // And then the remainder in the second DWORD. - BitVectorSet(dwOffset + dwInitialBits, dwLength - dwInitialBits, dwValue >> dwInitialBits); - } - - _ASSERTE(BitVectorGet(dwOffset, dwLength) == dwValue); - } - - // Get the dwLength bits at m_rgPackedFields + dwOffset bits. Value is zero-extended to DWORD size. - DWORD BitVectorGet(DWORD dwOffset, DWORD dwLength) - { - LIMITED_METHOD_DAC_CONTRACT; - - _ASSERTE(dwLength > 0 && dwLength <= kBitsPerDWORD); // Can get at most one DWORD at a time - - // Calculate the start and end naturally aligned DWORDs from which the value will come. - DWORD dwStartBlock = dwOffset / kBitsPerDWORD; - DWORD dwEndBlock = (dwOffset + dwLength - 1) / kBitsPerDWORD; - if (dwStartBlock == dwEndBlock) - { - // Easy case: the new value fits entirely within one aligned DWORD. Compute the number of bits - // we'll need to shift the extracted value (to the right) and a mask of the bits that will be - // extracted in the destination DWORD. - DWORD dwValueShift = dwOffset % kBitsPerDWORD; - DWORD dwValueMask = ((1U << dwLength) - 1) << dwValueShift; - - // Mask out the bits we want and shift them down into the bottom of the result DWORD. - return (m_rgPackedFields[dwStartBlock] & dwValueMask) >> dwValueShift; - } - else - { - // Hard case: the return value is split across two DWORDs (two DWORDs is the max as the new value - // can be at most DWORD-sized itself). For simplicity we'll simply break this into two separate - // non-spanning gets and stitch the result together from that. We can revisit this in the future - // if the perf is a problem. - DWORD dwInitialBits = kBitsPerDWORD - (dwOffset % kBitsPerDWORD); // Number of bits to get in the first DWORD - DWORD dwReturn; - - // Get the initial (low-order) bits from the first DWORD. - dwReturn = BitVectorGet(dwOffset, dwInitialBits); - - // Get the remaining bits from the second DWORD. These bits will need to be shifted to the left - // (past the bits we've already read) before being OR'd into the result. - dwReturn |= BitVectorGet(dwOffset + dwInitialBits, dwLength - dwInitialBits) << dwInitialBits; - - return dwReturn; - } - } - -#ifdef _DEBUG - DWORD m_rgDebugUnpackedFields[FIELD_COUNT]; // A copy of the unpacked fields so we can validate - // packed reads - bool m_fFieldsPacked; // The current packed/unpacked state so we can check - // the right methods are being called -#endif // _DEBUG - - union - { - DWORD m_rgUnpackedFields[FIELD_COUNT]; // The fields in their unpacked state - DWORD m_rgPackedFields[1]; // The first DWORD block of fields in the packed state - }; -}; diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index d09d90c83c12e..94088ba399947 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -355,7 +355,7 @@ void ThreadLocalBlock::AllocateThreadStaticBoxes(MethodTable * pMT) THROWS; GC_TRIGGERS; MODE_COOPERATIVE; - PRECONDITION(pMT->GetNumBoxedThreadStatics() > 0); + PRECONDITION(pMT->HasBoxedThreadStatics()); INJECT_FAULT(COMPlusThrowOM();); } CONTRACTL_END; @@ -566,7 +566,7 @@ void ThreadLocalModule::PopulateClass(MethodTable *pMT) // We need to allocate boxes any value-type statics that are not // primitives or enums, because these statics may contain references // to objects on the GC heap - if (pMT->GetNumBoxedThreadStatics() > 0) + if (pMT->HasBoxedThreadStatics()) { PTR_ThreadLocalBlock pThreadLocalBlock = ThreadStatics::GetCurrentTLB(); _ASSERTE(pThreadLocalBlock != NULL); From c34201c3880317f14f8d0771f4b5bb6c207f70c3 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 9 Jan 2024 12:52:25 -0800 Subject: [PATCH 2/3] Update src/coreclr/vm/array.cpp Co-authored-by: Austin Wise --- src/coreclr/vm/array.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index c19c9cb9f311f..7bb56a935593c 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -322,7 +322,7 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy offsetOfNonVirtualSlots = cbMT; cbMT += numNonVirtualSlots * sizeof(PCODE); - // Allocate ArrayClass (including space for packed fields), MethodTable, and class name in one alloc. + // Allocate ArrayClass, MethodTable, and class name in one alloc. // Remember to pad allocation size for ArrayClass portion to ensure MethodTable is pointer aligned. cbArrayClass = ALIGN_UP(sizeof(ArrayClass), sizeof(void*)); } From 966c47b3afd6b904bdcf1ce7863e2b9316dcdb54 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 11 Jan 2024 10:38:54 -0800 Subject: [PATCH 3/3] Quick fix --- src/coreclr/vm/array.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index d53f619f36a8c..26f1b3a8f3bbc 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -307,9 +307,6 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy if (pCanonMT == NULL) { - offsetOfNonVirtualSlots = cbMT; - cbMT += numNonVirtualSlots * sizeof(PCODE); - // Allocate ArrayClass, MethodTable, and class name in one alloc. // Remember to pad allocation size for ArrayClass portion to ensure MethodTable is pointer aligned. cbArrayClass = ALIGN_UP(sizeof(ArrayClass), sizeof(void*));