Uh oh!
There was an error while loading. Please reload this page.
Speed up KeyAnalyser - #87590
Conversation
ghost
commented
Jun 15, 2023
Tagging subscribers to this area: @dotnet/area-system-collections Issue Detailsnull
|
Supported commands
See additional documentation. |
|
Commenter does not have sufficient privileges for PR 87590 in repo dotnet/runtime |
There was a problem hiding this comment.
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.
b4ba147 to
3bd2490Compare|
Commenter does not have sufficient privileges for PR 87590 in repo dotnet/runtime |
3bd2490 to
70b7bbdCompare70b7bbd to
13fc249CompareIDisposable
commented
Jun 23, 2023
Hey @adamsitnik, can you do the |
adamsitnik
commented
Jun 23, 2023
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 |
Fixed that problem by whacking my nuget caches! |
5f231fa to
fae39cbCompareInitial results from using @adamsitnik benchmarks as of dotnet/performance@28e9ab3 BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22631.1906) PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
|
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. |
fae39cb to
42c3414CompareThere was a problem hiding this comment.
This encapsulates the login on old line 100
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
commented
Jun 29, 2023
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 :) |
42c3414 to
b623d95Compare7745a50 to
2e37d7fCompareThe 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.
2e37d7f to
01da475CompareIDisposable
commented
Jul 7, 2023
Closing this because it's gotten messy |
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
IsLeftconditionalized code out of the loop by adding a Slicer to the comparator. The Slicer is the same logic as was in the bodies of theEqualsandGetHashCodemethods, and indeed also matches the delegate that was passed to the internalCreateAnalysisResultsmethod.NOT SURE if this actually will speed things up at all, hoping to leverage some /azp magic to do the performance evaluations.