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.

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

Fix performance overhead from ternary state bindable callbacks when selection is changing #28387

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 4, 2024

RFC.

Closes #28369.

The reporter of the issue was incorrect; it's not the beat snap grid that is causing the problem, it's something far stupider than that.

When the current selection changes, EditorSelectionHandler.UpdateTernaryStates() is supposed to update the state of ternary bindables to reflect the reality of the current selection. This in turn will fire bindable change callbacks for said ternary toggles, which heavily use EditorBeatmap.PerformOnSelection().

The thing about that method is that it will attempt to check whether any changes were actually made to avoid producing empty undo states, but to do this, it must serialise out the entire beatmap to a stream and then binary equality check that to determine whether any changes were actually made:

WriteCurrentStateToStream(stream);
byte[] newState = stream.ToArray();
// if the previous state is binary equal we don't need to push a new one, unless this is the initial state.
if (savedStates.Count > 0 && newState.SequenceEqual(savedStates[currentState])) return;

As goes without saying, this is very expensive and unnecessary, which leads to stuff like keeping a selection box active while a taiko beatmap is playing under it dog slow. So to attempt to mitigate that, add precondition checks to every single ternary callback of this sort to avoid this serialisation overhead.

And yes, those precondition checks use linq, and that is still faster than not having them.

before after
1717489170 1717489431

bdach added 2 commits June 4, 2024 10:32
…election is changing

Closes ppy#28369.

The reporter of the issue was incorrect; it's not the beat snap grid
that is causing the problem, it's something far stupider than that.

When the current selection changes,
`EditorSelectionHandler.UpdateTernaryStates()` is supposed to update the
state of ternary bindables to reflect the reality of the current
selection. This in turn will fire bindable change callbacks for said
ternary toggles, which heavily use `EditorBeatmap.PerformOnSelection()`.

The thing about that method is that it will attempt to check whether any
changes were actually made to avoid producing empty undo states, *but*
to do this, it must *serialise out the entire beatmap to a stream* and
then *binary equality check that* to determine whether any changes were
actually made:

	https://github.com/ppy/osu/blob/7b14c77e43e4ee96775a9fcb6843324170fa70bb/osu.Game/Screens/Edit/EditorChangeHandler.cs#L65-L69

As goes without saying, this is very expensive and unnecessary, which
leads to stuff like keeping a selection box active while a taiko beatmap
is playing under it dog slow. So to attempt to mitigate that, add
precondition checks to every single ternary callback of this sort to
avoid this serialisation overhead.

And yes, those precondition checks use linq, and that is *still* faster
than not having them.
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Works for me.

@peppy peppy merged commit 9f6ff48 into ppy:master Jun 4, 2024
13 of 17 checks passed
@bdach bdach deleted the taiko-selection-handler-is-slow branch June 4, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lag when beat snap is shown on the taiko editor
3 participants