Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ public ArrayInitializeCache(RuntimeType arrayType)
// it for type <T> and executes it.
//
// The "T" will reflect the interface used to invoke the method. The actual runtime "this" will be
// array that is castable to "T[]" (i.e. for primitivs and valuetypes, it will be exactly
// array that is castable to "T[]" (i.e. for primitives and valuetypes, it will be exactly
// "T[]" - for orefs, it may be a "U[]" where U derives from T.)
//----------------------------------------------------------------------------------------
internal sealed class SZArrayHelper
Expand Down
19 changes: 2 additions & 17 deletions src/coreclr/vm/array.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1235,29 +1235,14 @@ MethodDesc* GetActualImplementationForArrayGenericIListOrIReadOnlyListMethod(Met
}
CONTRACTL_END

int slot = pItfcMeth->GetSlot();

// We need to pick the right starting method depending on the depth of the inheritance chain
static const BinderMethodID startingMethod[] = {
METHOD__SZARRAYHELPER__GETENUMERATOR, // First method of IEnumerable`1
METHOD__SZARRAYHELPER__GET_COUNT, // First method of ICollection`1/IReadOnlyCollection`1
METHOD__SZARRAYHELPER__GET_ITEM // First method of IList`1/IReadOnlyList`1
};

// Subtract one for the non-generic IEnumerable that the generic enumerable inherits from
unsigned int inheritanceDepth = pItfcMeth->GetMethodTable()->GetNumInterfaces() - 1;
PREFIX_ASSUME(0 <= inheritanceDepth && inheritanceDepth < ARRAY_SIZE(startingMethod));

MethodDesc *pGenericImplementor = CoreLibBinder::GetMethod((BinderMethodID)(startingMethod[inheritanceDepth] + slot));

// The most common reason for this assert is that the order of the SZArrayHelper methods in
// corelib.h does not match the order they are implemented on the generic interfaces.
_ASSERTE(pGenericImplementor == MemberLoader::FindMethodByName(g_pSZArrayHelperClass, pItfcMeth->GetName()));
MethodDesc *pGenericImplementor = MemberLoader::FindMethodByName(g_pSZArrayHelperClass, pItfcMeth->GetName());

// OPTIMIZATION: For any method other than GetEnumerator(), we can safely substitute
// "Object" for reference-type theT's. This causes fewer methods to be instantiated.
if (startingMethod[inheritanceDepth] != METHOD__SZARRAYHELPER__GETENUMERATOR &&
!theT.IsValueType())
if (inheritanceDepth != 1 && !theT.IsValueType())
Copy link
Member

@tannergooding tannergooding Mar 2, 2024

Choose a reason for hiding this comment

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

Shouldn't this be inheritanceDepth != 0?

My understanding is that previously we had:

  • IList<T> -> ICollection<T> -> IEnumerable<T> -> IEnumerable
  • IReadOnlyList<T> -> IReadOnlyCollection<T> -> IEnumerable<T> -> IEnumerable

So in both cases we have a maximum inheritance depth of 4. We then subtract one so that we're effectively ignoring the existence of IEnumerable, since this is only being used for the synthesized generic interfaces.

Since we're still subtracting 1 from the depth above, we want to check against 0 to preserve the optimization.


In the new world, we have:

          /-> ICollection<T>   -\
IList<T> -                       -> IReadOnlyCollection<T> -> IEnumerable<T> -> IEnumerable
          \-> IReadOnlyList<T> -/

So IEnumerable, IEnumerable<T>, IReadOnlyCollection<T>, and IReadOnlyList<T> should remain at -1, 0, 1, and 2 respectively (after the adjustment we're doing on L1214).

While ICollection<T> has had its adjusted depth bumped by 1 from 1 to 2, sharing the slot with IReadOnlyList<T>. Similarly, IList<T> has been bumped from 2 to 3 remaining a unique slot.

Thus, the lookup optimization can be preserved for any slot other than 2 where ICollection<T> and IReadOnlyList<T> need something additional to disambiguate them.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed up the one line fix here. Tests looked to generally be passing locally with it resolved.

I think we could add back in the lookup optimization as well, but left that out.

This comment was marked as outdated.

{
theT = TypeHandle(g_pObjectClass);
}
Expand Down
152 changes: 152 additions & 0 deletions src/libraries/Common/tests/System/Collections/CollectionAsserts.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using Xunit;
Expand All @@ -9,6 +10,151 @@ namespace System.Collections.Tests
{
internal static class CollectionAsserts
{
public static void HasCount<T>(ICollection<T> collection, int count)
{
Assert.Equal(count, collection.Count);
#if !NETFRAMEWORK
IReadOnlyCollection<T> readOnlyCollection = collection;
Assert.Equal(count, readOnlyCollection.Count);
#endif
}

public static void EqualAt<T>(IList<T> list, int index, T expected)
{
Assert.Equal(expected, list[index]);
#if !NETFRAMEWORK
IReadOnlyList<T> readOnlyList = list;
Assert.Equal(expected, readOnlyList[index]);
#endif
}

public static void NotEqualAt<T>(IList<T> list, int index, T expected)
{
Assert.NotEqual(expected, list[index]);
#if !NETFRAMEWORK
IReadOnlyList<T> readOnlyList = list;
Assert.NotEqual(expected, readOnlyList[index]);
#endif
}

public static void ThrowsElementAt<T>(IList<T> list, int index, Type exceptionType)
{
Assert.Throws(exceptionType, () => list[index]);
#if !NETFRAMEWORK
IReadOnlyList<T> readOnlyList = list;
Assert.Throws(exceptionType, () => readOnlyList[index]);
#endif
}

public static void ElementAtSucceeds<T>(IList<T> list, int index)
{
T result = list[index];
#if !NETFRAMEWORK
IReadOnlyList<T> readOnlyList = list;
Assert.Equal(result, readOnlyList[index]);
#endif
}

public static void EqualAt<TKey, TValue>(IDictionary<TKey, TValue> dictionary, TKey key, TValue expected)
{
Assert.Equal(expected, dictionary[key]);
#if !NETFRAMEWORK
IReadOnlyDictionary<TKey, TValue> readOnlyDictionary = dictionary;
Assert.Equal(expected, readOnlyDictionary[key]);
#endif
}

public static void ContainsKey<TKey, TValue>(IDictionary<TKey, TValue> dictionary, TKey key, bool expected)
{
Assert.Equal(expected, dictionary.ContainsKey(key));
#if !NETFRAMEWORK
IReadOnlyDictionary<TKey, TValue> readOnlyDictionary = dictionary;
Assert.Equal(expected, readOnlyDictionary.ContainsKey(key));
#endif
}

public static void TryGetValue<TKey, TValue>(IDictionary<TKey, TValue> dictionary, TKey key, bool expected, TValue expectedValue = default)
{
Assert.Equal(expected, dictionary.TryGetValue(key, out TValue value));
if (expected)
{
Assert.Equal(expectedValue, value);
}
#if !NETFRAMEWORK
IReadOnlyDictionary<TKey, TValue> readOnlyDictionary = dictionary;
Assert.Equal(expected, readOnlyDictionary.TryGetValue(key, out value));
if (expected)
{
Assert.Equal(expectedValue, value);
}
#endif
}

public static void Contains<T>(ISet<T> set, T expected)
{
Assert.True(set.Contains(expected));
#if !NETFRAMEWORK
ICollection<T> collection = set;
Assert.True(collection.Contains(expected));
IReadOnlySet<T> readOnlySet = set;
Assert.True(readOnlySet.Contains(expected));
#endif
}

public static void IsProperSubsetOf<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.IsProperSubsetOf(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.IsProperSubsetOf(enumerable));
#endif
}

public static void IsProperSupersetOf<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.IsProperSupersetOf(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.IsProperSupersetOf(enumerable));
#endif
}

public static void IsSubsetOf<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.IsSubsetOf(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.IsSubsetOf(enumerable));
#endif
}

public static void IsSupersetOf<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.IsSupersetOf(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.IsSupersetOf(enumerable));
#endif
}

public static void Overlaps<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.Overlaps(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.Overlaps(enumerable));
#endif
}

public static void SetEquals<T>(ISet<T> set, IEnumerable<T> enumerable, bool expected)
{
Assert.Equal(expected, set.SetEquals(enumerable));
#if !NETFRAMEWORK
IReadOnlySet<T> readOnlySet = set;
Assert.Equal(expected, readOnlySet.SetEquals(enumerable));
#endif
}

public static void Equal(ICollection expected, ICollection actual)
{
Assert.Equal(expected == null, actual == null);
Expand Down Expand Up @@ -43,6 +189,12 @@ public static void Equal<T>(ICollection<T> expected, ICollection<T> actual)
return;
}
Assert.Equal(expected.Count, actual.Count);
#if !NETFRAMEWORK
IReadOnlyCollection<T> readOnlyExpected = expected;
Assert.Equal(expected.Count, readOnlyExpected.Count);
IReadOnlyCollection<T> readOnlyActual = actual;
Assert.Equal(actual.Count, readOnlyActual.Count);
#endif
IEnumerator<T> e = expected.GetEnumerator();
IEnumerator<T> a = actual.GetEnumerator();
while (e.MoveNext())
Expand Down
Loading