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

Improve BindableList performance + reduce allocations #6450

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Improve BindableList performance + reduce allocations #6450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98e7875
Add/improve benchmarks
smoogipoo Oct 31, 2024
a4bbf25
General performance improvements
smoogipoo Oct 31, 2024
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
73 changes: 55 additions & 18 deletions osu.Framework.Benchmarks/BenchmarkBindableList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,87 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Linq;
using BenchmarkDotNet.Attributes;
using osu.Framework.Bindables;

namespace osu.Framework.Benchmarks
{
[MemoryDiagnoser]
[WarmupCount(0)]
[IterationCount(2)]
public class BenchmarkBindableList
{
private readonly BindableList<int> list = new BindableList<int>();
private IBindableList<int> iList => list;
private static readonly int[] small_data = Enumerable.Range(0, 10).ToArray();

[Params(0, 1, 10, 20)]
public int NumBindings { get; set; }

private BindableList<int>[] lists = null!;

[GlobalSetup]
public void GlobalSetup()
{
for (int i = 0; i < 10; i++)
list.Add(i);
lists = new BindableList<int>[NumBindings + 1];

lists[0] = new BindableList<int>(Enumerable.Range(0, 10000).ToArray());
for (int i = 1; i < lists.Length; i++)
lists[i] = lists[i - 1].GetBoundCopy();
lists[0].Clear();
}

[Benchmark(Baseline = true)]
public void Create()
{
setupList();
}

[Benchmark]
public int Enumerate()
public void Add()
{
int result = 0;
setupList().Add(1);
}

for (int i = 0; i < 100; i++)
{
foreach (int val in list)
result += val;
}
[Benchmark]
public void Remove()
{
setupList().Remove(0);
}

return result;
[Benchmark]
public void Clear()
{
setupList().Clear();
}

[Benchmark]
public int EnumerateInterface()
public void AddRange()
{
setupList().AddRange(small_data);
}

[Benchmark]
public void SetIndex()
{
setupList()[0]++;
}

[Benchmark]
public int Enumerate()
{
int result = 0;

for (int i = 0; i < 100; i++)
{
foreach (int val in iList)
result += val;
}
foreach (int val in setupList())
result += val;

return result;
}

private BindableList<int> setupList()
{
lists[0].Clear();
lists[0].Add(0);
return lists[0];
}
}
}
68 changes: 34 additions & 34 deletions osu.Framework/Bindables/BindableList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ private void setIndex(int index, T item, HashSet<BindableList<T>> appliedInstanc
b.setIndex(index, item, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, lastItem, index));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, lastItem, index));
}

/// <summary>
Expand All @@ -101,7 +101,7 @@ private void add(T item, HashSet<BindableList<T>> appliedInstances)
b.add(item, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, collection.Count - 1));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, collection.Count - 1));
}

/// <summary>
Expand Down Expand Up @@ -134,7 +134,7 @@ private void insert(int index, T item, HashSet<BindableList<T>> appliedInstances
b.insert(index, item, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index));
}

/// <summary>
Expand All @@ -154,7 +154,7 @@ private void clear(HashSet<BindableList<T>> appliedInstances)
return;

// Preserve items for subscribers
var clearedItems = collection.ToList();
List<T> clearedItems = CollectionChanged == null ? null : collection.ToList();

collection.Clear();

Expand All @@ -164,7 +164,7 @@ private void clear(HashSet<BindableList<T>> appliedInstances)
b.clear(appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, clearedItems, 0));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, clearedItems, 0));
}

/// <summary>
Expand Down Expand Up @@ -212,7 +212,7 @@ private bool remove(T item, HashSet<BindableList<T>> appliedInstances)
}
}

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

return true;
}
Expand All @@ -233,12 +233,13 @@ private void removeRange(int index, int count, HashSet<BindableList<T>> appliedI

ensureMutationAllowed();

var removedItems = collection.GetRange(index, count);
if (count == 0)
return;

collection.RemoveRange(index, count);
// Preserve items for subscribers
List<T> removedItems = CollectionChanged == null ? null : collection.GetRange(index, count);

if (removedItems.Count == 0)
return;
collection.RemoveRange(index, count);

if (bindings != null)
{
Expand All @@ -250,7 +251,7 @@ private void removeRange(int index, int count, HashSet<BindableList<T>> appliedI
}
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removedItems, index));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removedItems, index));
}

/// <summary>
Expand All @@ -277,7 +278,7 @@ private void removeAt(int index, HashSet<BindableList<T>> appliedInstances)
b.removeAt(index, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index));
}

/// <summary>
Expand All @@ -294,22 +295,22 @@ private int removeAll(Predicate<T> match, HashSet<BindableList<T>> appliedInstan

ensureMutationAllowed();

var removed = collection.FindAll(match);

if (removed.Count == 0) return removed.Count;
// Preserve items for subscribers
List<T> removedItems = CollectionChanged == null ? null : collection.FindAll(match);

// RemoveAll is internally optimised
collection.RemoveAll(match);
int countRemoved = collection.RemoveAll(match);
if (countRemoved == 0)
return 0;

if (bindings != null)
{
foreach (var b in bindings)
b.removeAll(match, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removed));

return removed.Count;
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removedItems));
return countRemoved;
}

/// <summary>
Expand All @@ -319,21 +320,22 @@ 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 IList ?? newItems.ToArray(), new HashSet<BindableList<T>>());
=> replaceRange(index, count, newItems as ICollection<T> ?? newItems.ToArray(), new HashSet<BindableList<T>>());

private void replaceRange(int index, int count, IList newItems, HashSet<BindableList<T>> appliedInstances)
private void replaceRange(int index, int count, ICollection<T> newItems, HashSet<BindableList<T>> appliedInstances)
{
if (checkAlreadyApplied(appliedInstances)) return;

ensureMutationAllowed();

var removedItems = collection.GetRange(index, count);
if (count == 0 && newItems.Count == 0)
return;

collection.RemoveRange(index, count);
collection.InsertRange(index, newItems.Cast<T>());
// Preserve items for subscribers
List<T> removedItems = CollectionChanged == null ? null : collection.GetRange(index, count);

if (removedItems.Count == 0 && newItems.Count == 0)
return;
collection.RemoveRange(index, count);
collection.InsertRange(index, newItems);

if (bindings != null)
{
Expand All @@ -345,7 +347,7 @@ private void replaceRange(int index, int count, IList newItems, HashSet<Bindable
}
}

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

/// <summary>
Expand Down Expand Up @@ -542,23 +544,23 @@ 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 IList ?? items.ToArray(), new HashSet<BindableList<T>>());
=> addRange(items as ICollection<T> ?? items.ToArray(), new HashSet<BindableList<T>>());

private void addRange(IList items, HashSet<BindableList<T>> appliedInstances)
private void addRange(ICollection<T> items, HashSet<BindableList<T>> appliedInstances)
{
if (checkAlreadyApplied(appliedInstances)) return;

ensureMutationAllowed();

collection.AddRange(items.Cast<T>());
collection.AddRange(items);

if (bindings != null)
{
foreach (var b in bindings)
b.addRange(items, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, items, collection.Count - items.Count));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, (IList)items, collection.Count - items.Count));
}

/// <summary>
Expand Down Expand Up @@ -586,7 +588,7 @@ private void move(int oldIndex, int newIndex, HashSet<BindableList<T>> appliedIn
b.move(oldIndex, newIndex, appliedInstances);
}

notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, item, newIndex, oldIndex));
CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, item, newIndex, oldIndex));
}

void IBindable.BindTo(IBindable them)
Expand Down Expand Up @@ -680,8 +682,6 @@ private void addWeakReference(WeakReference<BindableList<T>> weakReference)

#endregion IEnumerable

private void notifyCollectionChanged(NotifyCollectionChangedEventArgs args) => CollectionChanged?.Invoke(this, args);

private void ensureMutationAllowed()
{
if (Disabled)
Expand Down
Loading