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

Jump to bottom

Improve WeakList performance during staggered removals #3893

Merged
merged 14 commits into from Sep 25, 2020
Merged

Improve WeakList performance during staggered removals #3893

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
748b348
Improve weaklist performance
smoogipoo Sep 23, 2020
de44497
Add test
smoogipoo Sep 23, 2020
de1df2a
Improve code quality, only iterate over valid items normally
smoogipoo Sep 23, 2020
0fedeba
Make method private
smoogipoo Sep 23, 2020
0ddd33e
Rename parameter
smoogipoo Sep 23, 2020
1a6aaf1
Improve performance a bit
smoogipoo Sep 23, 2020
7bb4ef0
Fix current item not being set correctly
smoogipoo Sep 23, 2020
49f4e81
Remove unnecessary line
smoogipoo Sep 23, 2020
ad63553
Don't use weakreference as second-chance
smoogipoo Sep 23, 2020
c0558e9
Split enumerators and cleanup
smoogipoo Sep 23, 2020
43ce956
Use object equality as absolute truth
smoogipoo Sep 24, 2020
614b0ef
Remove enumerator in favour of manual enumeration
smoogipoo Sep 24, 2020
39d0b5f
Remove null handling
smoogipoo Sep 24, 2020
ba2f666
Merge branch 'master' into weaklist-improvement
peppy Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Framework/Lists/LockedWeakList.cs
View file
Open in desktop
Expand Up @@ -74,7 +74,7 @@ public struct Enumerator : IEnumerator<T>
{
private readonly WeakList<T> list;

private WeakList<T>.Enumerator listEnumerator;
private WeakList<T>.ValidItemsEnumerator listEnumerator;

private readonly bool lockTaken;

Expand Down
137 changes: 13 additions & 124 deletions osu.Framework/Lists/WeakList.cs
View file
Open in desktop
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using JetBrains.Annotations;

namespace osu.Framework.Lists
Expand All @@ -13,7 +12,7 @@ namespace osu.Framework.Lists
/// A list maintaining weak reference of objects.
/// </summary>
/// <typeparam name="T">Type of items tracked by weak reference.</typeparam>
public class WeakList<T> : IWeakList<T>, IEnumerable<T>
public partial class WeakList<T> : IWeakList<T>, IEnumerable<T>
where T : class
{
private readonly List<InvalidatableWeakReference> list = new List<InvalidatableWeakReference>();
Expand All @@ -37,14 +36,14 @@ private void add(in InvalidatableWeakReference item)
public bool Remove(T item)
{
int hashCode = item == null ? 0 : EqualityComparer<T>.Default.GetHashCode(item);
var enumerator = getEnumeratorNoTrim();
var enumerator = new AllItemsEnumerator(this);

while (enumerator.MoveNext())
{
if (!enumerator.CheckEquals(item, hashCode))
if (!enumerator.CheckEquals(hashCode))
continue;

RemoveAt(enumerator.CurrentItemIndex);
RemoveAt(enumerator.CurrentItemIndex - listStart);
return true;
}

Expand All @@ -53,14 +52,14 @@ public bool Remove(T item)

public bool Remove(WeakReference<T> weakReference)
{
var enumerator = getEnumeratorNoTrim();
var enumerator = new AllItemsEnumerator(this);

while (enumerator.MoveNext())
{
if (!enumerator.CheckEquals(weakReference))
continue;

RemoveAt(enumerator.CurrentItemIndex);
RemoveAt(enumerator.CurrentItemIndex - listStart);
return true;
}

Expand All @@ -86,11 +85,11 @@ public void RemoveAt(int index)
public bool Contains(T item)
{
int hashCode = item == null ? 0 : EqualityComparer<T>.Default.GetHashCode(item);
var enumerator = getEnumeratorNoTrim();
var enumerator = new AllItemsEnumerator(this);

while (enumerator.MoveNext())
{
if (enumerator.CheckEquals(item, hashCode))
if (enumerator.CheckEquals(hashCode))
return true;
}

Expand All @@ -99,7 +98,7 @@ public bool Contains(T item)

public bool Contains(WeakReference<T> weakReference)
{
var enumerator = getEnumeratorNoTrim();
var enumerator = new AllItemsEnumerator(this);

while (enumerator.MoveNext())
{
Expand All @@ -112,7 +111,7 @@ public bool Contains(WeakReference<T> weakReference)

public void Clear() => listStart = listEnd = 0;

public Enumerator GetEnumerator()
public ValidItemsEnumerator GetEnumerator()
{
// Trim from the sides - items that have been removed.
list.RemoveRange(listEnd, list.Count - listEnd);
Expand All @@ -125,123 +124,13 @@ public Enumerator GetEnumerator()
listStart = 0;
listEnd = list.Count;

return getEnumeratorNoTrim(true);
return new ValidItemsEnumerator(this);
}

IEnumerator<T> IEnumerable<T>.GetEnumerator() => GetEnumerator();

IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

/// <summary>
/// Creates a new <see cref="Enumerator"/> over this <see cref="WeakList{T}"/>.
/// </summary>
/// <param name="checkValidity">Whether only the valid items of this <see cref="WeakList{T}"/> should be enumerated.
/// If <c>false</c>, the user must check the validity of <see cref="Enumerator.Current"/> prior to usage.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private Enumerator getEnumeratorNoTrim(bool checkValidity = false) => new Enumerator(this, checkValidity);

/// <summary>
/// A <see cref="WeakList{T}"/> enumerator.
/// </summary>
public struct Enumerator : IEnumerator<T>
{
private WeakList<T> weakList;
private readonly bool checkValidity;

/// <summary>
/// Creates a new <see cref="Enumerator"/>.
/// </summary>
/// <param name="weakList">The <see cref="WeakList{T}"/> to enumerate over.</param>
/// <param name="checkValidity">Whether only the valid items in <paramref name="weakList"/> should be enumerated.
/// If <c>false</c>, the user must check the validity of <see cref="Current"/> prior to usage.</param>
internal Enumerator(WeakList<T> weakList, bool checkValidity)
{
this.weakList = weakList;
this.checkValidity = checkValidity;

CurrentItemIndex = -1; // The first MoveNext() should bring the iterator to the start
currentItem = default;
currentObject = null;
}

public bool MoveNext()
{
while (true)
{
++CurrentItemIndex;

int index = weakList.listStart + CurrentItemIndex;

// Check whether we're still within the valid range of the list.
if (index >= weakList.listEnd)
return false;

var weakReference = weakList.list[index].Reference;

// Check whether the reference exists.
if (weakReference == null)
{
// If the reference doesn't exist, it must have previously been removed and can be skipped.
continue;
}

currentItem = weakList.list[index];

if (checkValidity)
{
if ((currentObject = getCurrentObject()) == null)
{
// If the object can't be retrieved, mark the reference for removal.
// The removal will occur on the _next_ enumeration (see: GetEnumerator()).
weakList.RemoveAt(CurrentItemIndex);
continue;
}
}

return true;
}
}

public void Reset()
{
CurrentItemIndex = -1;
currentItem = default;
currentObject = null;
}

public readonly T Current => checkValidity ? currentObject : getCurrentObject();

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private readonly T getCurrentObject()
{
T obj = null;
currentItem.Reference?.TryGetTarget(out obj);
return obj;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public readonly bool CheckEquals(T obj, int hashCode)
=> currentItem.ObjectHashCode == hashCode;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public readonly bool CheckEquals(WeakReference<T> weakReference)
=> currentItem.Reference == weakReference;

private InvalidatableWeakReference currentItem;
private T currentObject;

internal int CurrentItemIndex { get; private set; }

readonly object IEnumerator.Current => Current;

public void Dispose()
{
weakList = null;
currentItem = default;
currentObject = null;
}
}

internal readonly struct InvalidatableWeakReference
{
[CanBeNull]
Expand All @@ -252,13 +141,13 @@ public void Dispose()
/// </summary>
public readonly int ObjectHashCode;

public InvalidatableWeakReference([CanBeNull] T reference)
public InvalidatableWeakReference(T reference)
{
Reference = new WeakReference<T>(reference);
ObjectHashCode = reference == null ? 0 : EqualityComparer<T>.Default.GetHashCode(reference);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know that this is correct, unfortunately... For one thing, this can break if the class implements IEquatable<T>, but even if you use the more-correct-for-that-case RuntimeHelpers.GetHashCode(), a docs note states:

Note that GetHashCode always returns identical hash codes for equal object references. However, the reverse is not true: equal hash codes do not indicate equal object references. A particular hash code value is not unique to a particular object reference; different object references can generate identical hash codes.

Unless I'm reading this wrong, this indicates that collisions are potentially possible and that the hash code is not an uniquely-identifying invertible function. Sure, that possibility is probably slim, but I don't know that we ever want to debug one of those...

I suppose for full correctness this would be salvageable by keeping hash buckets, iterating over those rather than stopping at first match, and using ReferenceEquals for absolute certainty, but that might kneecap the optimisation completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's little cost associated with using object equality as absolute truth. It's technically already using "hash buckets", so it'll degenerate back to the original performance with many hash collisions, but that's a rare case.

I'll keep using EqualityComparer though - want to avoid boxing.

Updated the o!f benchmarks, only about 100us difference in RemoveAllStaggered(1000).

}

public InvalidatableWeakReference([CanBeNull] WeakReference<T> weakReference)
public InvalidatableWeakReference(WeakReference<T> weakReference)
{
Reference = weakReference;

Expand Down
70 changes: 70 additions & 0 deletions osu.Framework/Lists/WeakList_AllItemsEnumerator.cs
View file
Open in desktop
@@ -0,0 +1,70 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace osu.Framework.Lists
{
public partial class WeakList<T>
{
/// <summary>
/// An enumerator over all items in a <see cref="WeakList{T}"/>. Does not guarantee the validity of items.
/// </summary>
private struct AllItemsEnumerator : IEnumerator<T>
smoogipoo marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly WeakList<T> weakList;

/// <summary>
/// Creates a new <see cref="AllItemsEnumerator"/>.
/// </summary>
/// <param name="weakList">The <see cref="WeakList{T}"/> to enumerate over.</param>
internal AllItemsEnumerator(WeakList<T> weakList)
{
this.weakList = weakList;

CurrentItemIndex = weakList.listStart - 1; // The first MoveNext() should bring the iterator to the start
}

public bool MoveNext()
{
while (true)
{
++CurrentItemIndex;

// Check whether we're still within the valid range of the list.
if (CurrentItemIndex >= weakList.listEnd)
return false;

if (weakList.list[CurrentItemIndex].Reference != null)
return true;
}
}

public void Reset()
{
CurrentItemIndex = weakList.listStart - 1;
}

public readonly T Current => throw new NotImplementedException("This enumerator doesn't support retrieving the current item.");

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public readonly bool CheckEquals(int hashCode)
=> weakList.list[CurrentItemIndex].ObjectHashCode == hashCode;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public readonly bool CheckEquals(WeakReference<T> weakReference)
=> weakList.list[CurrentItemIndex].Reference == weakReference;

internal int CurrentItemIndex { get; private set; }

readonly object IEnumerator.Current => Current;

public void Dispose()
{
}
}
}
}
72 changes: 72 additions & 0 deletions osu.Framework/Lists/WeakList_ValidItemsEnumerator.cs
View file
Open in desktop
@@ -0,0 +1,72 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Collections;
using System.Collections.Generic;

namespace osu.Framework.Lists
{
public partial class WeakList<T>
{
/// <summary>
/// An enumerator over only the valid items of a <see cref="WeakList{T}"/>.
/// </summary>
public struct ValidItemsEnumerator : IEnumerator<T>
{
private WeakList<T> weakList;
private int currentItemIndex;

/// <summary>
/// Creates a new <see cref="ValidItemsEnumerator"/>.
/// </summary>
/// <param name="weakList">The <see cref="WeakList{T}"/> to enumerate over.</param>
internal ValidItemsEnumerator(WeakList<T> weakList)
{
this.weakList = weakList;

currentItemIndex = weakList.listStart - 1; // The first MoveNext() should bring the iterator to the start
Current = null;
}

public bool MoveNext()
{
while (true)
{
++currentItemIndex;

// Check whether we're still within the valid range of the list.
if (currentItemIndex >= weakList.listEnd)
return false;

var weakReference = weakList.list[currentItemIndex].Reference;

// Check whether the reference exists.
if (weakReference == null || !weakReference.TryGetTarget(out var obj))
{
// If the reference doesn't exist, it must have previously been removed and can be skipped.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I've removed the RemoveAt() code from here since the enumerators have been split out and it was unnecessary overhead.

This enumerator is only used by GetEnumerator() which does its own pre-trimming, so most of its use is negated.

continue;
}

Current = obj;
return true;
}
}

public void Reset()
{
currentItemIndex = weakList.listStart - 1;
Current = null;
}

public T Current { get; private set; }

readonly object IEnumerator.Current => Current;

public void Dispose()
{
weakList = null;
Current = null;
}
}
}
}