Speed up KeyAnalyser #87590

IDisposable · 2023-06-15T02:48:00Z

While reviewing #87510 , I noticed the inline code in the Comparers seems like since it's in the hot-loop path, might be faster to move the IsLeft conditionalized code out of the loop by adding a Slicer to the comparator. The Slicer is the same logic as was in the bodies of the Equals and GetHashCode methods, and indeed also matches the delegate that was passed to the internal CreateAnalysisResults method.

NOT SURE if this actually will speed things up at all, hoping to leverage some /azp magic to do the performance evaluations.

ghost · 2023-06-15T02:48:12Z

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

Issue Details

null

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

azure-pipelines · 2023-06-15T02:53:40Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

azure-pipelines · 2023-06-15T02:54:22Z

Commenter does not have sufficient privileges for PR 87590 in repo dotnet/runtime

IDisposable · 2023-06-15T02:56:52Z

src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/String/KeyAnalyzer.cs

@@ -124,7 +120,7 @@ private static bool TryUseSubstring(ReadOnlySpan<string> uniqueStrings, bool ign
                // actually perform the comparison as case-sensitive even if case-insensitive
                // was requested, as there's nothing that would compare equally to the substring
                // other than the substring itself.
-                bool canSwitchIgnoreCaseToCaseSensitive = ignoreCase;
+                bool canSwitchIgnoreCaseToCaseSensitive = true;


This is demonstrably true because we're inside the if (ignoreCase) immediately above and could be a distinct PR, but it actually changes nothing but the readability and not hoping the Compiler/JIT groks this truth.

azure-pipelines · 2023-06-22T05:58:10Z

Commenter does not have sufficient privileges for PR 87590 in repo dotnet/runtime

IDisposable · 2023-06-23T05:07:58Z

Hey @adamsitnik, can you do the /azp magic to make this run the perf tests?

adamsitnik · 2023-06-23T14:45:27Z

Hey @adamsitnik, can you do the /azp magic to make this run the perf tests?

I am afraid we don't have that in dotnet/runtime yet (cc @sblom)

For now you need to follow the docs I provided in #87510 (comment) and benchmark it locally

IDisposable · 2023-06-23T15:13:04Z

~~Sadly @adamsitnik , I can't even get the dotnet/performance stuff to execute at all on my machine... :( dotnet/performance#3096~~

Fixed that problem by whacking my nuget caches!

IDisposable · 2023-06-29T07:25:30Z

dotnet run -c Release -f net8.0 \ 
 --filter *Frozen* \
 --statisticalTest 2ms \
 --coreRun \
 "C:\Dev\dotnet\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"  \
 "C:\Dev\dotnet\runtime_baseline\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"

Initial results from using @adamsitnik benchmarks as of dotnet/performance@28e9ab3

BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22631.1906)
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
Change: .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2 from IDisposable@fae39cb
Baseline : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2 from cfae69f

PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1

Method	Toolchain	Size	Mean	Error	StdDev	Median	Min	Max	Ratio	MannWhitney(2ms)	Allocated	Alloc Ratio
FrozenDictionary	Change	512	6.363 us	0.0315 us	0.0263 us	6.364 us	6.317 us	6.404 us	1.00	Base	-	NA
FrozenDictionary	Baseline	512	6.447 us	0.0726 us	0.0644 us	6.438 us	6.360 us	6.577 us	1.01	Same	-	NA

IDisposable · 2023-06-29T07:35:07Z

So, is a ratio of 1.01 (old code) vs. 1.00 (new code) worthwhile? It isn't slower, and it (to me) is less coupled. Also, I note that the Baseline version has a higher SD and Error, which (to me) means it's more influenced by branch mispredictions.

Also, realized I should have presented the Baseline first in the command, future runs will do so... sorry, learning here.

IDisposable · 2023-06-29T08:00:13Z

src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/String/KeyAnalyzer.cs

@@ -263,25 +259,32 @@ public AnalysisResults(bool ignoreCase, bool allAsciiIfIgnoreCase, int hashIndex
            public bool RightJustifiedSubstring => HashIndex < 0;
        }

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static ReadOnlySpan<char> SliceLeft(string s, int index, int count) => s!.AsSpan(index, count);


This encapsulates the login on old line 75

IDisposable · 2023-06-29T08:01:58Z

src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/String/KeyAnalyzer.cs

+        public static ReadOnlySpan<char> SliceLeft(string s, int index, int count) => s!.AsSpan(index, count);
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static ReadOnlySpan<char> SliceRight(string s, int index, int count) => s!.AsSpan(s!.Length + index, count);


This encapsulates the login on old line 100

IDisposable · 2023-06-29T08:05:45Z

src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/String/KeyAnalyzer.cs

            public abstract bool Equals(string? x, string? y);
            public abstract int GetHashCode(string s);
        }

        private sealed class JustifiedSubstringComparer : SubstringComparer
        {
-            public override bool Equals(string? x, string? y) => x.AsSpan(IsLeft ? Index : (x!.Length + Index), Count).SequenceEqual(y.AsSpan(IsLeft ? Index : (y!.Length + Index), Count));
-            public override int GetHashCode(string s) => Hashing.GetHashCodeOrdinal(s.AsSpan(IsLeft ? Index : (s.Length + Index), Count));
+            public override bool Equals(string? x, string? y) => Slicer(x!, Index, Count).SequenceEqual(Slicer(y!, Index, Count));


Here we can just call Slicer, which no longer has an IsLeft conditional test inside a hot loop, so calling through to the GetSpan-like method that was set on lines 60 (for the IsLeft == true logic) and 84 (for the IsLeft == false logic). This (to my reasoning) will result in much better low-level branch-prediction and since the actual delegate bodies are [MethodImpl(MethodImplOptions.AggressiveInlining)] they should be inlined stubs.

We also get to reuse those stubs when we do the construction (instead of "similar" ones coded up for the calls to CreateAnalysisResults)

I am not confident about the various not-null hints ! that I injected, but I think they're right, if not overkill. Not sure it really would affect the code generated and welcome input on that from low-level ninjas

IDisposable · 2023-06-29T20:20:16Z

I'm not sure this is an endpoint or a jumping off for more or simply an exploration that should be tabled. I welcome input from @adamsitnik and @MichalStrehovsky or anyone else :)

The slicing is really only changed once per Count, so move the IsLeft-dependent logic into delegates to see if that makes things faster. A little more refactoring Encourage inlining.

IDisposable · 2023-07-07T03:16:17Z

Closing this because it's gotten messy

ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 15, 2023

dotnet-issue-labeler bot added the area-System.Collections label Jun 15, 2023

IDisposable commented Jun 15, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jun 15, 2023

Tracking issue for CI build timeouts #76454

Closed

IDisposable force-pushed the faster-freeze-strings branch from b4ba147 to 3bd2490 Compare June 22, 2023 05:56

IDisposable force-pushed the faster-freeze-strings branch from 3bd2490 to 70b7bbd Compare June 22, 2023 20:44

build-analysis bot mentioned this pull request Jun 22, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

IDisposable force-pushed the faster-freeze-strings branch from 70b7bbd to 13fc249 Compare June 22, 2023 23:32

build-analysis bot mentioned this pull request Jun 23, 2023

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

IDisposable force-pushed the faster-freeze-strings branch 3 times, most recently from 5f231fa to fae39cb Compare June 29, 2023 05:19

IDisposable marked this pull request as ready for review June 29, 2023 07:39

IDisposable requested a review from MichalStrehovsky as a code owner June 29, 2023 07:39

IDisposable force-pushed the faster-freeze-strings branch from fae39cb to 42c3414 Compare June 29, 2023 07:56

IDisposable commented Jun 29, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jun 29, 2023

windows-x86 release MSBuild_CMake is failing #88172

Closed

IDisposable force-pushed the faster-freeze-strings branch from 42c3414 to b623d95 Compare July 5, 2023 21:05

build-analysis bot mentioned this pull request Jul 6, 2023

Changes to default comparer for byte broke BinaryFormatter tests #88453

Closed

IDisposable force-pushed the faster-freeze-strings branch 5 times, most recently from 7745a50 to 2e37d7f Compare July 7, 2023 02:18

Move the IsLeft/IsRight decision out of the loop

01da475

The slicing is really only changed once per Count, so move the IsLeft-dependent logic into delegates to see if that makes things faster. A little more refactoring Encourage inlining.

IDisposable force-pushed the faster-freeze-strings branch from 2e37d7f to 01da475 Compare July 7, 2023 03:15

IDisposable closed this Jul 7, 2023

build-analysis bot mentioned this pull request Jul 7, 2023

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

ghost locked as resolved and limited conversation to collaborators Aug 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up KeyAnalyser #87590

Speed up KeyAnalyser #87590

IDisposable commented Jun 15, 2023 •

edited

Loading

ghost commented Jun 15, 2023

azure-pipelines bot commented Jun 15, 2023

azure-pipelines bot commented Jun 15, 2023

IDisposable Jun 15, 2023 •

edited

Loading

azure-pipelines bot commented Jun 22, 2023

IDisposable commented Jun 23, 2023

adamsitnik commented Jun 23, 2023

IDisposable commented Jun 23, 2023 •

edited

Loading

IDisposable commented Jun 29, 2023 •

edited

Loading

IDisposable commented Jun 29, 2023 •

edited

Loading

IDisposable Jun 29, 2023

IDisposable Jun 29, 2023

IDisposable Jun 29, 2023 •

edited

Loading

IDisposable Jun 29, 2023

IDisposable commented Jun 29, 2023

IDisposable commented Jul 7, 2023

Speed up KeyAnalyser #87590

Speed up KeyAnalyser #87590

Conversation

IDisposable commented Jun 15, 2023 • edited Loading

ghost commented Jun 15, 2023

azure-pipelines bot commented Jun 15, 2023

azure-pipelines bot commented Jun 15, 2023

IDisposable Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

azure-pipelines bot commented Jun 22, 2023

IDisposable commented Jun 23, 2023

adamsitnik commented Jun 23, 2023

IDisposable commented Jun 23, 2023 • edited Loading

IDisposable commented Jun 29, 2023 • edited Loading

IDisposable commented Jun 29, 2023 • edited Loading

IDisposable Jun 29, 2023

Choose a reason for hiding this comment

IDisposable Jun 29, 2023

Choose a reason for hiding this comment

IDisposable Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

IDisposable Jun 29, 2023

Choose a reason for hiding this comment

IDisposable commented Jun 29, 2023

IDisposable commented Jul 7, 2023

IDisposable commented Jun 15, 2023 •

edited

Loading

IDisposable Jun 15, 2023 •

edited

Loading

IDisposable commented Jun 23, 2023 •

edited

Loading

IDisposable commented Jun 29, 2023 •

edited

Loading

IDisposable commented Jun 29, 2023 •

edited

Loading

IDisposable Jun 29, 2023 •

edited

Loading