Remove linq from BaseConfigurationComparer #2465
Conversation
| foreach (var key in config1.SigningKeys) | ||
| { | ||
| if (!ContainsKeyWithInternalId(config2, key.InternalId)) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Be a little careful here. While I applaud the reduction of LINQ usage on a hot path, Enumerable.Except may or may not be something you want to get rid of like this. Except works by building up a HashSet for one of the inputs and then enumerating the other and checking each item against the HashSet. This makes it generally O(N + M). You've replaced it with a doubly-nested loop, where for every item in config1.SigningKeys you're iterating through config2.SigningKeys... that makes it O(N * M). If these collections are always small, that's probably not a big deal. If they could be bigger, that's a problem. Also, every time you're iterating over SigningKeys in ContainsKeyWithInternalId, you're allocating an enumerator. So, the "fix" here may not actually be making things better.
There was a problem hiding this comment.
thanks for taking a look, the typical case is something like 2-5 keys so I wasn't too worried about the nested for loop.
I could also just revert this part.
There was a problem hiding this comment.
resolving, these are small collections.
There was a problem hiding this comment.
Did you measure the impact of this change on both throughput and allocation? It's quite possible it actually made things worse. With the change in GetHashCode, it's easy to eyeball and it know "yes, this change is definitively better". With this change here, it's much less obvious, e.g. you're replacing a few allocations for a HashSet with an enumerator allocation per config1 key plus the n-squared-ness I mentioned. You might be right that the result is better, but it's not necessarily obvious. Always good to measure these kinds of changes.
There was a problem hiding this comment.
You are completely right, better to lead with data. I wrote a microbenchmark just for the equals method using real data:
// * Summary *
BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22631.3007), VM=Hyper-V
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.101
[Host] : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
DefaultJob : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
| Method | Mean | Error | StdDev | Gen0 | Allocated |
|---|---|---|---|---|---|
| BaseConfigurationComparer | 461.8 ns | 1.13 ns | 0.95 ns | 0.0172 | 440 B |
| BaseConfigurationComparerWithLinq | 926.3 ns | 5.82 ns | 5.44 ns | 0.0410 | 1048 B |
There was a problem hiding this comment.
If we're confident this is representative, great.
Thanks
Fixes #2464