Improve BindableList performance
#6405
Conversation
Remove usages of `Cast<T>` by using `ICollection<T>`. `List<T>` internally optimises across `ICollection<T>`, so doing this removes an enumeration. Replaced `notifyCollectionChanged()` with local invocations, allowing null-check to happen. Only capture previous states (e.g. the cleared items during `Clear()`) if there's a subscription to `CollectionChanged`.
| // This will return the unique runtime identity for the object. | ||
| instanceId = RuntimeHelpers.GetHashCode(this); |
There was a problem hiding this comment.
I immediately don't like whatever this is.
To quote docs:
Although the RuntimeHelpers.GetHashCode method returns identical hash codes for identical object references, you should not use this method to test for object identity, because this hash code does not uniquely identify an object reference. To test for object identity (that is, to test that two objects reference the same object in memory), call the Object.ReferenceEquals method. Nor should you use GetHashCode to test whether two strings represent equal object references, because the string is interned. To test for string interning, call the String.IsInterned method.
I do not want to ever debug a situation wherein a one-in-a-trillion hash collision happened and suddenly two bindable lists are freakishly joined at the hip. The hash code isn't even 64 bits, it's a plain int.
I'm not even sure I can bring myself to read the rest of this diff after that, because it looks mightly complicated and all of it seems predicated on the (in my opinion) flawed premise that this hash code never collides.
There was a problem hiding this comment.
Dang my understanding of this method was incorrect. What you said makes sense in hindsight...
That said, I'm somewhat happy this blocks the PR because I believe the cycle detection can be done better. More tricks to pull from my sleeve :)
RFC.
I'm still looking for ways to quantify this, to the point that it may not be worth it if the comprehension overhead is too large... Basically, I started optimising diffcalc and reached a point where
BindableList.addRange()andBindableList.clear()were the two biggest hotspots. This is on-top of various other (potentially upcoming) optimisations applied.A few optimisations are applied here:
ICollection<T>instead ofIListto avoid a.Cast<>enumeration.HashSetallocation used for cycle detection. It will use up to 64B of stack space (16 bindables) before falling back toHashSet. In practice (in game), I haven't foundBindableListinvocation chains longer than 10 calls, so this should cover the majority of cases.Over a 20s
osu-difficulty-calculator(16T) run period...Before:
After:
If you think it doesn't look like much, I would agree with you. Diffcalc seems to be one of those things where you optimise one part and it moves the hotspot down the chain.
In particular I'm interested in the alloc savings, because diffcalc is mostly GC-limited as far as I can tell (allocates ~1GB/s). Keep in mind the above results are per call.