-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Avoid generic virtual dispatch for frozen collections alternate lookup #108732
Avoid generic virtual dispatch for frozen collections alternate lookup #108732
Conversation
Tagging subscribers to this area: @dotnet/area-system-collections |
@stephentoub I would greatly appreciate any feedback on this PR if you have the time |
@adamsitnik I would greatly appreciate any feedback on this PR if you have the time |
@andrewjsaid Apologies for the delay in getting this reviewed. We will get back to this as soon as possible. |
...llections.Immutable/src/System/Collections/Frozen/DefaultFrozenDictionary.AlternateLookup.cs
Show resolved
Hide resolved
...stem.Collections.Immutable/src/System/Collections/Frozen/DefaultFrozenSet.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...llections.Immutable/src/System/Collections/Frozen/DefaultFrozenDictionary.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...llections.Immutable/src/System/Collections/Frozen/DefaultFrozenDictionary.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...stem.Collections.Immutable/src/System/Collections/Frozen/DefaultFrozenSet.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...stem.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...stem.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.AlternateLookup.cs
Show resolved
Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs
Show resolved
Hide resolved
...tions.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenDictionary.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
...tions.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenDictionary.AlternateLookup.cs
Show resolved
Hide resolved
...tions.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenDictionary.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
....Collections.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenSet.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
....Collections.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenSet.AlternateLookup.cs
Outdated
Show resolved
Hide resolved
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.
This generally looks reasonable. Thanks for the improvement.
@stephentoub I've completed all the changes and am awaiting next steps. |
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.
Thanks.
@adamsitnik or @eiriktsarpalis , can you please take a look as well? Thanks.
/ba-g unrelated interop failure |
* main: (31 commits) Fix linux-x86 build (dotnet#111861) Add FrozenDictionary specialization for integers / enums (dotnet#111886) [SRM] Refactor reading from streams. (dotnet#111323) Sign the DAC and DBI during the build process instead of in separate steps (dotnet#111416) Removing Entry2MethodDesc as it is unnecessary (dotnet#111756) Cross Product for Vector2 and Vector4 (dotnet#111265) Handle unicode in absolute URI path for combine. (dotnet#111710) Drop RequiresProcessIsolation on mcc tests (dotnet#111887) [main] Update dependencies from dotnet/roslyn (dotnet#111691) new trimmer feature System.TimeZoneInfo.Invariant (dotnet#111215) [browser] reduce msbuild memory footprint (dotnet#111751) Add debugging checks for stack overflow tests failure (dotnet#111867) Localized file check-in by OneLocBuild Task: Build definition ID 679: Build ID 2629821 (dotnet#111884) Bump main to preview2 (dotnet#111882) Avoid generic virtual dispatch for frozen collections alternate lookup (dotnet#108732) Bump main versioning to preview1 (dotnet#111880) Switch OneLoc to main (dotnet#111872) Improve docs on building ILVerify (dotnet#111851) Update Debian version to 13 (dotnet#111768) win32: add fallback to environment vars for system folder (dotnet#109673) ...
Background
In .NET 9 the Alternate Lookup concept was added to dictionaries where you could look up in a dictionary by a key type different than the original one. The main motivator was reading data off the wire e.g. keying a
Dictionary<string, T>
using aReadOnlySpan<char>
(orbyte
). Thus I created a PR in ASP.NET Core to use this new method onFrozenDictionary<string, T>
when matching routing but was surprised to find that there's quite a hefty penalty when using Alternate Lookups due to the generic virtual dispatch mechanism itself being a dictionary. Thus while the memory allocations disappeared the performance in terms of speed improved only slightly in the microbenchmarks.See dotnet/aspnetcore#58305
Solution
FrozenDictionary<string, T>
currently exposes a method with the following signature where the actual work happens.To replace it, a new mechanism was added:
The idea is that when the
AlternateLookup
struct is created it pays the price of the generic virtual dispatch once. It can then use the delegate as follows to avoid generic virtual dispatch:In concrete types of
FrozenDictionary
we can then use the following pattern to cache a static instance of the delegate which calls the actual worker method.In the above example
GetAlternateLookupDelegate<TAlternateKey>
simply loads a singleton delegate from a nested generic type. The delegate is implemented by callingGetValueRefOrNullRefCoreAlternate<TAlternateKey
which although generic importantly is not a virtual method.In order to avoid repeating this pattern for all the classes inheriting from
OrdinalStringFrozenDictionary
, a different pattern is used there. Note that in this case we do not need generics as we know that the concrete type ofTAlternateKey
will beReadOnlySpan<char>
, a fact which is asserted when creating the lookup.In the above example we are able to remove the generics from the worker method, and thus we can make it virtual - still avoiding the combination of generics and virtual dispatch. Thus the derived types can use the existing approach as follows.
Note that further investigation is necessary to measure whether it is a good idea to remove the virtual dispatch entirely even for classes inheriting
OrdinalStringFrozenDictionary
. It would result in more complex code but could be faster depending on how expensive the (non-generic) virtual dispatch is.Conclusion
As we can see from the benchmarks below, the PR does not change the "normal"
TryGetValue
scenario and improves the "Alternate Lookup"TryGetValue
scenario significantly.This way the alternate lookup still suffers a performance penalty but it's not quite as significant and is more likely to justify it's use if it results in fewer allocations.
Benchmark Results
dotnet/performance Frozen Dictionary benchmarks (https://github.com/dotnet/performance/blob/main/src/benchmarks/micro/libraries/System.Collections/Frozen/Perf_FrozenDictionary_String.cs) were modified locally to add a case to use the AlternateLookup feature. I didn't create a PR as I couldn't find a clean way to add it with conditional compilation for .NET 9+
DefaultFrozenDictionary
LengthBucketsFrozenDictionary
SingleCharFrozenDictionary
SubstringFrozenDictionary