Improve WeakList performance during staggered removals #3893
Conversation
|
Is the optimised case common enough to warrant 5-18% decrease across all other benchmarks? Probably worth touching on that for the record. |
|
I made a few more optimisations that bring it closer, and updated the benchmarks. The difference is coming from the additional size of On a profiling of loading of Centipede (the non-optimised version from earlier today), which creates all those bindables, I see the following: Also, in most of our usages, bindables are loaded async and then have their value changed event bound to in |
| // 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. |
There was a problem hiding this comment.
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.
osu.Framework/Lists/WeakList.cs
Outdated
| { | ||
| Reference = new WeakReference<T>(reference); | ||
| ObjectHashCode = reference == null ? 0 : EqualityComparer<T>.Default.GetHashCode(reference); |
There was a problem hiding this comment.
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
GetHashCodealways 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.
There was a problem hiding this comment.
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).
|
I'd also recommend testing with https://osu.ppy.sh/beatmapsets/121591#osu/311269, for me exiting goes from 10s -> 1s after this change. |
|
I think the code looks good now and can reproduce performance improvements. I can get behind the enabling of nullable reference types done in these files in the last commit, but I'm not entirely sure we'll all be on the same page in that matter. @peppy is that ok by you? |
|
Seems local enough to not be an issue. |
Description
"Staggered removals" are removals of items appearing mostly in the middle of the list and rarely at the ends of the list - they do not follow a linear pattern, and are hard to optimise.
WeakListalready employs optimisations when items are removed at the ends of the list.For
WeakList, such staggered cases require most elements in the list to be iterated over for subsequent removals to take place, including elements that have lost their references and/or have been previouslyRemove()d.When this happens,
WeakListcallsTryGetTargeton all elements that haven't beenRemove()d, which turns out to be quite expensive if done too much - retrieving objects throughWeakReferenceis ~6x more expensive than via reference in my benchmarking.Solution
The only two cases where this comes up are
Remove()andContains(), both of which don't really need to know the exact object stored - only that the given object is present somewhere in the list. The object is guaranteed to be alive due to the nature of these two methods.So for the purpose of the above two methods, this change stores the object's hash code to perform comparisons against, eliminating calls to
TryGetTarget.Benchmarks
Framework-side benchmarks
Before
After
osu!-side testing
Tested exiting from gameplay on Centipede:
Before
After