Skip to content

Commit

Permalink
Remove HashSet allocation in majority cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@smoogipoo
smoogipoo committed Oct 31, 2024
1 parent 255d89e commit 58f3221
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 58 deletions.
149 changes: 91 additions & 58 deletions osu.Framework/Bindables/BindableList.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Collections.Specialized;
using System.Globalization;
using System.Linq;
using System.Runtime.CompilerServices;
using JetBrains.Annotations;
using osu.Framework.Caching;
using osu.Framework.Extensions.TypeExtensions;
Expand Down Expand Up @@ -37,12 +38,17 @@ public class BindableList<T> : IBindableList<T>, IBindable, IParseable, IList<T>

private LockedWeakList<BindableList<T>> bindings;

private readonly int instanceId;

/// <summary>
/// Creates a new <see cref="BindableList{T}"/>, optionally adding the items of the given collection.
/// </summary>
/// <param name="items">The items that are going to be contained in the newly created <see cref="BindableList{T}"/>.</param>
public BindableList(IEnumerable<T> items = null)
{
// This will return the unique runtime identity for the object.
instanceId = RuntimeHelpers.GetHashCode(this);

if (items != null)
collection.AddRange(items);
}
Expand All @@ -57,12 +63,17 @@ public BindableList(IEnumerable<T> items = null)
public T this[int index]
{
get => collection[index];
set => setIndex(index, value, new HashSet<BindableList<T>>());
set
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
setIndex(index, value, ref instances);
}
}

private void setIndex(int index, T item, HashSet<BindableList<T>> appliedInstances)
private void setIndex(int index, T item, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -73,7 +84,7 @@ private void setIndex(int index, T item, HashSet<BindableList<T>> appliedInstanc
if (bindings != null)
{
foreach (var b in bindings)
b.setIndex(index, item, appliedInstances);
b.setIndex(index, item, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, lastItem, index));
Expand All @@ -85,11 +96,15 @@ private void setIndex(int index, T item, HashSet<BindableList<T>> appliedInstanc
/// <param name="item">The item to be added.</param>
/// <exception cref="InvalidOperationException">Thrown when this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public void Add(T item)
=> add(item, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
add(item, ref instances);
}

private void add(T item, HashSet<BindableList<T>> appliedInstances)
private void add(T item, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -98,7 +113,7 @@ private void add(T item, HashSet<BindableList<T>> appliedInstances)
if (bindings != null)
{
foreach (var b in bindings)
b.add(item, appliedInstances);
b.add(item, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, collection.Count - 1));
Expand All @@ -118,11 +133,15 @@ private void add(T item, HashSet<BindableList<T>> appliedInstances)
/// <param name="item">The item to insert.</param>
/// <exception cref="InvalidOperationException">Thrown when this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public void Insert(int index, T item)
=> insert(index, item, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
insert(index, item, ref instances);
}

private void insert(int index, T item, HashSet<BindableList<T>> appliedInstances)
private void insert(int index, T item, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -131,7 +150,7 @@ private void insert(int index, T item, HashSet<BindableList<T>> appliedInstances
if (bindings != null)
{
foreach (var b in bindings)
b.insert(index, item, appliedInstances);
b.insert(index, item, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index));
Expand All @@ -142,11 +161,15 @@ private void insert(int index, T item, HashSet<BindableList<T>> appliedInstances
/// </summary>
/// <exception cref="InvalidOperationException">Thrown when this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public void Clear()
=> clear(new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
clear(ref instances);
}

private void clear(HashSet<BindableList<T>> appliedInstances)
private void clear(ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -163,7 +186,7 @@ private void clear(HashSet<BindableList<T>> appliedInstances)
if (bindings != null)
{
foreach (var b in bindings)
b.clear(appliedInstances);
b.clear(ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, clearedItems, 0));
Expand All @@ -184,11 +207,14 @@ public bool Contains(T item)
/// <returns><code>true</code> if the removal was successful.</returns>
/// <exception cref="InvalidOperationException">Thrown if this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public bool Remove(T item)
=> remove(item, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
return remove(item, ref instances);
}

private bool remove(T item, HashSet<BindableList<T>> appliedInstances)
private bool remove(T item, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances))
if (!instances.Add(instanceId))
return false;

ensureMutationAllowed();
Expand All @@ -210,12 +236,11 @@ private bool remove(T item, HashSet<BindableList<T>> appliedInstances)
{
// prevent re-removing from the callee.
// That would result in a <see cref="StackOverflowException"/>.
b.remove(listItem, appliedInstances);
b.remove(listItem, ref instances);
}
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, listItem, index));

return true;
}

Expand All @@ -226,12 +251,14 @@ private bool remove(T item, HashSet<BindableList<T>> appliedInstances)
/// <param name="count">The count of items to be removed.</param>
public void RemoveRange(int index, int count)
{
removeRange(index, count, new HashSet<BindableList<T>>());
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
removeRange(index, count, ref instances);
}

private void removeRange(int index, int count, HashSet<BindableList<T>> appliedInstances)
private void removeRange(int index, int count, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -250,7 +277,7 @@ private void removeRange(int index, int count, HashSet<BindableList<T>> appliedI
{
// Prevent re-adding the item back to the callee.
// That would result in a <see cref="StackOverflowException"/>.
b.removeRange(index, count, appliedInstances);
b.removeRange(index, count, ref instances);
}
}

Expand All @@ -263,11 +290,15 @@ private void removeRange(int index, int count, HashSet<BindableList<T>> appliedI
/// <param name="index">The index of the item to remove.</param>
/// <exception cref="InvalidOperationException">Thrown if this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public void RemoveAt(int index)
=> removeAt(index, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
removeAt(index, ref instances);
}

private void removeAt(int index, HashSet<BindableList<T>> appliedInstances)
private void removeAt(int index, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -278,7 +309,7 @@ private void removeAt(int index, HashSet<BindableList<T>> appliedInstances)
if (bindings != null)
{
foreach (var b in bindings)
b.removeAt(index, appliedInstances);
b.removeAt(index, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index));
Expand All @@ -289,11 +320,14 @@ private void removeAt(int index, HashSet<BindableList<T>> appliedInstances)
/// </summary>
/// <param name="match">The predicate.</param>
public int RemoveAll(Predicate<T> match)
=> removeAll(match, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
return removeAll(match, ref instances);
}

private int removeAll(Predicate<T> match, HashSet<BindableList<T>> appliedInstances)
private int removeAll(Predicate<T> match, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances))
if (!instances.Add(instanceId))
return 0;

ensureMutationAllowed();
Expand All @@ -310,7 +344,7 @@ private int removeAll(Predicate<T> match, HashSet<BindableList<T>> appliedInstan
if (bindings != null)
{
foreach (var b in bindings)
b.removeAll(match, appliedInstances);
b.removeAll(match, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removedItems));
Expand All @@ -324,11 +358,15 @@ private int removeAll(Predicate<T> match, HashSet<BindableList<T>> appliedInstan
/// <param name="count">The count of items to be removed.</param>
/// <param name="newItems">The items to replace the removed items with.</param>
public void ReplaceRange(int index, int count, IEnumerable<T> newItems)
=> replaceRange(index, count, newItems as ICollection<T> ?? newItems.ToArray(), new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
replaceRange(index, count, newItems as ICollection<T> ?? newItems.ToArray(), ref instances);
}

private void replaceRange(int index, int count, ICollection<T> newItems, HashSet<BindableList<T>> appliedInstances)
private void replaceRange(int index, int count, ICollection<T> newItems, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -348,26 +386,13 @@ private void replaceRange(int index, int count, ICollection<T> newItems, HashSet
{
// Prevent re-adding the item back to the callee.
// That would result in a <see cref="StackOverflowException"/>.
b.replaceRange(index, count, newItems, appliedInstances);
b.replaceRange(index, count, newItems, ref instances);
}
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newItems, removedItems, index));
}

/// <summary>
/// As we are applying non-atomic operations on a potentially complex bindable tree, care needs to be taken to not apply
/// the same operation twice to the same bindable instance.
///
/// This call tracks all instances which an operation has already been applied to.
/// </summary>
/// <param name="appliedInstances">A hash set to be passed down the recursive call tree tracking all applied instances.</param>
/// <returns>Whether the current instance has already been applied.</returns>
private bool checkAlreadyApplied(HashSet<BindableList<T>> appliedInstances)
{
return !appliedInstances.Add(this);
}

/// <summary>
/// Copies the contents of this <see cref="BindableList{T}"/> to the given array, starting at the given index.
/// </summary>
Expand Down Expand Up @@ -549,11 +574,15 @@ public virtual void UnbindFrom(IUnbindable them)
/// <param name="items">The collection whose items should be added to this collection.</param>
/// <exception cref="InvalidOperationException">Thrown if this collection is <see cref="Disabled"/></exception>
public void AddRange(IEnumerable<T> items)
=> addRange(items as ICollection<T> ?? items.ToArray(), new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
addRange(items as ICollection<T> ?? items.ToArray(), ref instances);
}

private void addRange(ICollection<T> items, HashSet<BindableList<T>> appliedInstances)
private void addRange(ICollection<T> items, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -562,7 +591,7 @@ private void addRange(ICollection<T> items, HashSet<BindableList<T>> appliedInst
if (bindings != null)
{
foreach (var b in bindings)
b.addRange(items, appliedInstances);
b.addRange(items, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, (IList)items, collection.Count - items.Count));
Expand All @@ -574,11 +603,15 @@ private void addRange(ICollection<T> items, HashSet<BindableList<T>> appliedInst
/// <param name="oldIndex">The index of the item to move.</param>
/// <param name="newIndex">The index specifying the new location of the item.</param>
public void Move(int oldIndex, int newIndex)
=> move(oldIndex, newIndex, new HashSet<BindableList<T>>());
{
InstanceTracker instances = new InstanceTracker(stackalloc int[16]);
move(oldIndex, newIndex, ref instances);
}

private void move(int oldIndex, int newIndex, HashSet<BindableList<T>> appliedInstances)
private void move(int oldIndex, int newIndex, ref InstanceTracker instances)
{
if (checkAlreadyApplied(appliedInstances)) return;
if (!instances.Add(instanceId))
return;

ensureMutationAllowed();

Expand All @@ -590,7 +623,7 @@ private void move(int oldIndex, int newIndex, HashSet<BindableList<T>> appliedIn
if (bindings != null)
{
foreach (var b in bindings)
b.move(oldIndex, newIndex, appliedInstances);
b.move(oldIndex, newIndex, ref instances);
}

CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, item, newIndex, oldIndex));
Expand Down
Loading

0 comments on commit 58f3221

Please sign in to comment.