Change `ValueChangedEvent` to a `struct`

[peppy]

There's very little reason for this to be a class.

It is constructed by each bindable locally when it has listeners, and generally never modified or passed further. Changing to a struct reduces GC overhead.

(honestly we shouldn't be firing these enough for it to be an issue in the first place, but there's some egregious usage of bindables in the game which needs further consideration to fix, and this may alleviate things somewhat).

Not sure whether this needs profiling. A well structured benchmark may show lower GC counts, but allocations should not change.

osu!-side changes:

diff --git a/osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs b/osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs
index 2cd8e45d28..d9c7ee3ffd 100644
--- a/osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs
+++ b/osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs
@@ -493,7 +493,7 @@ private void endHandlingTrack()
             cancelTrackLooping();
         }
 
-        private void applyLoopingToTrack(ValueChangedEvent<WorkingBeatmap> _ = null)
+        private void applyLoopingToTrack(ValueChangedEvent<WorkingBeatmap> _ = default)
         {
             if (!this.IsCurrentScreen())
                 return;
diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs
index 2d5c44e5a5..6b229936d5 100644
--- a/osu.Game/Screens/Select/SongSelect.cs
+++ b/osu.Game/Screens/Select/SongSelect.cs
@@ -493,9 +493,9 @@ public void FinaliseSelection(BeatmapInfo? beatmapInfo = null, RulesetInfo? rule
 
         private ScheduledDelegate? selectionChangedDebounce;
 
-        private void updateCarouselSelection(ValueChangedEvent<WorkingBeatmap>? e = null)
+        private void updateCarouselSelection(ValueChangedEvent<WorkingBeatmap> e = default)
         {
-            var beatmap = e?.NewValue ?? Beatmap.Value;
+            var beatmap = e.NewValue ?? Beatmap.Value;
             if (beatmap is DummyWorkingBeatmap || !this.IsCurrentScreen()) return;
 
             Logger.Log($"Song select working beatmap updated to {beatmap}");

[bdach]

My main problem with this change is that I have no idea how to gauge if it does anything good or if it breaks anything (loudly or silently).

Like yes it takes 5 minutes to review the diff but this may as well break somewhere in the hundreds of usages of the struct somewhere else due to type semantics changing. And without conclusive profiling I'm not even seeing what we get out of this. It is not obvious to me that the decreased heap pressure is worth trading off for increased number of struct copies.

I'm currently running game-side test suites as an initial sweep in a weak attempt to estimate the blast radius of this. But I don't know what comes next.

[peppy]

for increased number of struct copies.

Do you have an example of where this struct is copied? Most every usage I looked at was (as you'd hope) immediate usage and then throw-away. That's the only reason I made this change and am confident in doing so without much thought.

[bdach]

Do you have an example of where this struct is copied

No but that's part of the point. I'm not sure how I would be able to tell where it is or isn't copied without spending hours combing through every usage.

[peppy]

Well even if it's copies, ignoring performance issues (since logically i'm sure we can agree any copies would be a an edge case) it should be enough to check for zero mutation of the event's properties to ensure behaviour is not regressed, right?

[bdach]

since logically i'm sure we can agree any copies would be a an edge case

Yeah see I'm not sure about that either.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#passing-structure-type-variables-by-reference says:

When you pass a structure-type variable to a method as an argument or return a structure-type value from a method, the whole instance of a structure type is copied. Pass by value can affect the performance of your code in high-performance scenarios that involve large structure types. You can avoid value copying by passing a structure-type variable by reference. Use the ref, out, in, or ref readonly method parameter modifiers to indicate that an argument must be passed by reference. Use ref returns to return a method result by reference. For more information, see Avoid allocations.

Which suggests that copies are going to be happening as there is no readonly carve-out here, and moreover, they're not going to be rare, since the point of the thing is to be passed to callbacks, which - if I am interpreting the above correctly - will incur struct copies rather than by-ref passing that was there before.

If we were wanting to fix that, we'd have to change the signature of ValueChanged et al. to pass the struct by ref (or probably in), and I don't think I need to explain how sweeping of a change that would be...

[bdach]

I tried to write a benchmark to confirm or deny the above and I'm even more confused than I was before.

https://gist.github.com/bdach/80815935278b1ff580cebe16f9931ec3

The results make zero sense to me.

• Why are classes twice as slow?
• Why is there no difference between struct, readonly struct by-val, and readonly struct by-in?

Did I screw up the benchmark somehow?

[smoogipoo]

Why are classes twice as slow?

Likely due to having to dereference from non-contiguous references.

Why is there no difference between struct, readonly struct by-val, and readonly struct by-in?

Because the JIT is smart enough to pass structs by value if they're small enough. You should see an effect by increasing the size of the struct.

https://devblogs.microsoft.com/premier-developer/the-in-modifier-and-the-readonly-structs-in-c/

[bdach]

You should see an effect by increasing the size of the struct

I spammed a few extra fields... but am equally unsure how to interpret the results...?

Okay sure the gen0-2 counts are gone (which is expected? I think?) but also structs are now... slower? Universally, too? in doesn't seem to change much?

I must be somehow benchmarking wrong because this is nonsense. The blogpost linked above even says this:

If the size of a readonly struct is bigger than IntPtr.Size you should pass it as an in-parameter for performance reasons.

which confirms my initial thinking, and is kind of where I'm stuck at deciding if it should be in for not, because the size of the struct is gonna depend on the generic T in ValueChangedEvent, but I think it's likely that 60-70% of usages are going to exceed that (because either two references, which will be IntPtr.Size wide each, or a value type larger than IntPtr.Size, which is long / double / probably most non-trivial structs).

[peppy]

gen0-2 counts are gone (which is expected? I think?)

I would have expected these to go away for even the earlier benchmarks, can you explain why they are still there? o_O

[bdach]

Nope, I cannot.

[peppy]

I'm going to drop this into a draft state in the hope of investigating and understanding this for the future. Let's not make rash changes here for now.