Optimize ImmutableSortedSet<T>.SetEquals to avoid unnecessary allocations#126549
Optimize ImmutableSortedSet<T>.SetEquals to avoid unnecessary allocations#126549aw0lid wants to merge 1 commit intodotnet:mainfrom
Conversation
f9f1622 to
1c158bc
Compare
1c158bc to
827bfda
Compare
666ab6c to
41c75bf
Compare
41c75bf to
390c55b
Compare
132ed04 to
9afc5d0
Compare
9afc5d0 to
0302e72
Compare
|
Just gentle ping in case this fell through the cracks. |
| @@ -366,33 +366,118 @@ public ImmutableSortedSet<T> WithComparer(IComparer<T>? comparer) | |||
| /// </summary> | |||
| /// <param name="other">The sequence of items to check against this set.</param> | |||
| /// <returns>A value indicating whether the sets are equal.</returns> | |||
| public bool SetEquals(IEnumerable<T> other) | |||
| private bool SetEqualsFastPath(ICollection<T> other) | |||
There was a problem hiding this comment.
Avoid putting private implementations ahead of public implementations. If factoring out code into a separate method is necessary, consider using a local method instead.
There was a problem hiding this comment.
I have moved the private implementations below the public ones to follow the project's convention.
Regarding local methods, I opted for separate private static helpers to maintain the consistency of the class and avoid making the main method's body overly complex or deeply nested.
| { | ||
| if (!this.Contains(item)) | ||
| using var e2 = otherSet.GetEnumerator(); |
There was a problem hiding this comment.
This appears to be repeating the same logic as when other is already a sorted set. I think you could rework the structure of your nested functions a little bit... Lose the "fast path" and "fallback" methods and have methods for comparing SortedSet and ImmutableSortedSet specifically.
There was a problem hiding this comment.
Indeed, I had already noticed that duplication and streamlined the code in the latest update.
0302e72 to
5697f96
Compare
| { | ||
| if (!this.Contains(item)) | ||
| //We check for < instead of != because other is not guaranteed to be a set; it could be a List<T> or another collection with duplicates. |
There was a problem hiding this comment.
| //We check for < instead of != because other is not guaranteed to be a set; it could be a List<T> or another collection with duplicates. | |
| // We check for < instead of != because other is not guaranteed to be a set; it could be a List<T> or another collection with duplicates. |
| matches++; | ||
| if (other is ImmutableSortedSet<T> otherAsImmutableSortedSet) | ||
| { | ||
| if (otherAsImmutableSortedSet.KeyComparer == this.KeyComparer) |
There was a problem hiding this comment.
I suspect this could be simplified using a switch statement that pattern matches on other using a when clause to check the comparer. What do you think?
Part of #127279
Summary
ImmutableSortedSet<T>.SetEqualsalways creates a new intermediateSortedSet<T>for theothercollection, leading to avoidable allocations and GC pressure, especially for large datasetsOptimization Logic
otheris anImmutableSortedSetorSortedSet, triggering optimized logic only if theirComparermatches.ReferenceEqualscheck and leveragesICollectionto returnfalseearly ifother.Countis less thanthis.Count..Contains()check with a dual-enumeratorwhileloop. This leverages the sorted nature of both sets to achievenew SortedSet<T>is required for generalIEnumerabletypes, the final comparison now uses the same efficientClick to expand Benchmark Source Code
Click to expand Benchmark Results
Benchmark Results (Before Optimization)
Benchmark Results (After Optimization)
Performance Analysis Summary (100,000 Elements)
Performance Summary (Speedup & Memory)