Skip to content

Further clean up usages of LowLevelList<T> #109114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

Further clean up usages of LowLevelList<T> #109114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d465850
Use List<T> in CoreLib
huoyaoyuan Oct 22, 2024
a672cdf
Use List<T> in ReflectionExecution
huoyaoyuan Oct 22, 2024
0bfd0cb
Replace with bool[] in GC layout
huoyaoyuan Oct 22, 2024
46546e6
Update documentation and remove unused members
huoyaoyuan Oct 22, 2024
8d45b8d
Remove unused _version
huoyaoyuan Oct 22, 2024
b1aa7db
Replace usage of ref array
huoyaoyuan Oct 22, 2024
4bc86fe
Merge branch 'main' into lowlevel-list-2
huoyaoyuan Oct 24, 2024
2aa0f67
Remove offset and input for bitfield
huoyaoyuan Oct 26, 2024
e561f32
Merge branch 'main' into lowlevel-list-2
huoyaoyuan Nov 15, 2024
cbfe20c
Merge branch 'main' into lowlevel-list-2
huoyaoyuan Jan 4, 2025
9bcd4dc
Use Dictionary instead
huoyaoyuan Jan 4, 2025
5686096
Merge branch 'main' into lowlevel-list-2
agocke Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 8 additions & 64 deletions src/coreclr/nativeaot/Common/src/System/Collections/Generic/LowLevelList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@
/*============================================================
**
**
** Private version of List<T> for internal System.Private.CoreLib use. This
** permits sharing more source between BCL and System.Private.CoreLib (as well as the
** fact that List<T> is just a useful class in general.)
** Private version of List<T> for internal System.Private.TypeLoader use. Type
** loader itself can't use generic types without [ForceDictionaryLookups], and
** we don't want to annotate List<T> with it because it would impact size and
** performance for general usages.
**
** This does not strive to implement the full api surface area
** (but any portion it does implement should match the real List<T>'s
** behavior.)
**
** This file is a subset of System.Collections\System\Collections\Generics\List.cs
** and should be kept in sync with that file.
** This file is a subset of
** src\libraries\System.Private.CoreLib\src\System\Collections\Generics\List.cs
** and should be kept in sync with that file when necessary.
**
===========================================================*/

Expand All @@ -32,16 +34,14 @@ namespace System.Collections.Generic
// Data size is smaller because there will be minimal virtual function table.
// Code size is smaller because only functions called will be in the binary.
[DebuggerDisplay("Count = {Count}")]
#if TYPE_LOADER_IMPLEMENTATION
[System.Runtime.CompilerServices.ForceDictionaryLookups]
#endif
internal class LowLevelList<T>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the type be moved under TypeLoader since it's only required for it?

Additional question: is ForceDictionaryLookups only applicable for reference types because of generic sharing, or also applicable for value types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the type be moved under TypeLoader since it's only required for it?

It is very likely that there is code in CoreLib that is used by the TypeLoader indirectly that would need to be annotated with this attribute if we ever resurrected this (#85184 (comment)).

I think it would be ok to delete ForceDictionaryLookups and its few remaining uses. It is likely heavily bitrotten.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in this PR I was using Array.Resize which is generic. It seems that the real foreseeing solution is to limit TypeLoader usage of CoreLib, according to the linked discussion. It also means that TypeLoader needs its specialized collection. If we resurrected the generic lookup work, is it expected to use the attribute, or just do some specialized handling for TypeLoader?

The original question is that, would there be any place else (not involved with TypeLoader) unable to use the CoreLib collection? I assume no.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #85184 (comment), if we ever resurrected the lazy generic lookup work, we would need likely ended up with TypeLoader in its own partition that does not perform the lazy generic lookups. It would be very large refactoring.

{
private const int _defaultCapacity = 4;

protected T[] _items;
protected int _size;
protected int _version;
// No _version field because no IEnumerable<T> support

#pragma warning disable CA1825 // avoid the extra generic instantiation for Array.Empty<T>()
private static readonly T[] s_emptyArray = new T[0];
Expand All @@ -55,60 +55,6 @@ public LowLevelList()
_items = s_emptyArray;
}

// Constructs a List with a given initial capacity. The list is
// initially empty, but will have room for the given number of elements
// before any reallocations are required.
//
public LowLevelList(int capacity)
{
ArgumentOutOfRangeException.ThrowIfNegative(capacity);

if (capacity == 0)
_items = s_emptyArray;
else
_items = new T[capacity];
}

// Constructs a List, copying the contents of the given collection. The
// size and capacity of the new list will both be equal to the size of the
// given collection.
//
public LowLevelList(IEnumerable<T> collection)
{
ArgumentNullException.ThrowIfNull(collection);

ICollection<T>? c = collection as ICollection<T>;
if (c != null)
{
int count = c.Count;
if (count == 0)
{
_items = s_emptyArray;
}
else
{
_items = new T[count];
c.CopyTo(_items, 0);
_size = count;
}
}
else
{
_size = 0;
_items = s_emptyArray;
// This enumerable could be empty. Let Add allocate a new array, if needed.
// Note it will also go to _defaultCapacity first, not 1, then 2, etc.

using (IEnumerator<T> en = collection.GetEnumerator())
{
while (en.MoveNext())
{
Add(en.Current);
}
}
}
}

// Gets and sets the capacity of this list. The capacity is the size of
// the internal array used to hold items. When set, the internal
// array of the list is reallocated to the given capacity.
Expand Down Expand Up @@ -163,7 +109,6 @@ public T this[int index]
{
ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual((uint)index, (uint)_size, nameof(index));
_items[index] = value;
_version++;
}
}

Expand All @@ -176,7 +121,6 @@ public void Add(T item)
{
if (_size == _items.Length) EnsureCapacity(_size + 1);
_items[_size++] = item;
_version++;
}

// Ensures that the capacity of this list is at least the given minimum
Expand Down
2 changes: 1 addition & 1 deletion ...Private.CoreLib/src/Internal/Reflection/Extensions/NonPortable/CustomAttributeSearcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ private IEnumerable<CustomAttributeData> GetMatchingCustomAttributesIterator(E e
return rawPassesFilter(attributeType);
};

LowLevelList<CustomAttributeData> immediateResults = new LowLevelList<CustomAttributeData>();
List<CustomAttributeData> immediateResults = new List<CustomAttributeData>();
foreach (CustomAttributeData cad in GetDeclaredCustomAttributes(element))
{
if (passesFilter(cad.AttributeType))
Expand Down
2 changes: 1 addition & 1 deletion ...iveaot/System.Private.CoreLib/src/Internal/Runtime/CompilerServices/FunctionPointerOps.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public override int GetHashCode()

private static uint s_genericFunctionPointerNextIndex;
private const uint c_genericDictionaryChunkSize = 1024;
private static LowLevelList<IntPtr> s_genericFunctionPointerCollection = new LowLevelList<IntPtr>();
private static readonly List<IntPtr> s_genericFunctionPointerCollection = new List<IntPtr>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This s_genericFunctionPointerCollection pool looks like a premature optimization. I think it would be fine to allocate GenericMethodDescriptorInfos one at a time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean there will be memory leak per function pointer, instead of per method instantiation? This would become unbounded instead of bounded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s_genericFunctionPointerCollection does not enable any GenericMethodDescriptor reuse. There is s_genericFunctionPointerDictionary for that. I am not suggesting deleting s_genericFunctionPointerDictionary.

s_genericFunctionPointerCollection reduces frequency of NativeMemory.Alloc calls. We allocate spaces for c_genericDictionaryChunkSize GenericMethodDescriptors at a time using NativeMemory.Alloc and then hand them out one at a time.

private static LowLevelDictionary<GenericMethodDescriptorInfo, uint> s_genericFunctionPointerDictionary = new LowLevelDictionary<GenericMethodDescriptorInfo, uint>();

public static unsafe IntPtr GetGenericMethodFunctionPointer(IntPtr canonFunctionPointer, IntPtr instantiationArgument)
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,6 @@
<Compile Include="$(CompilerCommonPath)\TypeSystem\Common\Utilities\LockFreeReaderHashtableOfPointers.cs">
<Link>Utilities\LockFreeReaderHashtableOfPointers.cs</Link>
</Compile>
<Compile Include="$(AotCommonPath)\System\Collections\Generic\LowLevelList.cs">
<Link>System\Collections\Generic\LowLevelList.cs</Link>
</Compile>
<Compile Include="$(AotCommonPath)\System\Collections\Generic\LowLevelDictionary.cs">
<Link>System\Collections\Generic\LowLevelDictionary.cs</Link>
</Compile>
Expand Down
2 changes: 1 addition & 1 deletion .../nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/NamespaceChain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal NamespaceChain(MetadataReader reader, NamespaceDefinitionHandle innerMo
NamespaceDefinition currentNamespaceDefinition = innerMostNamespaceHandle.GetNamespaceDefinition(reader);
ConstantStringValueHandle currentNameHandle = currentNamespaceDefinition.Name;
Handle currentNamespaceHandle;
LowLevelList<string> names = new LowLevelList<string>();
List<string> names = new List<string>();
for (; ; )
{
string name = currentNameHandle.GetStringOrNull(reader);
Expand Down
24 changes: 12 additions & 12 deletions ...src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.ManifestResources.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal sealed partial class ExecutionEnvironmentImplementation : ExecutionEnvi
{
public sealed override ManifestResourceInfo GetManifestResourceInfo(Assembly assembly, string resourceName)
{
LowLevelList<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
List<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
for (int i = 0; i < resourceInfos.Count; i++)
{
if (resourceName == resourceInfos[i].Name)
Expand All @@ -35,7 +35,7 @@ public sealed override ManifestResourceInfo GetManifestResourceInfo(Assembly ass

public sealed override string[] GetManifestResourceNames(Assembly assembly)
{
LowLevelList<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
List<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
string[] names = new string[resourceInfos.Count];
for (int i = 0; i < resourceInfos.Count; i++)
{
Expand All @@ -50,7 +50,7 @@ public sealed override Stream GetManifestResourceStream(Assembly assembly, strin

// This was most likely an embedded resource which the toolchain should have embedded
// into an assembly.
LowLevelList<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
List<ResourceInfo> resourceInfos = GetExtractedResources(assembly);
for (int i = 0; i < resourceInfos.Count; i++)
{
ResourceInfo resourceInfo = resourceInfos[i];
Expand Down Expand Up @@ -78,17 +78,17 @@ private static unsafe UnmanagedMemoryStream ReadResourceFromBlob(ResourceInfo re
return new UnmanagedMemoryStream(pBlob + resourceInfo.Index, resourceInfo.Length);
}

private static LowLevelList<ResourceInfo> GetExtractedResources(Assembly assembly)
private static List<ResourceInfo> GetExtractedResources(Assembly assembly)
{
LowLevelDictionary<string, LowLevelList<ResourceInfo>> extractedResourceDictionary = ExtractedResourceDictionary;
Dictionary<string, List<ResourceInfo>> extractedResourceDictionary = ExtractedResourceDictionary;
string assemblyName = assembly.GetName().FullName;
LowLevelList<ResourceInfo> resourceInfos;
List<ResourceInfo> resourceInfos;
if (!extractedResourceDictionary.TryGetValue(assemblyName, out resourceInfos))
return new LowLevelList<ResourceInfo>();
return new List<ResourceInfo>();
return resourceInfos;
}

private static LowLevelDictionary<string, LowLevelList<ResourceInfo>> ExtractedResourceDictionary
private static Dictionary<string, List<ResourceInfo>> ExtractedResourceDictionary
{
get
{
Expand All @@ -97,7 +97,7 @@ private static LowLevelDictionary<string, LowLevelList<ResourceInfo>> ExtractedR
// Lazily create the extracted resource dictionary. If two threads race here, we may construct two dictionaries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question to ask here is whether we need to create this global cache in the first place.

// and overwrite one - this is ok since the dictionaries are read-only once constructed and they contain the identical data.

LowLevelDictionary<string, LowLevelList<ResourceInfo>> dict = new LowLevelDictionary<string, LowLevelList<ResourceInfo>>();
Dictionary<string, List<ResourceInfo>> dict = new Dictionary<string, List<ResourceInfo>>();

foreach (NativeFormatModuleInfo module in ModuleList.EnumerateModules())
{
Expand All @@ -120,10 +120,10 @@ private static LowLevelDictionary<string, LowLevelList<ResourceInfo>> ExtractedR

ResourceInfo resourceInfo = new ResourceInfo(resourceName, resourceOffset, resourceLength, module);

LowLevelList<ResourceInfo> assemblyResources;
List<ResourceInfo> assemblyResources;
if (!dict.TryGetValue(assemblyName, out assemblyResources))
{
assemblyResources = new LowLevelList<ResourceInfo>();
assemblyResources = new List<ResourceInfo>();
dict[assemblyName] = assemblyResources;
}

Expand All @@ -144,7 +144,7 @@ private static LowLevelDictionary<string, LowLevelList<ResourceInfo>> ExtractedR
/// The dictionary's key is a Fusion-style assembly name.
/// The dictionary's value is a list of (resourcename,index) tuples.
/// </summary>
private static volatile LowLevelDictionary<string, LowLevelList<ResourceInfo>> s_extractedResourceDictionary;
private static volatile Dictionary<string, List<ResourceInfo>> s_extractedResourceDictionary;

private struct ResourceInfo
{
Expand Down
3 changes: 0 additions & 3 deletions ...iveaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@
<Link>System\NotImplemented.cs</Link>
</Compile>
<Compile Include="$(LibrariesProjectRoot)\System.Private.CoreLib\src\System\SR.cs" />
<Compile Include="$(AotCommonPath)\System\Collections\Generic\LowLevelList.cs">
<Link>System\Collections\Generic\LowLevelList.cs</Link>
</Compile>
<Compile Include="$(AotCommonPath)\System\Collections\Generic\LowLevelDictionary.cs">
<Link>System\Collections\Generic\LowLevelDictionary.cs</Link>
</Compile>
Expand Down
18 changes: 9 additions & 9 deletions ...eclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/EETypeCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,9 +457,9 @@ private static unsafe int GetInstanceGCDescSize(TypeBuilderState state, MethodTa
}
}

private static bool IsAllGCPointers(LowLevelList<bool> bitfield)
private static bool IsAllGCPointers(bool[] bitfield)
{
int count = bitfield.Count;
int count = bitfield.Length;
Debug.Assert(count > 0);

for (int i = 0; i < count; i++)
Expand All @@ -471,7 +471,7 @@ private static bool IsAllGCPointers(LowLevelList<bool> bitfield)
return true;
}

private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int rank, bool isSzArray, void* gcdesc)
private static unsafe int CreateArrayGCDesc(bool[] bitfield, int rank, bool isSzArray, void* gcdesc)
{
if (bitfield == null)
return 0;
Expand All @@ -495,7 +495,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
int first = -1;
int last = 0;
short numPtrs = 0;
while (i < bitfield.Count)
while (i < bitfield.Length)
{
if (bitfield[i])
{
Expand All @@ -513,7 +513,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
numSeries++;
numPtrs = 0;

while ((i < bitfield.Count) && (bitfield[i]))
while ((i < bitfield.Length) && (bitfield[i]))
{
numPtrs++;
i++;
Expand All @@ -531,7 +531,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
{
if (numSeries > 0)
{
*ptr-- = (short)((first + bitfield.Count - last) * IntPtr.Size);
*ptr-- = (short)((first + bitfield.Length - last) * IntPtr.Size);
*ptr-- = numPtrs;

*(void**)gcdesc = (void*)-numSeries;
Expand All @@ -542,7 +542,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
return numSeries;
}

private static unsafe int CreateGCDesc(LowLevelList<bool> bitfield, int size, bool isValueType, bool isStatic, void* gcdesc)
private static unsafe int CreateGCDesc(bool[] bitfield, int size, bool isValueType, bool isStatic, void* gcdesc)
{
int offs = 0;
// if this type is a class we have to account for the gcdesc.
Expand All @@ -558,15 +558,15 @@ private static unsafe int CreateGCDesc(LowLevelList<bool> bitfield, int size, bo

int numSeries = 0;
int i = 0;
while (i < bitfield.Count)
while (i < bitfield.Length)
{
if (bitfield[i])
{
numSeries++;
int seriesOffset = i * IntPtr.Size + offs;
int seriesSize = 0;

while ((i < bitfield.Count) && (bitfield[i]))
while ((i < bitfield.Length) && (bitfield[i]))
{
seriesSize += IntPtr.Size;
i++;
Expand Down
Loading
Loading