-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Enable RuntimeType.GenericCache to hold multiple types of cache entries #102034
Enable RuntimeType.GenericCache to hold multiple types of cache entries #102034
Conversation
…down visibility of some of the types/members
Tagging subscribers to this area: @dotnet/area-system-runtime |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
/benchmark micro aspnet-perf-win runtime --variable filter=System.Tests.Perf_Array |
/benchmark micro aspnet-perf-win runtime --variable filter=System.Tests.Perf_Enum |
/benchmark micro aspnet-perf-win runtime --variable filter=System.Reflection.Activator |
Benchmark started for micro on aspnet-perf-win with runtime and arguments |
micro - aspnet-perf-win
| benchmark | mean (micro.base) | mean (micro.pr) | ratio | allocated (micro.base) | allocated (micro.pr) | ratio |
| -------------------------------------------- | ----------------- | --------------- | ----- | ---------------------- | -------------------- | ----- |
| Activator<EmptyClass>.CreateInstanceGeneric | 7.302 ns | 70.649 ns | 9.67 | 24 B | 56 B | 2.33 |
| Activator<EmptyClass>.CreateInstanceNames | 1.5 μs | 1.5 μs | 1.00 | 920 B | 920 B | 1.00 |
| Activator<EmptyClass>.CreateInstanceType | 6.434 ns | 8.138 ns | 1.26 | 24 B | 24 B | 1.00 |
| Activator<EmptyStruct>.CreateInstanceGeneric | 0.0001 ns | 0.0000 ns | 0.20 | 0 B | 0 B | |
| Activator<EmptyStruct>.CreateInstanceNames | 1.3 μs | 1.3 μs | 1.04 | 744 B | 744 B | 1.00 |
| Activator<EmptyStruct>.CreateInstanceType | 5.630 ns | 6.893 ns | 1.22 | 24 B | 24 B | 1.00 | |
Benchmark started for micro on aspnet-perf-win with runtime and arguments |
Return a ref into the boxed composite entry to make sure we're actually updating the composite entry.
…ons (faster in this case)
I have run this micro-benchmark on Win x64 to see the perf of composite cache: using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
var sw = new Stopwatch();
for (;;)
{
sw.Restart();
Work();
Internal.Console.WriteLine(sw.ElapsedMilliseconds.ToString());
}
static void Work()
{
for (int i = 0; i < 100000000; i++)
{
GC.KeepAlive(Activator.CreateInstance(typeof(object)));
GC.KeepAlive(RuntimeHelpers.GetUninitializedObject(typeof(object)));
}
} It runs about 1.3x-1.4x slower compared to current main. These seems to be a lot of perf lost in the generics. Can we do better? |
I'll see if I can make it faster. I've been focusing on matching performance with our existing (single-entry-focused) microbenchmarks. I've been able to get within single-digit percentage (even with a possible improvement in one run) for Activator.CreateInstance. I'll spend some time on the composite case. |
I have tried to replace the InlineArray with explicit fields jkotas@1a96a78 . It fixed the perf for my micro-benchmark. (The change is not complete - it is missing handling of EnumInfo.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/RuntimeType.GenericCache.cs
Outdated
Show resolved
Hide resolved
@jkotas can you take one last review pass on this? Looks like Build Analysis is green so it’s ready to merge once it’s approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Enable GenericCache to hold multiple types of cache entries in preparation for some boxing optimizations that can use the cache. Try to encapsulate the design so the users of the
GenericCache
don't need to think about multiple cache entries.