Improve performance of UR calculations #30868
Conversation
| public static bool AffectsUnstableRate(HitEvent e) => AffectsUnstableRate(e.HitObject, e.Result); | ||
| public static bool AffectsUnstableRate(HitObject hitObject, HitResult result) => hitObject.HitWindows != HitWindows.Empty && result.IsHit(); | ||
|
|
||
| public class UnstableRateCalculationResult |
There was a problem hiding this comment.
I'm sure people are going to want changes to this, but I'm not sure how strict review will be so I've just left it without any safeties initially.
There was a problem hiding this comment.
I'd rename NextProcessableIndex to EventCount probably (with the necessary off-by-one adjustments) but otherwise I'm not sure I have much of a problem with this...?
There was a problem hiding this comment.
Actually doesn't require off-by-one adjustments since it already kinda is the event count (at least post-loop-execution).
| public static bool AffectsUnstableRate(HitEvent e) => AffectsUnstableRate(e.HitObject, e.Result); | ||
| public static bool AffectsUnstableRate(HitObject hitObject, HitResult result) => hitObject.HitWindows != HitWindows.Empty && result.IsHit(); | ||
|
|
||
| public class UnstableRateCalculationResult |
There was a problem hiding this comment.
I'd rename NextProcessableIndex to EventCount probably (with the necessary off-by-one adjustments) but otherwise I'm not sure I have much of a problem with this...?
originally i wasn't going to PR, so it probably reads a bit that way. the stand-alone benchmark wasn't giving great improvements. i'll take a second look at the benchmark today to see if i'm doing anything obviously wrong. |
|
I think it's probably fine as is. |
| /// The optimisations used here rely on hit events being a consecutive sequence from a single gameplay session. | ||
| /// When a new gameplay session is started, any existing results should be disposed. | ||
| /// </remarks> | ||
| public class UnstableRateCalculationResult |
There was a problem hiding this comment.
My only query is why this isn't a struct with readonly fields. Perhaps a readonly record struct even.
Minor nitpick, though.
There was a problem hiding this comment.
I can try a struct, but as a record the alloc overhead (cpu, not memory) outweighted the benefits, seemingly (see ea68d4b).
But maybe this was just local to the benchmark and not relevant in real-world scenarios.
There was a problem hiding this comment.
If it has a significant overhead, I'd say leave it then.
Closes #30635.
Baseline:
With patch applied (605fe71):
Didn't show as much of an improvement as I expected to see based on the profiler results (seems like profiler is getting the point of usage slightly wrong...)
With list
foriteration (33d725e withforeachswitched tofor):Full PR:
I'm not immediately sure why it's benchmarking slower. Spent my allocated time on this so I'm going just going to smile and nod, but interested if anyone has an idea.
Importantly though, real world performance is much better (over a single autoplay run of this beatmap):
Baseline:
On ea68d4b:
On bbe8f2e (kinda OP because slider ticks and what not will not recalc at all):