Speed up KeyAnalyzer for substring based frozen collections #89689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The slicing is really only changed once per Count, so move the IsLeft-dependent logic into `Slicer` method and eliminate the `GetSpan` delegate. Changed to also pass the already-computed `set` of unique substrings to the `CreateAnalysisResults` method, so we don't recompute the slices twice. In order than either the set or the original `uniqueStrings` can be passed, swapped that argument for the `Analyze` method to take the `uniqueStrings` as a `string[]` (which it already is at all call-sites).
Subtle bug in that the entire string is being placed in the set, not the span.
Since we are working with the same set of input strings in each strategy there's no reason to take the length every time we make an attempt (per count, both left and right justified). Hoist the calculation of the acceptable number of collisions out to the top, do it once, and pass that number into the `HasSufficientUniquenessFactor` method for it to (locally) use up.
Benchmarks ever so slightly better.
Looks like the overhead of IEnumerable<string> is not worth the savings for the benchmark test data. Perhaps it would matter less if we were freezing more strings, but not likely
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-collections Issue Detailsnull
|
|
Performance tests.
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22631.2115)
Intel Core i7-10875H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.100-preview.7.23322.33
[Host] : .NET 8.0.0 (8.0.23.32106), X64 RyuJIT AVX2
Job-SVXVNR : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-CESATQ : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-YVAYLV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
|
This will cost one extra `HashSet<string>` and one extra `SubstringComparer` to be allocated, but might make the code run faster. Use the GSW strategy for virtual flattening
|
This was a fantastic learning experience... This gained, at most, 3% performance without adversely impacting the allocation patterns and helped me learn the |
|
What is GSW pattern in this context? Maybe i missed it. |
Using a GenericSpecializedWrapper to "pass in" the explicit implementation details at template declaration time as a |
Replace ISubstringComparer with ISubstringEqualityComparer as that's more indicative of the interface it's exposing, and rename everything derived from that. Fixed the implementation of FullStringEqualityComparer to not actually use the slice ReadOnlySpan(s) in the Equals and GetHashCode specializations. Added comments for the SubstringEquality classes Renamed the files for the specialization of the comparers. Fixed the unit tests under debug builds. Extended the test suite data for the KeyAnalyzer tests to exercise using the data in FrozenFromKnownValuesTests.
|
I've done another pass to clean up the documentation and make it a little cleaner. |
This was referenced Aug 2, 2023
|
Superseded by PR #89863 |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
sealedclasses where all the logic in conditional-free (down to the internals of theString.AsSpan()methods themselves. This required making some potentially hard-to-read changes, but the essence is that now we construct a 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
uniqueStringswe're presented with, the overhead of having another comparer/collection is pretty low in comparison.To ensure we don't bloat memory. I reversed when we clean the
setto right before we return false fromHasSufficientUniquenessFactorsince we know that we're going to get called again for a different index/count... this means that bothHashSet<string>s spend their lifetimes empty except when being tested (we could clear them on success, as well, but the old code didn't bother).The resulting SubstringComparer classes could be useful elsewhere, but are currently just internal to the System.Collections.Immutable assembly. I suspect we could/should pass them out in the AnalysisResults and use the chosen substring 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 KeyAnalyser tests to verify newly exposed behavior.
The other changes imported from PR #88516 here are:
TryUseSubstringbecause we can just early return the calculated results as we build themacceptableNonUniqueCountout to the top level since it never changes (which means we pass that into theHasSufficientUniquenessFactormethod for it to "use up" internally (passed by value, so unchanged at call-site)delegate ReadOnlySpan<char> GetSpanand use, which helps reduce dynamic dispatch overhead in theCreateAnalysisResultsmethodIsLeftfield of theSubstringComparersince we can tell by theIndexbeing negative that we're doing right-justified slicing (and documented that on the class)IndexandCounton the comparer for right-justified substrings.[MethodImpl(MethodImplOptions.AggressiveInlining)]to theEqualsandGetHashCodeoverrides.