Optimize type check efficiency with .IsValueType #82305
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
|
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis pull request replaces cases of default(T) != null with typeof(T).IsValueType for better assembly performance. Before After Fix #82291
|
| @@ -1742,7 +1742,7 @@ protected override sealed bool ItemsMustBeUnique | |||
|
|
|||
| protected override sealed bool ItemsMustBeNonNull | |||
| { | |||
| get { return default(T) != null; } | |||
| get { return typeof(T).IsValueType; } | |||
There was a problem hiding this comment.
This is a test; it doesn't need to be changed.
| @@ -19,7 +19,7 @@ internal static bool IsValueType<T>() | |||
| #if NETCOREAPP | |||
| return typeof(T).IsValueType; | |||
| #else | |||
| if (default(T) != null) | |||
| if (typeof(T).IsValueType) | |||
There was a problem hiding this comment.
This is a downlevel path. This may be used in runtimes where typeof(T).IsValueType isn't an intrinsic and isn't optimized. Note that this is part of an #else where the #if NETCOREAPP already uses IsValueType.
| @@ -78,7 +78,7 @@ public void Comparer_IComparerCompareWithObjectsNotOfMatchingTypeShouldThrow() | |||
| // throw if both inputs are non-null and one of them is not of type T | |||
| IComparer comparer = Comparer<T>.Default; | |||
| StrongBox<T> notOfTypeT = new StrongBox<T>(default(T)); | |||
| if (default(T) != null) // if default(T) is null these asserts will fail as IComparer.Compare returns early if either side is null | |||
| if (typeof(T).IsValueType) // if default(T) is null these asserts will fail as IComparer.Compare returns early if either side is null | |||
There was a problem hiding this comment.
This is a test and doesn't need to be changed. (The comment would also be stale if it were changed.)
| @@ -85,7 +85,7 @@ public void EqualityComparer_IEqualityComparerEqualsWithObjectsNotOfMatchingType | |||
| Assert.Equal(default(T) == null, comparer.Equals(null, default(T))); | |||
| Assert.True(comparer.Equals(null, null)); | |||
|
|
|||
| if (default(T) != null) // if default(T) is null this assert will fail as IEqualityComparer.Equals returns early if either input is null | |||
| if (typeof(T).IsValueType) // if default(T) is null this assert will fail as IEqualityComparer.Equals returns early if either input is null | |||
There was a problem hiding this comment.
This is a test and doesn't need to be changed.
| @@ -451,7 +451,7 @@ private static bool IsCompatibleObject(object? value) | |||
|
|
|||
| private static void ValidateNullValue(object? value, string argument) | |||
| { | |||
| if (value == null && default(T) != null) | |||
| if (value == null && typeof(T).IsValueType) | |||
There was a problem hiding this comment.
This will be wrong for Nullable<T>, e.g. if value is null and typeof(T) is typeof(int?), then the code previously wouldn't have thrown but now it will.
| @@ -31,7 +31,7 @@ internal static class AggregationMinMaxHelpers<T> | |||
|
|
|||
| AssociativeAggregationOperator<T, Pair<bool, T>, T> aggregation = | |||
| new AssociativeAggregationOperator<T, Pair<bool, T>, T>(source, new Pair<bool, T>(false, default!), null, | |||
| true, intermediateReduce, finalReduce, resultSelector, default(T) != null, QueryAggregationOptions.AssociativeCommutative); | |||
| true, intermediateReduce, finalReduce, resultSelector, typeof(T).IsValueType, QueryAggregationOptions.AssociativeCommutative); | |||
| @@ -232,7 +232,7 @@ public static unsafe void Fill<T>(ref T refData, nuint numElements, T value) | |||
|
|
|||
| nint index = 0; // Use nint for arithmetic to avoid unnecessary 64->32->64 truncations | |||
|
|
|||
| if (default(T) != null || (object?)value != null) | |||
There was a problem hiding this comment.
Do these changes really have a material impact on the codegen? I wouldn't expect them to. The before/after in the opening post where there's poorer code gen isn't for this case.
Thanks for your interest in contributing! This doesn't actually fix that issue, though. That issue is about code generated by the JIT in certain cases; whether or not we use those patterns in our own code, others might use them as well, so the fix there is actually in the JIT. |
Thank you for the quick feedback! That makes a lot of sense. At this point, would you say it is best to close this pull request? |
I think so. But as I said, thank you for your interest in contributing, and we'd welcome other contributions in the future if you're interested. |
|
Thank you again for your patience. This is a great community. I saw the help-wanted tag and expected it to be an easy fix, but I guess I would need to first reach a deep understanding of the codebase. I will definitely keep an eye on other help-wanted issues and put more thought into my contributions. Issue closed. |
What
This pull request replaces cases of default(T) != null with typeof(T).IsValueType for better assembly performance, as mentioned in #82291.
Effect
Before
After
This is my first time contributing to open source projects. Please let me know if there are any formatting issues.
Fix #82291