Skip to content

Commit aa2ad66

Browse files
author
msftbot[bot]
authored
Minor codegen improvements in Microsoft.Toolkit.Collections APIs (#3439)
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
2 parents b1a2ccf + b66af08 commit aa2ad66

File tree

2 files changed

+10
-3
lines changed

2 files changed

+10
-3
lines changed

Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,6 @@ private static void RemoveItemAtWithLinq<TKey, TValue>(
444444
/// <summary>
445445
/// Throws a new <see cref="InvalidOperationException"/> when a key is not found.
446446
/// </summary>
447-
[MethodImpl(MethodImplOptions.NoInlining)]
448447
private static void ThrowArgumentExceptionForKeyNotFound()
449448
{
450449
throw new InvalidOperationException("The requested key was not present in the collection");

Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,16 @@ private void OnSourceCollectionChanged(object sender, NotifyCollectionChangedEve
5252
// reporting the changes one by one. We consider only this case for now.
5353
if (e.OldItems?.Count > 1 || e.NewItems?.Count > 1)
5454
{
55-
Debug.Fail("OldItems and NewItems should contain at most 1 item");
56-
throw new NotSupportedException();
55+
static void ThrowNotSupportedException()
56+
{
57+
throw new NotSupportedException(
58+
"ReadOnlyObservableGroupedCollection<TKey, TValue> doesn't support operations on multiple items at once.\n" +
59+
"If this exception was thrown, it likely means support for batched item updates has been added to the " +
60+
"underlying ObservableCollection<T> type, and this implementation doesn't support that feature yet.\n" +
61+
"Please consider opening an issue in https://aka.ms/windowstoolkit to report this.");
62+
}
63+
64+
ThrowNotSupportedException();
5765
}
5866

5967
var newItem = e.NewItems?.Cast<ObservableGroup<TKey, TValue>>()?.FirstOrDefault();

0 commit comments

Comments
 (0)