From a30ad3d9d2718260d40572e13ff862cba00e867e Mon Sep 17 00:00:00 2001 From: Brian Fiete Date: Mon, 11 May 2020 10:17:45 -0700 Subject: [PATCH] When resizing, insured we didn't release old memory until after insert --- BeefLibs/corlib/src/Collections/Dictionary.bf | 94 +++++++++--- BeefLibs/corlib/src/Collections/List.bf | 140 +++++++++--------- BeefTools/BeefPerf/src/BPClient.bf | 2 +- .../minlib/src/System/Collections/List.bf | 115 ++++++++------ 4 files changed, 212 insertions(+), 139 deletions(-) diff --git a/BeefLibs/corlib/src/Collections/Dictionary.bf b/BeefLibs/corlib/src/Collections/Dictionary.bf index 168a1cc9..d3ea79ab 100644 --- a/BeefLibs/corlib/src/Collections/Dictionary.bf +++ b/BeefLibs/corlib/src/Collections/Dictionary.bf @@ -12,6 +12,7 @@ namespace System.Collections using System.Collections; using System.Diagnostics; using System.Diagnostics.Contracts; + using System.Threading; public class Dictionary : ICollection<(TKey key, TValue value)>, @@ -29,8 +30,8 @@ namespace System.Collections public int_cosize mNext; // Index of next entry, -1 if last } - int_cosize* mBuckets ~ delete _; - Entry* mEntries ~ delete _; + int_cosize* mBuckets; + Entry* mEntries; int_cosize mAllocSize; int_cosize mCount; int_cosize mFreeList; @@ -49,6 +50,20 @@ namespace System.Collections //TODO: this.comparer = comparer ?? EqualityComparer.Default; } + public ~this() + { + if (mEntries != null) + { + var entries = mEntries; +#if BF_ENABLE_REALTIME_LEAK_CHECK + // To avoid scanning items being deleted + mEntries = null; + Interlocked.Fence(); +#endif + Free(entries); + } + } + public int Count { get { return mCount - mFreeCount; } @@ -123,7 +138,7 @@ namespace System.Collections { TKey* keyPtr; TValue* valuePtr; - bool inserted = Insert(key, false, out keyPtr, out valuePtr); + bool inserted = Insert(key, false, out keyPtr, out valuePtr, null); if (!inserted) return false; *keyPtr = key; @@ -133,7 +148,32 @@ namespace System.Collections public bool TryAdd(TKey key, out TKey* keyPtr, out TValue* valuePtr) { - return Insert(key, false, out keyPtr, out valuePtr); + return Insert(key, false, out keyPtr, out valuePtr, null); + } + + protected virtual (Entry*, int_cosize*) Alloc(int size) + { + int byteSize = size * (strideof(Entry) + sizeof(int_cosize)); + uint8* allocPtr = new uint8[byteSize]*; + return ((Entry*)allocPtr, (int_cosize*)(allocPtr + size * strideof(Entry))); + } + + protected virtual void Free(Entry* entryPtr) + { + delete (void*)entryPtr; + } + + protected override void GCMarkMembers() + { + if (mEntries == null) + return; + let type = typeof(Entry); + if ((type.[Friend]mTypeFlags & .WantsMark) == 0) + return; + for (int i < mCount) + { + GC.Mark_Unbound(mEntries[i]); + } } public enum AddResult @@ -145,8 +185,8 @@ namespace System.Collections public AddResult TryAdd(TKey key) { TKey* keyPtr; - TValue* valuePtr; - if (Insert(key, false, out keyPtr, out valuePtr)) + TValue* valuePtr; + if (Insert(key, false, out keyPtr, out valuePtr, null)) return .Added(keyPtr, valuePtr); return .Exists(keyPtr, valuePtr); } @@ -299,9 +339,8 @@ namespace System.Collections private void Initialize(int capacity) { int_cosize size = GetPrimeish((int_cosize)capacity); - mBuckets = new int_cosize[size]*; + (mEntries, mBuckets) = Alloc(size); for (int_cosize i < (int_cosize)size) mBuckets[i] = -1; - mEntries = new Entry[size]*; mAllocSize = size; mFreeList = -1; } @@ -328,6 +367,7 @@ namespace System.Collections } } int_cosize index; + Entry* oldData = null; if (mFreeCount > 0) { index = mFreeList; @@ -338,7 +378,7 @@ namespace System.Collections { if (mCount == mAllocSize) { - Resize(); + oldData = Resize(false); targetBucket = hashCode % (int_cosize)mAllocSize; } index = mCount; @@ -350,17 +390,18 @@ namespace System.Collections mEntries[index].mKey = key; mEntries[index].mValue = value; mBuckets[targetBucket] = index; + if (oldData != null) + Free(oldData); #if VERSION_DICTIONARY mVersion++; #endif } - private bool Insert(TKey key, bool add, out TKey* keyPtr, out TValue* valuePtr) + private bool Insert(TKey key, bool add, out TKey* keyPtr, out TValue* valuePtr, Entry** outOldData) { if (mBuckets == null) Initialize(0); int32 hashCode = (int32)key.GetHashCode() & 0x7FFFFFFF; int_cosize targetBucket = hashCode % (int_cosize)mAllocSize; - for (int_cosize i = mBuckets[targetBucket]; i >= 0; i = mEntries[i].mNext) { if (mEntries[i].mHashCode == hashCode && (mEntries[i].mKey == key)) @@ -371,10 +412,13 @@ namespace System.Collections } keyPtr = &mEntries[i].mKey; valuePtr = &mEntries[i].mValue; + if (outOldData != null) + *outOldData = null; return false; } } int_cosize index; + Entry* oldData = null; if (mFreeCount > 0) { index = mFreeList; @@ -385,7 +429,7 @@ namespace System.Collections { if (mCount == mAllocSize) { - Resize(); + oldData = Resize(false); targetBucket = hashCode % (int_cosize)mAllocSize; } index = mCount; @@ -402,6 +446,10 @@ namespace System.Collections keyPtr = &mEntries[index].mKey; valuePtr = &mEntries[index].mValue; + if (outOldData != null) + *outOldData = oldData; + else + Free(oldData); return true; } @@ -418,17 +466,16 @@ namespace System.Collections return GetPrimeish(newSize); } - private void Resize() + private Entry* Resize(bool autoFree) { - Resize(ExpandSize(mCount), false); + return Resize(ExpandSize(mCount), false, autoFree); } - private void Resize(int newSize, bool forceNewHashCodes) + private Entry* Resize(int newSize, bool forceNewHashCodes, bool autoFree) { Contract.Assert(newSize >= mAllocSize); - int_cosize* newBuckets = new int_cosize[newSize]*; + (var newEntries, var newBuckets) = Alloc(newSize); for (int_cosize i = 0; i < newSize; i++) newBuckets[i] = -1; - Entry* newEntries = new Entry[newSize]*; Internal.MemCpy(newEntries, mEntries, mCount * strideof(Entry), alignof(Entry)); if (forceNewHashCodes) @@ -451,12 +498,17 @@ namespace System.Collections } } - delete mBuckets; - delete mEntries; - + let oldPtr = mEntries; mBuckets = newBuckets; mEntries = newEntries; mAllocSize = (int_cosize)newSize; + + if (autoFree) + { + Free(oldPtr); + return null; + } + return oldPtr; } public bool Remove(TKey key) @@ -875,7 +927,7 @@ namespace System.Collections public struct KeyEnumerator : IEnumerator, IResettable { private Dictionary mDictionary; -# if VERSION_DICTIONARY +#if VERSION_DICTIONARY private int32 mVersion; #endif private int_cosize mIndex; diff --git a/BeefLibs/corlib/src/Collections/List.bf b/BeefLibs/corlib/src/Collections/List.bf index e69c9036..d099c08f 100644 --- a/BeefLibs/corlib/src/Collections/List.bf +++ b/BeefLibs/corlib/src/Collections/List.bf @@ -105,7 +105,8 @@ namespace System.Collections if (IsDynAlloc) { var items = mItems; -#if BF_ENABLE_REALTIME_LEAK_CHECK +#if BF_ENABLE_REALTIME_LEAK_CHECK + // To avoid scanning items being deleted mItems = null; Interlocked.Fence(); #endif @@ -138,41 +139,9 @@ namespace System.Collections Debug.Assert((uint)value <= (uint)SizeFlags); if (value != AllocSize) { - if (value > 0) - { - T* newItems = Alloc(value); - -#if DBG - int typeId = typeof(T).GetTypeId(); - if (typeId == sDebugTypeId) - { - Debug.WriteLine("Alloc {0} {1} {2}", scope Object[] { this, newItems, sDebugIdx } ); - sDebugIdx++; - } -#endif - - if (mSize > 0) - Internal.MemCpy(newItems, mItems, mSize * strideof(T), alignof(T)); - - var oldItems = mItems; - mItems = newItems; - if (IsDynAlloc) - { -#if BF_ENABLE_REALTIME_LEAK_CHECK - // We need to avoid scanning a deleted mItems - Interlocked.Fence(); -#endif - Free(oldItems); - } - mAllocSizeAndFlags = (.)(value | DynAllocFlag); - } - else - { - if (IsDynAlloc) - Free(mItems); - mItems = null; - mAllocSizeAndFlags = 0; - } + T* oldAlloc = Realloc(value, true); + if (oldAlloc != null) + Free(oldAlloc); } } } @@ -259,39 +228,58 @@ namespace System.Collections } } - protected T* Alloc(int size) + protected virtual T* Alloc(int size) { return Internal.AllocRawArrayUnmarked(size); } - protected void Free(T* val) + protected virtual void Free(T* val) { delete (void*)val; } - /*protected T[] Alloc(int size) - { - return new:this T[size]; - } - protected void Free(Object obj) + T* Realloc(int newSize, bool autoFree) { - delete:this obj; - } + T* oldAlloc = null; + if (newSize > 0) + { + T* newItems = Alloc(newSize); - protected virtual Object AllocObject(TypeInstance type, int size) - { - return Internal.ObjectAlloc(type, size); - } + if (mSize > 0) + Internal.MemCpy(newItems, mItems, mSize * strideof(T), alignof(T)); + if (IsDynAlloc) + oldAlloc = mItems; + mItems = newItems; + mAllocSizeAndFlags = (.)(newSize | DynAllocFlag); + } + else + { + if (IsDynAlloc) + oldAlloc = mItems; + mItems = null; + mAllocSizeAndFlags = 0; + } - protected virtual void FreeObject(Object obj) - { - delete obj; - }*/ + if ((autoFree) && (oldAlloc != null)) + { + Free(oldAlloc); + return null; + } + + return oldAlloc; + } /// Adds an item to the back of the list. public void Add(T item) { - if (mSize == AllocSize) EnsureCapacity(mSize + 1); + if (mSize == AllocSize) + { + // We free after the insert to allow for inserting a ref to another list element + let oldPtr = EnsureCapacity(mSize + 1, false); + mItems[mSize++] = item; + Free(oldPtr); + return; + } mItems[mSize++] = item; #if VERSION_LIST mVersion++; @@ -301,7 +289,14 @@ namespace System.Collections /// Adds an item to the back of the list. public void Add(Span addSpan) { - if (mSize == AllocSize) EnsureCapacity(mSize + addSpan.Length); + if (mSize == AllocSize) + { + let oldPtr = EnsureCapacity(mSize + addSpan.Length, false); + for (var val in ref addSpan) + mItems[mSize++] = val; + Free(oldPtr); + return; + } for (var val in ref addSpan) mItems[mSize++] = val; #if VERSION_LIST @@ -312,7 +307,7 @@ namespace System.Collections /// Returns a pointer to the start of the added uninitialized section public T* GrowUnitialized(int addSize) { - if (mSize + addSize > AllocSize) EnsureCapacity(mSize + addSize); + if (mSize + addSize > AllocSize) EnsureCapacity(mSize + addSize, true); mSize += (int_cosize)addSize; #if VERSION_LIST mVersion++; @@ -365,7 +360,7 @@ namespace System.Collections public void CopyTo(List destList) { - destList.EnsureCapacity(mSize); + destList.EnsureCapacity(mSize, true); destList.mSize = mSize; if (mSize > 0) Internal.MemCpy(destList.mItems, mItems, mSize * strideof(T), alignof(T)); @@ -385,25 +380,23 @@ namespace System.Collections array[i + arrayIndex] = mItems[i + index]; } - public void EnsureCapacity(int min) + public T* EnsureCapacity(int min, bool autoFree) { int allocSize = AllocSize; - if (allocSize < min) - { - int newCapacity = allocSize == 0 ? cDefaultCapacity : allocSize + allocSize / 2; - // If we overflow, try to set to max. The "< min" check after still still bump us up - // if necessary - if (newCapacity > SizeFlags) - newCapacity = SizeFlags; - if (newCapacity < min) - newCapacity = min; - Capacity = newCapacity; - } + if (allocSize >= min) + return null; + + int_cosize newCapacity = (int_cosize)(allocSize == 0 ? cDefaultCapacity : allocSize * 2); + // Allow the list to grow to maximum possible capacity (~2G elements) before encountering overflow. + // Note that this check works even when mItems.Length overflowed thanks to the (uint) cast + //if ((uint)newCapacity > Array.MaxArrayLength) newCapacity = Array.MaxArrayLength; + if (newCapacity < min) newCapacity = (int_cosize)min; + return Realloc(newCapacity, autoFree); } public void Reserve(int size) { - EnsureCapacity(size); + EnsureCapacity(size, true); } public Enumerator GetEnumerator() @@ -453,7 +446,8 @@ namespace System.Collections public void Insert(int index, T item) { - if (mSize == AllocSize) EnsureCapacity(mSize + 1); + var item; // This creates a copy - required if item is a ref to an element + if (mSize == AllocSize) EnsureCapacity(mSize + 1, true); if (index < mSize) { Internal.MemCpy(mItems + index + 1, mItems + index, (mSize - index) * strideof(T), alignof(T)); @@ -467,10 +461,12 @@ namespace System.Collections public void Insert(int index, Span items) { + //TODO: Handle case where Span is a reference to ourselves + if (items.Length == 0) return; int addCount = items.Length; - if (mSize + addCount > AllocSize) EnsureCapacity(mSize + addCount); + if (mSize + addCount > AllocSize) EnsureCapacity(mSize + addCount, true); if (index < mSize) { Internal.MemCpy(mItems + index + addCount, mItems + index, (mSize - index) * strideof(T), alignof(T)); diff --git a/BeefTools/BeefPerf/src/BPClient.bf b/BeefTools/BeefPerf/src/BPClient.bf index 64a37302..43a6c660 100644 --- a/BeefTools/BeefPerf/src/BPClient.bf +++ b/BeefTools/BeefPerf/src/BPClient.bf @@ -183,7 +183,7 @@ namespace BeefPerf if (mStreamDataList.Capacity == 0) mStreamDataList.Capacity = srcStreamDataList.Count; else - mStreamDataList.EnsureCapacity(srcStreamDataList.Count); + mStreamDataList.EnsureCapacity(srcStreamDataList.Count, true); mStreamDataList.GrowUnitialized(idx - (int32)mStreamDataList.Count + 1); } diff --git a/IDE/mintest/minlib/src/System/Collections/List.bf b/IDE/mintest/minlib/src/System/Collections/List.bf index c962f820..8224f396 100644 --- a/IDE/mintest/minlib/src/System/Collections/List.bf +++ b/IDE/mintest/minlib/src/System/Collections/List.bf @@ -140,33 +140,9 @@ namespace System.Collections Debug.Assert((uint)value <= (uint)SizeFlags); if (value != AllocSize) { - if (value > 0) - { - T* newItems = Alloc(value); - -#if DBG - int typeId = typeof(T).GetTypeId(); - if (typeId == sDebugTypeId) - { - Debug.WriteLine("Alloc {0} {1} {2}", scope Object[] { this, newItems, sDebugIdx } ); - sDebugIdx++; - } -#endif - - if (mSize > 0) - Internal.MemCpy(newItems, mItems, mSize * strideof(T), alignof(T)); - if (IsDynAlloc) - Free(mItems); - mItems = newItems; - mAllocSizeAndFlags = (.)(value | DynAllocFlag); - } - else - { - if (IsDynAlloc) - Free(mItems); - mItems = null; - mAllocSizeAndFlags = 0; - } + T* oldAlloc = Realloc(value, true); + if (oldAlloc != null) + Free(oldAlloc); } } } @@ -258,6 +234,46 @@ namespace System.Collections #endif } + T* Realloc(int newSize, bool autoFree) + { + T* oldAlloc = null; + if (newSize > 0) + { + T* newItems = Alloc(newSize); + +#if DBG + int typeId = typeof(T).GetTypeId(); + if (typeId == sDebugTypeId) + { + Debug.WriteLine("Alloc {0} {1} {2}", scope Object[] { this, newItems, sDebugIdx } ); + sDebugIdx++; + } +#endif + + if (mSize > 0) + Internal.MemCpy(newItems, mItems, mSize * strideof(T), alignof(T)); + if (IsDynAlloc) + oldAlloc = mItems; + mItems = newItems; + mAllocSizeAndFlags = (.)(newSize | DynAllocFlag); + } + else + { + if (IsDynAlloc) + oldAlloc = mItems; + mItems = null; + mAllocSizeAndFlags = 0; + } + + if ((autoFree) && (oldAlloc != null)) + { + Free(oldAlloc); + return null; + } + + return oldAlloc; + } + protected void Free(T* val) { delete (void*)val; @@ -285,12 +301,14 @@ namespace System.Collections /// Adds an item to the back of the list. public void Add(T item) { - if (((int)Internal.UnsafeCastToPtr(this) & 0xFFFF) == 0x4BA0) + if (mSize == AllocSize) { - NOP!(); + // We free after the insert to allow for inserting a ref to another list element + let oldPtr = EnsureCapacity(mSize + 1, false); + mItems[mSize++] = item; + Free(oldPtr); + return; } - - if (mSize == AllocSize) EnsureCapacity(mSize + 1); mItems[mSize++] = item; #if VERSION_LIST mVersion++; @@ -300,7 +318,7 @@ namespace System.Collections /// Returns a pointer to the start of the added uninitialized section public T* GrowUnitialized(int addSize) { - if (mSize + addSize > AllocSize) EnsureCapacity(mSize + addSize); + if (mSize + addSize > AllocSize) EnsureCapacity(mSize + addSize, true); mSize += (int_cosize)addSize; #if VERSION_LIST mVersion++; @@ -353,7 +371,7 @@ namespace System.Collections public void CopyTo(List destList) { - destList.EnsureCapacity(mSize); + destList.EnsureCapacity(mSize, true); destList.mSize = mSize; if (mSize > 0) Internal.MemCpy(destList.mItems, mItems, mSize * strideof(T), alignof(T)); @@ -366,23 +384,23 @@ namespace System.Collections array[i + arrayIndex] = mItems[i]; } - public void EnsureCapacity(int min) + public T* EnsureCapacity(int min, bool autoFree) { int allocSize = AllocSize; - if (allocSize < min) - { - int_cosize newCapacity = (int_cosize)(allocSize == 0 ? cDefaultCapacity : allocSize * 2); - // Allow the list to grow to maximum possible capacity (~2G elements) before encountering overflow. - // Note that this check works even when mItems.Length overflowed thanks to the (uint) cast - //if ((uint)newCapacity > Array.MaxArrayLength) newCapacity = Array.MaxArrayLength; - if (newCapacity < min) newCapacity = (int_cosize)min; - Capacity = newCapacity; - } + if (allocSize >= min) + return null; + + int_cosize newCapacity = (int_cosize)(allocSize == 0 ? cDefaultCapacity : allocSize * 2); + // Allow the list to grow to maximum possible capacity (~2G elements) before encountering overflow. + // Note that this check works even when mItems.Length overflowed thanks to the (uint) cast + //if ((uint)newCapacity > Array.MaxArrayLength) newCapacity = Array.MaxArrayLength; + if (newCapacity < min) newCapacity = (int_cosize)min; + return Realloc(newCapacity, autoFree); } public void Reserve(int size) { - EnsureCapacity(size); + EnsureCapacity(size, true); } public Enumerator GetEnumerator() @@ -417,7 +435,14 @@ namespace System.Collections public void Insert(int index, T item) { - if (mSize == AllocSize) EnsureCapacity(mSize + 1); + if (mSize == AllocSize) + { + let oldPtr = EnsureCapacity(mSize + 1, false); + Internal.MemCpy(mItems + index + 1, mItems + index, (mSize - index) * strideof(T), alignof(T)); + Free(oldPtr); + return; + } + if (index < mSize) { Internal.MemCpy(mItems + index + 1, mItems + index, (mSize - index) * strideof(T), alignof(T));