Optimization/grouped extensions #3260
Conversation
Changes from official master
Changes from master
Changes from master
|
Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
| { | ||
| var groupIndex = 0; | ||
| foreach (var group in source) | ||
| { | ||
| if (GroupKeyPredicate(group, key)) | ||
| if (EqualityComparer<TKey>.Default.Equals(group.Key, key)) |
There was a problem hiding this comment.
Could we achieve the same performances by keeping GroupKeyPredicate(group, key) here and adding the [MethodImpl(MethodImplOptions.AggressiveInlining)] to the GroupKeyPredicate() method ?
The code feel easier to read with a named method
There was a problem hiding this comment.
Generally the JIT should be able to produce the same code at the end, yeah. Though also from a style perspective, personally I feel like just leaving EqualityComparer<T> call directly makes the code clearer in its behavior, instead of hiding the logic away. Like, while working on this PR I found myself looking up that method multiple times as I wasn't sure exactly what its semantics was or how it was working exactly. Since it's just a wrapper on this call and it's only shorter by a few characters, I think I'd prefer to just leave the EqualityComparer<T> invocation explicit. Just my two cents though, so let me know what you think! 😊
There was a problem hiding this comment.
I usually prefer having a named method to describe the intent (even if it here, it seems that it missed the point ☺). I find it make the code easier to maintain because we have a single update point if one day we change the equality logic (even if it here it is unlikely to happen).
There was a problem hiding this comment.
I see, yeah that makes sense. I guess I just tend to do the opposite in general out of habit when the wrapped code is so short just to give the JIT a litte nudge, but either approach is fine, really 😊
Not sure what you mean by "even if it here, it seems that it missed the point", would you like me to revert that change then and reintroduce that wrapping method, or do we want to leave this as is for now? Let me know and I'll make the changes if needed!
There was a problem hiding this comment.
even if it here, it seems that it missed the point;
I was meaning that the naming wasn't that good since you said you had to look several times at the implementation to understand what it was doing 🙂.
We can leave this code as-is for now. No need to push another update only for this.
There was a problem hiding this comment.
Alright, perfect then! 😊
Thank you for the review!
Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (group is null) | ||
| { | ||
| ThrowArgumentExceptionForKeyNotFound(); |
There was a problem hiding this comment.
@Sergio0694 Guard API or no?
There was a problem hiding this comment.
I thought it might be best for us not to use that in this case so that we could have a clearer error message for users. As in, this way we'll get "The desired key is not present in the collection" rather than a more obscure "Parameter group must not be null". I mean, I could see devs going like, what group? Where? 🤔
Quick-fix PR to remove an unnecessary `[MethodImpl(MethodImplOptions.NoInlining)]` attribute from a throw helper method. That attribute caused the JIT not to see the throw in the body, resulting in the caller branch assuming the forward path being taken (the faulty one). This produced worse codegen, as the non-faulting path was always forced to have an extra jump ahead. Also did another small codegen tweak and added a more detailed exception message for users. ## Leftover fix from #3260 <!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. --> <!-- Add a brief overview here of the feature/bug & fix. --> ## PR Type What kind of change does this PR introduce? <!-- Please uncomment one or more that apply to this PR. --> <!-- - Bugfix --> <!-- - Feature --> - Optimization <!-- - Code style update (formatting) --> <!-- - Refactoring (no functional changes, no api changes) --> <!-- - Build or CI related changes --> <!-- - Documentation content changes --> <!-- - Sample app changes --> <!-- - Other... Please describe: --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying, or link to a relevant issue. --> Unoptimal codegen for this throw helper method: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/30452cf0bbf627f12825cf50bbd31b0e526b9abe/Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs#L447-L451 ## What is the new behavior? <!-- Describe how was this issue resolved or changed? --> The JIT is now properly handling calls to this method. ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [X] Contains **NO** breaking changes
Follow up to #3246
This PR just includes some speed/memory optimizations to the extensions for the
ObservableGroupedCollection<TKey, TValue>type. In particular, it leverages the fact that the underlying collection is almost always aList<T>instance to obtain astructenumerator and use that to perform the requested query. This allows the operation to be performed faster (as theMoveNextandCurrentare no longercallvirt-s and hence can be inlined) and without allocations (the enumerator is just a value type now). Here's a benchmark to show the OG/PRFirstmethod:You can see how in this example, no matter the size of the grouped collection, the updated
Firstmethod is consistently over twice as fast, and without any memory allocations at all.PR Type
What kind of change does this PR introduce?
PR Checklist
Please check if your PR fulfills the following requirements:
Pull Request has been submitted to the documentation repository instructions. Link:Sample in sample app has been added / updated (for bug fixes / features)Icon has been created (if new sample) following the Thumbnail Style Guide and templatesIf new feature, add to Feature ListOther information
Note to self: once #3167 gets merged with the .NET Standard 2.1 multitarget, these extensions need to be decorated with nullability attributes for uncostrained generic type parameters as well.