Speed up KeyAnalyzer for substring based frozen collections

[IDisposable]

Building on PR #89689, I tried making everything as explicitly inline-able and sealed to get the JIT to eliminate any conditional jumps in the hot loop by teasing out the various comparator options into trait-like classes to mix in to get sealed classes where all the logic in conditional-free (down to the internals of the String.AsSpan() methods themselves.

This required making some complex extractions. The essence is that now we construct an equality comparer and hash set for the from-the-left attempt, and then lazily construct another comparer and matching HashSet<string> only if we need one for from-the-right attempt. Since in the vast number of cases, we're going to spend the bulk of the execution time iterating through the uniqueStrings we're presented with, the overhead of having another comparer/collection is pretty low in comparison.

To ensure we don't bloat memory, we now .Clear() the set just before we return false; from HasSufficientUniquenessFactor since we know that we're going to get called again for a different index/count... this means that both HashSet<string>s spend their lifetimes empty except in mid-check (we could clear them on success, as well, but the old code didn't bother).

The resulting SubstringEqualityComparer classes could be useful elsewhere, but are currently just internal to the System.Collections.Immutable assembly. We could easily extend them to also support the InvariantCulture and CurrentCulture options, but not sure when we would want to use either (it wouldn't make much sense for a FrozenDictionary/FrozenSet, but there could be other use-cases).

Hoisted the calculation of the maximum offset per-pass up to the top of the count (length loop), because that calculation applies to both left-justified and right-justified, and also doesn't change during the inner loops.

The comparers expose an interface that internally manipulates the starting-index and count in such a way that it gets in-lined completely. Basically you tell it where to Start() when you change the count (width) of the slice, such that when doing a pass of count width in a from-the-left pass, you start with index of zero. When doing a from-the-right pass, you start with the negative count offset (e.g. as far right as will accommodate the slice width) . Once you start, you can GoLeft() or GoRight() to bump the index appropriately (without field/property calls).

The implementation uses:

• Static "mix-in" Slicer methods for LeftSlice and RightSlice to extract the slice's ReadOnlySpan<char>
• Static "mix-in" OrdinalEquality for Equals and GetHashCode operations to use when implementing IEqualityComparer<string> in an ordinal case-sensitive way
• Static "mix-in" OrdinalInsensitiveEquality for Equals and GetHashCode operations to use when implementing IEqualityComparer<string> in an ordinal case-insensitive way
• Sealed concrete encapsulations of the mix-ins for LeftSubstringOrdinalComparer, RightSubstringOrdinalComparer, LeftSubstringCaseInsensitiveComparer, and RightSubstringCaseInsensitiveComparer
• Sealed concrete FullStringComparer for use when we decide that we aren't going to use substrings at all.
• Use of the IGenericSpecializedWrapper / GSW pattern to "pass in" the specialization to eliminate all virtcalls and can be completely inlined
• Aggressive inline of the slicers/comparers via liberal application of [MethodImpl(MethodImplOptions.AggressiveInlining)]

I suspect we could pass the chosen slicer out in the AnalysisResults as it encapsulates the offset, length, and ignore/honor case for use in the resulting Frozen collection constructions, but that would be another pass.

I also added some more tests to the KeyAnalyserTests for ContainsAnyLetters and HasSufficientUniquenessFactor to verify newly exposed behavior and also added some testing based on the known instances of freezable data that is being used in the FrozenFromKnownValuesTests.

The other changes imported from PR #88516 here are:

• Eliminate the the inner TryUseSubstring because we can just early return the calculated results as we build them
• Hoist the calculation of acceptableNonUniqueCount out to the top level since it never changes (which means we pass that into the HasSufficientUniquenessFactor method for it to "use up" internally (passed by value, so unchanged at call-site)
• Eliminated the delegate ReadOnlySpan<char> GetSpan and use, which helps reduce dynamic dispatch overhead in the CreateAnalysisResults method
• Eliminated the IsLeft field of the SubstringComparer since we can tell by the Index being negative that we're doing right-justified slicing (and documented that on the class)
• Changed the logic managing the Index and Count on the comparer for right-justified substrings.
  Added [MethodImpl(MethodImplOptions.AggressiveInlining)] to the Equals and GetHashCode overrides.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Building on PR #88516, I tried making everything as explicitly inline-able and sealed to get the JIT to eliminate any conditional jumps in the hot loop by teasing out the various comparator options into trait-like classes to mix in to get sealed classes where all the logic in conditional-free (down to the internals of the String.AsSpan() methods themselves.

This required making some complex extraction the essence is that now we construct an equality comparer and hash set for the from-the-left attempt, and then lazily construct another comparer and matching hash set only if we need one for from-the-right attempt. Since in the vast number of cases, we're going to spend the bulk of the execution time iterating through the uniqueStrings we're presented with, the overhead of having another comparer/collection is pretty low in comparison.

To ensure we don't bloat memory, we now .Clear() the set to right before we return false; from HasSufficientUniquenessFactor since we know that we're going to get called again for a different index/count... this means that both HashSet<string>s spend their lifetimes empty except in mid-check (we could clear them on success, as well, but the old code didn't bother).

The resulting SubstringEqualityComparer classes could be useful elsewhere, but are currently just internal to the System.Collections.Immutable assembly. We could easily extend them to also support the InvariantCulture and CurrentCulture options, but not sure when we would want to use either (it wouldn't make much sense for a FrozenDictionary/FrozenSet, but there could be other use-cases).

Also hoisted the calculation of the maximum offset per-pass up to the top of the count (length loop), because that calculation applies to both left-justified and right-justified, and also doesn't change during the inner loops.

The comparers expose an interface that internally manipulates the starting-index and count in such a way that it gets in-lined completely with no virtcalls. Basically you tell it where to Start() when you change the count (width) of the slice, such that when doing a pass of count width in a from-the-left pass, you start with index of zero. When doing a from-the-right pass, you start with the negative count offset (e.g. as far right as will accommodate the slice width) . Once you start, you can GoLeft() or GoRight() to bump the index appropriately (without field/property virtcalls).

The implementation uses:

• Static "mix-in" Slicer methods for LeftSlice and RightSlice to extract the slice's ReadOnlySpan<char>
• Static "mix-in" OrdinalEquality for Equals and GetHashCode operations to use when implementing IEqualityComparer<string> in an ordinal case-sensitive way
• Static "mix-in" OrdinalInsensitiveEquality for Equals and GetHashCode operations to use when implementing IEqualityComparer<string> in an ordinal case-insensitive way
• Sealed concrete encapsulations of the mix-ins for LeftSubstringOrdinalComparer, RightSubstringOrdinalComparer, LeftSubstringCaseInsensitiveComparer, and RightSubstringCaseInsensitiveComparer
• Sealed concrete FullStringComparer for use when we decide that we aren't going to use substrings at all.
• Use of the IGenericSpecializedWrapper / GSW pattern to ensure all calls are de-virtualized and can be completely inlined
• Aggressive inline of the slicers/comparers via liberal application of [MethodImpl(MethodImplOptions.AggressiveInlining)]

I suspect we could/should pass them out in the AnalysisResults and use the chosen actual extractor as it already has the offset, length, and ignore/honor case encapsulated, but that would be another pass. If we do that, I would like to add some tests for those classes first.

I also added some more tests to the KeyAnalyserTests for ContainsAnyLetters and HasSufficientUniquenessFactor to verify newly exposed behavior and also added some testing based on the known instances of freezable data that is being used in the FrozenFromKnownValuesTests.

The other changes imported from PR #88516 here are:

• Eliminate the the inner TryUseSubstring because we can just early return the calculated results as we build them
• Hoist the calculation of acceptableNonUniqueCount out to the top level since it never changes (which means we pass that into the HasSufficientUniquenessFactor method for it to "use up" internally (passed by value, so unchanged at call-site)
• Eliminated the delegate ReadOnlySpan<char> GetSpan and use, which helps reduce dynamic dispatch overhead in the CreateAnalysisResults method
• Eliminated the IsLeft field of the SubstringComparer since we can tell by the Index being negative that we're doing right-justified slicing (and documented that on the class)
• Changed the logic managing the Index and Count on the comparer for right-justified substrings.
  Added [MethodImpl(MethodImplOptions.AggressiveInlining)] to the Equals and GetHashCode overrides.

Author:	IDisposable
Assignees:	-
Labels:	area-System.Collections, community-contribution
Milestone:	-

[IDisposable]

This should supersede both PR #89689 and PR #88516

[IDisposable]

SharpLab with all the classes combined so you can see the resulting IL/JIT here

[IDisposable]

@adamsitnik, just to level-set... this is likely barely worth all the changes if we're only trying to speed up KeyAnalyzer itself... but if this is a gateway toward eventually exposing the higher-performance slicing comparers (based on ReadOnlySpan<char>) then it might be worth landing. It would also be useful to expose eventually the Slicer in the AnalyzeResults for the frozen collections to use as that would eliminate the delegates there, but that's not in the scope of this PR yet.

It does seem to net a 1-3% improvement against baseline for non-trivial sets, and the JITted code is very compact despite all the in-lining and is branchless beyond null-checks and array bounds length checks that are part of the inlined String.AsSpan() code.

If desired, I can tease out the unit tests that I added (which are higher value) and the more subtle changes I made along the way to make a simpler version of this.

Additionally, I think it might be worth discussing in a distinct issue the idea of making the String.AsSpan(start, length) understand negative offsets internally to eliminate a ton of this code, but that's a public API change as right now passing negative values will throw.

[stephentoub]

Thanks, @IDisposable. Regarding "but if this is a gateway toward eventually exposing the higher-performance slicing comparers", we don't have any plans or intention to do so.

[IDisposable]

Thanks @stephentoub!

Regarding the other points:

If desired, I can tease out the unit tests that I added (which are higher value) and the more subtle changes I made along the way to make a simpler version of this.

This would be limited to the smaller changes like hoisting the constant calculations exposing the internals for testing, and the tests and possibly going back to my original attempt
UPDATE: created a PR with the unit-tests and trivial hoistings #90301

Additionally, I think it might be worth discussing in a distinct issue the idea of making the String.AsSpan(start, length) understand negative offsets internally to eliminate a ton of this code, but that's a public API change as right now passing negative values will throw.

Any idea if this would be something folks would like to see done? It would be nice to not have to put in conditionals in the first place and leverage the internal knowledge. The current AsSpan merely throws when confronted with negative start index, so it would be a "light breaking" change. The concept would be that "foo".AsSpan(-2, 2) would return a ReadOnlySpan<char> pointing at the last two characters from the string (e.g. the ['o','o']), and "zot".AsSpan(-3,1) -> a ReadOnlySpan<char> pointing at ['z']. This would be tweaking code for MemoryExtensions and the ReadOnlySpan<T> constructor

[eiriktsarpalis]

Let's revisit this in .NET 9. We're very close to concluding .NET 8 development and making performance sensitive changes can be risky at this stage.

[IDisposable]

Makes perfect sense! I will put up a PR for just the additional unit tests as that would be nice :)

[IDisposable]

Extracted just the unit test changes and a couple calculation hoists into PR #90301

[eiriktsarpalis]

Given #90301 could you close this PR? You can always rebase and reopen it if and when the other one gets merged.

[IDisposable]

Sure!