Remove LINQ usage from OpenAPI comparers #56599
Conversation
ghost
added
the
needs-area-label
| return false; | ||
| } | ||
|
|
||
| foreach (var key in x.Keys) |
There was a problem hiding this comment.
Something to A/B test as I don't know the answer.
Would iterating like foreach ((var key, var valueX) in x) and avoiding the x[key] call be better or worse? I guess it depends on if iterating the keys is cheaper that iterating both and how often the right hand side of the || is accessed.
| for (var i = 0; i < x.Length; i++) | ||
| { | ||
| if (!Equals(x[i], y[i])) | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would SequenceEquals() on the two arrays be more performant maybe?
mkArtakMSFT
added
feature-openapi
area-networking
captainsafia
added
area-mvc
|
|
||
| internal static class ComparerHelpers | ||
| { | ||
| internal static bool DictionaryEquals<TKey, TValue>(IDictionary<TKey, TValue> x, IDictionary<TKey, TValue> y, IEqualityComparer<TValue> comparer) |
There was a problem hiding this comment.
Are the IDictionary and IList properties on OpenApiSchema just Dictionary and List? If so, there might be some perf benefits of adding a path that casts to the concrete types and duplicates the comparison logic.
That would avoid interface dispatch overhead, at the cost of some duplication.
There was a problem hiding this comment.
Are the IDictionary and IList properties on OpenApiSchema just Dictionary and List?
They are. It's technically feasible for an end-user to override them with their own implementations but I suspect that is not likely. This was a good idea. There's a reasonable delta between the baseline and this change.
Remove LINQ from comparers
| Method | TransformerCount | Mean | Error | StdDev | Op/s | Gen0 | Allocated |
|---|---|---|---|---|---|---|---|
| SchemaTransformer | 10 | 28.16 us | 0.510 us | 0.477 us | 35,515.0 | 0.2441 | 31.78 KB |
| SchemaTransformer | 100 | 30.16 us | 0.480 us | 0.449 us | 33,158.4 | 0.2441 | 31.78 KB |
| SchemaTransformer | 1000 | 35.36 us | 0.395 us | 0.370 us | 28,277.9 | 0.2441 | 31.78 KB |
Cast to concrete types in overloads
| Method | TransformerCount | Mean | Error | StdDev | Op/s | Gen0 | Allocated |
|---|---|---|---|---|---|---|---|
| SchemaTransformer | 10 | 24.39 us | 0.486 us | 0.431 us | 40,995.4 | 0.2441 | 31.19 KB |
| SchemaTransformer | 100 | 25.32 us | 0.304 us | 0.284 us | 39,491.7 | 0.2441 | 31.19 KB |
| SchemaTransformer | 1000 | 29.84 us | 0.328 us | 0.290 us | 33,517.4 | 0.2441 | 32.19 KB |
|
Closing this in favor of a new PR that accounts for the latest changes merged into this implementation. |
Trying to improve schema transformer perf... 🤔