-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Improve ImmutableArrayExtensions.SequenceEqual
#118932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improve ImmutableArrayExtensions.SequenceEqual
#118932
Conversation
@dotnet-policy-service agree |
src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-collections |
src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs
Outdated
Show resolved
Hide resolved
Perhaps a simpler (and faster) way to do this is to just call |
…utableArrayExtensions.cs Co-authored-by: Pranav Senthilnathan <[email protected]>
I'm not sure if it can be applied to |
This should work:
If it is acceptable to delegate the argument validation to
|
@xtqqczze Full benchmark results
|
@prozolic I see a few regressions. Could you collect benchmark data for https://github.com/xtqqczze/dotnet-runtime/tree/ImmutableArrayExtensions.SequenceEqual?. This approach will not be worth the additional complexity, however. |
Sorry. Indeed, there were memory allocation issues and regressions in some cases. I conducted performance measurements comparing different implementations of SequenceEqual.
Full benchmark results
|
OK, so using I think the best balance of complexity to performance would be to delegate to |
Something like:
and then put argument validation and existing implementation in static local function |
Thank you for the advice!
|
@prozolic You don't need the Could you also provide benchmark data again? |
@xtqqczze Here are the benchmark results comparing with the current existing implementation (SequenceEqual_Original):
|
@prozolic The results look excellent, very close to |
It doesn’t look like we currently have performance coverage for this method in https://github.com/dotnet/performance. |
You're right, there's no official performance test coverage for this yet. From what I can see, there isn't much performance test coverage for ImmutableArray itself in dotnet/performance either. |
Regarding the benchmark, I submitted a pull request at dotnet/performance#4915. Original:
Modified:
|
@prozolic Does PR description need updating with new performance numbers? |
@xtqqczze Therefore, I have updated the Summary description to match the current implementation and changed the benchmark results to those from dotnet/performance#4915. |
We cannot make this conclusion from the data, as there is high Error/StdDev for the ICollection benchmark. Were other applications running at the same time as the benchmark?
We didn't delegate to |
You're right about the ICollection claim. I've updated it to "No statistically significant performance difference" (please let me know if there's a better wording). Regarding benchmark results, other applications may have been running and affecting the measurements, so I re-ran the benchmarks in a clean environment and updated the results.
I've updated it to "Uses existing implementation for this case because the existing implementation had better performance and didn't allocate a second enumerator" |
int i = 0; | ||
int n = immutableArray.Length; | ||
foreach (TDerived item in items) | ||
static bool Impl(ImmutableArray<TBase> immutableArray, IEnumerable<TDerived> items, IEqualityComparer<TBase>? comparer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can change the type of the items
parameter here to IEnumerable<TBase>
and cast at the call site. Let's see what other reviewers think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there would be much difference in complexity or performance compared to the current implementation, so I personally think we don't particularly need to make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be a difference on R2R due to boxing, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not very familiar with R2R (ReadyToRun), so I'm sorry but I don't know the impact of this change either.
src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs
Outdated
Show resolved
Hide resolved
…utableArrayExtensions.cs Co-authored-by: xtqqczze <[email protected]>
I've marked this as
NO-MERGE
|
Summary
This PR improves the performance of ImmutableArrayExtensions.SequenceEqual method through optimized implementation strategies:
Key Performance Improvements:
Implementation Approach
The implementation was refined based on review feedback from @neon-sunset and @xtqqczze to achieve optimal performance while balancing complexity:
ICollection<T>
and delegates to the highly optimizedEnumerable.SequenceEqual
using the underlying arrayIEnumerable<T>
types to maintain behavioral consistencyBenchmark (dotnet/performance#4915)
Original:
Modified: