Skip to content
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.

Sign up for GitHub

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

Optimization/grouped extensions #3260

Merged
merged 21 commits into from
May 26, 2020
Merged

Optimization/grouped extensions #3260

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
afa5047
Merge pull request #10 from windows-toolkit/master
Sergio0694 Mar 15, 2020
e8cd4f7
Merge pull request #18 from windows-toolkit/master
Sergio0694 Mar 27, 2020
cbc80ef
Merge pull request #19 from windows-toolkit/master
Sergio0694 Apr 3, 2020
4079a75
Merge remote-tracking branch 'upstream/master' into master2
Sergio0694 Apr 9, 2020
f0b5bf0
Merge remote-tracking branch 'upstream/master' into master2
Sergio0694 Apr 23, 2020
6129c69
Merge remote-tracking branch 'upstream/master' into master2
Sergio0694 Apr 30, 2020
fc7624f
Added ObservableGroup<TKey, TValue> debugger display
Sergio0694 Apr 30, 2020
4e05301
Optimized the readonly ObservableGroupedCollection extensions
Sergio0694 Apr 30, 2020
d129d74
Minor tweaks to RemoveGroup<TKey, TValue>
Sergio0694 Apr 30, 2020
c017e48
Finished refactoring of observable collections extensions
Sergio0694 Apr 30, 2020
1a88b0b
Minor style tweaks
Sergio0694 Apr 30, 2020
a8e4d67
Merge branch 'master' into optimization/grouped-extensions
michael-hawker May 6, 2020
6400993
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 12, 2020
630ffaf
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 13, 2020
3d4b0d6
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 15, 2020
b57db84
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 15, 2020
9bae4eb
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 16, 2020
ef1cd1a
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 17, 2020
e088c5b
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 21, 2020
9a8beef
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 26, 2020
d111b06
Merge branch 'master' into optimization/grouped-extensions
Sergio0694 May 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Microsoft.Toolkit/Collections/ObservableGroup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Linq;

namespace Microsoft.Toolkit.Collections
Expand All @@ -14,6 +15,7 @@ namespace Microsoft.Toolkit.Collections
/// </summary>
/// <typeparam name="TKey">The type of the group key.</typeparam>
/// <typeparam name="TValue">The type of the items in the collection.</typeparam>
[DebuggerDisplay("Key = {Key}, Count = {Count}")]
public sealed class ObservableGroup<TKey, TValue> : ObservableCollection<TValue>, IGrouping<TKey, TValue>, IReadOnlyObservableGroup
{
/// <summary>
Expand Down
16 changes: 16 additions & 0 deletions Microsoft.Toolkit/Collections/ObservableGroupedCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Runtime.CompilerServices;

#nullable enable

namespace Microsoft.Toolkit.Collections
{
Expand All @@ -30,5 +33,18 @@ public ObservableGroupedCollection(IEnumerable<IGrouping<TKey, TValue>> collecti
: base(collection.Select(c => new ObservableGroup<TKey, TValue>(c)))
{
}

/// <summary>
/// Tries to get the underlying <see cref="List{T}"/> instance, if present.
/// </summary>
/// <param name="list">The resulting <see cref="List{T}"/>, if one was in use.</param>
/// <returns>Whether or not a <see cref="List{T}"/> instance has been found.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool TryGetList(out List<ObservableGroup<TKey, TValue>>? list)
{
list = Items as List<ObservableGroup<TKey, TValue>>;

return !(list is null);
}
}
}
198 changes: 176 additions & 22 deletions Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.Contracts;
using System.Linq;
using System.Runtime.CompilerServices;

#nullable enable

namespace Microsoft.Toolkit.Collections
{
Expand All @@ -22,8 +26,18 @@ public static class ObservableGroupedCollectionExtensions
/// <param name="key">The key of the group to query.</param>
/// <returns>The first group matching <paramref name="key"/>.</returns>
/// <exception cref="InvalidOperationException">The target group does not exist.</exception>
[Pure]
public static ObservableGroup<TKey, TValue> First<TKey, TValue>(this ObservableGroupedCollection<TKey, TValue> source, TKey key)
=> source.First(group => GroupKeyPredicate(group, key));
{
ObservableGroup<TKey, TValue>? group = source.FirstOrDefault(key);

if (group is null)
{
ThrowArgumentExceptionForKeyNotFound();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sergio0694 Guard API or no?

Copy link
Member Author

@Sergio0694 Sergio0694 May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

}

return group!;
}

/// <summary>
/// Return the first group with <paramref name="key"/> key or null if not found.
Expand All @@ -33,8 +47,34 @@ public static ObservableGroup<TKey, TValue> First<TKey, TValue>(this ObservableG
/// <param name="source">The source <see cref="ObservableGroupedCollection{TKey, TValue}"/> instance.</param>
/// <param name="key">The key of the group to query.</param>
/// <returns>The first group matching <paramref name="key"/> or null.</returns>
public static ObservableGroup<TKey, TValue> FirstOrDefault<TKey, TValue>(this ObservableGroupedCollection<TKey, TValue> source, TKey key)
=> source.FirstOrDefault(group => GroupKeyPredicate(group, key));
[Pure]
public static ObservableGroup<TKey, TValue>? FirstOrDefault<TKey, TValue>(this ObservableGroupedCollection<TKey, TValue> source, TKey key)
{
if (source.TryGetList(out var list))
{
foreach (var group in list!)
{
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
return group;
}
}

return null;
}

return FirstOrDefaultWithLinq(source, key);
}

/// <summary>
/// Slow path for <see cref="First{TKey,TValue}"/>.
/// </summary>
[Pure]
[MethodImpl(MethodImplOptions.NoInlining)]
private static ObservableGroup<TKey, TValue>? FirstOrDefaultWithLinq<TKey, TValue>(ObservableGroupedCollection<TKey, TValue> source, TKey key)
{
return source.FirstOrDefault(group => EqualityComparer<TKey>.Default.Equals(group.Key, key));
}

/// <summary>
/// Return the element at position <paramref name="index"/> from the first group with <paramref name="key"/> key.
Expand All @@ -47,6 +87,7 @@ public static ObservableGroup<TKey, TValue> FirstOrDefault<TKey, TValue>(this Ob
/// <returns>The element.</returns>
/// <exception cref="InvalidOperationException">The target group does not exist.</exception>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="index"/> is less than zero or <paramref name="index"/> is greater than the group elements' count.</exception>
[Pure]
public static TValue ElementAt<TKey, TValue>(
this ObservableGroupedCollection<TKey, TValue> source,
TKey key,
Expand All @@ -62,18 +103,21 @@ public static TValue ElementAt<TKey, TValue>(
/// <param name="key">The key of the group to query.</param>
/// <param name="index">The index of the item from the targeted group.</param>
/// <returns>The element or default(TValue) if it does not exist.</returns>
[Pure]
public static TValue ElementAtOrDefault<TKey, TValue>(
this ObservableGroupedCollection<TKey, TValue> source,
TKey key,
int index)
{
var existingGroup = source.FirstOrDefault(key);
if (existingGroup is null)
var group = source.FirstOrDefault(key);

if (group is null ||
(uint)index >= (uint)group.Count)
{
return default;
return default!;
}

return existingGroup.ElementAtOrDefault(index);
return group[index];
}

/// <summary>
Expand All @@ -89,7 +133,7 @@ public static ObservableGroup<TKey, TValue> AddGroup<TKey, TValue>(
this ObservableGroupedCollection<TKey, TValue> source,
TKey key,
TValue value)
=> AddGroup(source, key, new[] { value });
=> AddGroup(source, key, new[] { value });

/// <summary>
/// Adds a key-collection <see cref="ObservableGroup{TKey, TValue}"/> item into a target <see cref="ObservableGroupedCollection{TKey, TValue}"/>.
Expand Down Expand Up @@ -141,15 +185,17 @@ public static ObservableGroup<TKey, TValue> AddItem<TKey, TValue>(
TKey key,
TValue item)
{
var existingGroup = source.FirstOrDefault(key);
if (existingGroup is null)
var group = source.FirstOrDefault(key);

if (group is null)
{
existingGroup = new ObservableGroup<TKey, TValue>(key);
source.Add(existingGroup);
group = new ObservableGroup<TKey, TValue>(key);
source.Add(group);
}

existingGroup.Add(item);
return existingGroup;
group.Add(item);

return group;
}

/// <summary>
Expand All @@ -172,6 +218,7 @@ public static ObservableGroup<TKey, TValue> InsertItem<TKey, TValue>(
{
var existingGroup = source.First(key);
existingGroup.Insert(index, item);

return existingGroup;
}

Expand All @@ -195,6 +242,7 @@ public static ObservableGroup<TKey, TValue> SetItem<TKey, TValue>(
{
var existingGroup = source.First(key);
existingGroup[index] = item;

return existingGroup;
}

Expand All @@ -209,11 +257,38 @@ public static ObservableGroup<TKey, TValue> SetItem<TKey, TValue>(
public static void RemoveGroup<TKey, TValue>(
this ObservableGroupedCollection<TKey, TValue> source,
TKey key)
{
if (source.TryGetList(out var list))
{
var index = 0;
foreach (var group in list!)
{
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
source.RemoveAt(index);

return;
}

index++;
}
}
else
{
RemoveGroupWithLinq(source, key);
}
}

/// <summary>
/// Slow path for <see cref="RemoveGroup{TKey,TValue}"/>.
/// </summary>
[MethodImpl(MethodImplOptions.NoInlining)]
private static void RemoveGroupWithLinq<TKey, TValue>(ObservableGroupedCollection<TKey, TValue> source, TKey key)
{
var index = 0;
foreach (var group in source)
{
if (GroupKeyPredicate(group, key))
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
source.RemoveAt(index);
return;
Expand All @@ -238,15 +313,51 @@ public static void RemoveItem<TKey, TValue>(
TKey key,
TValue item,
bool removeGroupIfEmpty = true)
{
if (source.TryGetList(out var list))
{
var index = 0;
foreach (var group in list!)
{
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
if (group.Remove(item) &&
removeGroupIfEmpty &&
group.Count == 0)
{
source.RemoveAt(index);
}

return;
}

index++;
}
}
else
{
RemoveItemWithLinq(source, key, item, removeGroupIfEmpty);
}
}

/// <summary>
/// Slow path for <see cref="RemoveItem{TKey,TValue}"/>.
/// </summary>
[MethodImpl(MethodImplOptions.NoInlining)]
private static void RemoveItemWithLinq<TKey, TValue>(
ObservableGroupedCollection<TKey, TValue> source,
TKey key,
TValue item,
bool removeGroupIfEmpty)
{
var index = 0;
foreach (var group in source)
{
if (GroupKeyPredicate(group, key))
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
group.Remove(item);

if (removeGroupIfEmpty && group.Count == 0)
if (group.Remove(item) &&
removeGroupIfEmpty &&
group.Count == 0)
{
source.RemoveAt(index);
}
Expand All @@ -268,16 +379,53 @@ public static void RemoveItem<TKey, TValue>(
/// <param name="key">The key of the group where the item at <paramref name="index"/> should be removed.</param>
/// <param name="index">The index of the item to remove in the group.</param>
/// <param name="removeGroupIfEmpty">If true (default value), the group will be removed once it becomes empty.</param>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="index"/> is less than zero or <paramref name="index"/> is greater than the group elements' count.</exception>
public static void RemoveItemAt<TKey, TValue>(
this ObservableGroupedCollection<TKey, TValue> source,
TKey key,
int index,
bool removeGroupIfEmpty = true)
{
if (source.TryGetList(out var list))
{
var groupIndex = 0;
foreach (var group in list!)
{
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
{
group.RemoveAt(index);

if (removeGroupIfEmpty && group.Count == 0)
{
source.RemoveAt(groupIndex);
}

return;
}

groupIndex++;
}
}
else
{
RemoveItemAtWithLinq(source, key, index, removeGroupIfEmpty);
}
}

/// <summary>
/// Slow path for <see cref="RemoveItemAt{TKey,TValue}"/>.
/// </summary>
[MethodImpl(MethodImplOptions.NoInlining)]
private static void RemoveItemAtWithLinq<TKey, TValue>(
ObservableGroupedCollection<TKey, TValue> source,
TKey key,
int index,
bool removeGroupIfEmpty)
{
var groupIndex = 0;
foreach (var group in source)
{
if (GroupKeyPredicate(group, key))
if (EqualityComparer<TKey>.Default.Equals(group.Key, key))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 😊

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, perfect then! 😊
Thank you for the review!

{
group.RemoveAt(index);

Expand All @@ -293,7 +441,13 @@ public static void RemoveItemAt<TKey, TValue>(
}
}

private static bool GroupKeyPredicate<TKey, TValue>(ObservableGroup<TKey, TValue> group, TKey expectedKey)
=> EqualityComparer<TKey>.Default.Equals(group.Key, expectedKey);
/// <summary>
/// Throws a new <see cref="InvalidOperationException"/> when a key is not found.
/// </summary>
[MethodImpl(MethodImplOptions.NoInlining)]
private static void ThrowArgumentExceptionForKeyNotFound()
{
throw new InvalidOperationException("The requested key was not present in the collection");
}
}
}