-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Speed up case-insensitive Enum parsing #131030
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |||||||||||||||
| using System.Runtime; | ||||||||||||||||
| using System.Runtime.CompilerServices; | ||||||||||||||||
| using System.Runtime.InteropServices; | ||||||||||||||||
| using System.Threading; | ||||||||||||||||
|
|
||||||||||||||||
| namespace System.Reflection | ||||||||||||||||
| { | ||||||||||||||||
|
|
@@ -41,6 +42,39 @@ public EnumInfo(Type underlyingType, TStorage[] values, string[] names, bool isF | |||||||||||||||
| internal TStorage[] Values { get; } | ||||||||||||||||
| internal bool ValuesAreSequentialFromZero { get; } | ||||||||||||||||
|
|
||||||||||||||||
| // Lazily-built, cached case-insensitive name-to-value lookup used to accelerate | ||||||||||||||||
| // case-insensitive parsing. Null until the first case-insensitive parse of this enum type. | ||||||||||||||||
| private Dictionary<string, TStorage>? _namesToValuesIgnoreCase; | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can store the result of The difference in the fixed per-lookup overhead may also change where the crossover between a linear scan vs. dictionary would be. |
||||||||||||||||
|
|
||||||||||||||||
| /// <summary> | ||||||||||||||||
| /// Gets a case-insensitive name-to-value lookup, building and caching it on first use. | ||||||||||||||||
| /// </summary> | ||||||||||||||||
| public Dictionary<string, TStorage> GetNamesToValuesIgnoreCase() | ||||||||||||||||
| { | ||||||||||||||||
| return Volatile.Read(ref _namesToValuesIgnoreCase) ?? Initialize(); | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| Dictionary<string, TStorage> Initialize() | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These extra dictionary instantiations cost up to 70kB binary size of Native AOT compiled apps: MichalStrehovsky/rt-sz#242 It should be acceptable binary size regression.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is it so much since we get like 8 instantiations max for the common
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can drill into it with sizescope if you really want to know. My guess is that there are unrelated places in the app that use interfaces implemented by Dictionary, like |
||||||||||||||||
| { | ||||||||||||||||
| string[] names = Names; | ||||||||||||||||
| TStorage[] values = Values; | ||||||||||||||||
| var lookup = new Dictionary<string, TStorage>(names.Length, StringComparer.OrdinalIgnoreCase); | ||||||||||||||||
|
Comment on lines
+52
to
+60
|
||||||||||||||||
|
|
||||||||||||||||
| // Insert in Names order (which is sorted by value). For names that differ only by case, | ||||||||||||||||
| // TryAdd keeps the first (lowest-value) entry, matching the original linear scan's | ||||||||||||||||
| // "first match wins" behavior. | ||||||||||||||||
| for (int i = 0; i < names.Length; i++) | ||||||||||||||||
| { | ||||||||||||||||
| lookup.TryAdd(names[i], values[i]); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Publish atomically. If another thread raced and already published a lookup, | ||||||||||||||||
| // reuse theirs so every caller observes a single cached instance. The fast-path | ||||||||||||||||
| // Volatile.Read pairs an acquire load with this release publish so readers always | ||||||||||||||||
| // observe a fully-initialized dictionary. | ||||||||||||||||
| return Interlocked.CompareExchange(ref _namesToValuesIgnoreCase, lookup, null) ?? lookup; | ||||||||||||||||
|
Comment on lines
+70
to
+74
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There should be no need to synchronize here, we're the only caller and we don't care which exact instance we got |
||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// <summary>Create a copy of <see cref="Values"/>.</summary> | ||||||||||||||||
| public unsafe TResult[] CloneValues<TResult>() where TResult : struct | ||||||||||||||||
| { | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| #define RARE_ENUMS | ||
| #endif | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Globalization; | ||
|
|
@@ -1052,6 +1053,12 @@ private static bool TryParseByName<TStorage>(RuntimeType enumType, ReadOnlySpan< | |
|
|
||
| bool parsed = true; | ||
| TStorage localResult = default; | ||
|
|
||
| // For case-insensitive parsing, use a cached name->value lookup and match spans without allocating. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is counter-intuitive that case-insensitive is faster with this change, but case-sensitive is not. The general expectation is that case-sensitive operations are faster. Should this be done for case-sensitive parsing as well? |
||
| Dictionary<string, TStorage>.AlternateLookup<ReadOnlySpan<char>> ignoreCaseLookup = ignoreCase ? | ||
| enumInfo.GetNamesToValuesIgnoreCase().GetAlternateLookup<ReadOnlySpan<char>>() : | ||
| default; | ||
|
|
||
| while (value.Length > 0) | ||
|
Comment on lines
+1057
to
1062
|
||
| { | ||
| // Find the next separator. | ||
|
|
@@ -1080,14 +1087,10 @@ private static bool TryParseByName<TStorage>(RuntimeType enumType, ReadOnlySpan< | |
| bool success = false; | ||
| if (ignoreCase) | ||
| { | ||
| for (int i = 0; i < enumNames.Length; i++) | ||
| if (ignoreCaseLookup.TryGetValue(subvalue, out TStorage matchedValue)) | ||
| { | ||
| if (subvalue.EqualsOrdinalIgnoreCase(enumNames[i])) | ||
| { | ||
| localResult |= enumValues[i]; | ||
| success = true; | ||
| break; | ||
| } | ||
| localResult |= matchedValue; | ||
| success = true; | ||
| } | ||
| } | ||
| else | ||
|
|
||
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.