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

Improve EditorTable performance #27958

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

EVAST9919
Copy link
Contributor

The issue occurs while we are hovering the mouse over the table, generating input sub-tree with thousands of items that will be never used for input handling (with all the overhead that comes with it).

Map for testing: https://osu.ppy.sh/beatmapsets/1238185#mania/2574372

master pr
osu_2024-04-21_17-42-22 osu_2024-04-21_17-43-25

@@ -30,6 +30,10 @@ public abstract partial class EditorTable : TableContainer

protected readonly FillFlowContainer<RowBackground> BackgroundFlow;

// We can avoid potentially thousands of objects being added to the input sub-tree since item selection is being handled by the BackgroundFlow
// and no items in the underlying table are clickable.
protected override bool ShouldBeConsideredForInput(Drawable child) => base.ShouldBeConsideredForInput(child) && child == BackgroundFlow;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit:

Suggested change
protected override bool ShouldBeConsideredForInput(Drawable child) => base.ShouldBeConsideredForInput(child) && child == BackgroundFlow;
protected override bool ShouldBeConsideredForInput(Drawable child) => child == BackgroundFlow && base.ShouldBeConsideredForInput(child);

smoogipoo
smoogipoo previously approved these changes Apr 22, 2024
@smoogipoo
Copy link
Contributor

smoogipoo commented Apr 22, 2024

That map is nowhere near as bad for me as your screenshots indicate. ~20ms -> ~8ms. It's a worthwhile improvement though I worry about the locality of these optimisations preventing similar issues from being tackled directly in o!f in the future. This is exactly the sort of situation which I'd expect IsMaskedAway to handle.

I'm curious why your screenshots are showing 3000ms per frame?

@EVAST9919
Copy link
Contributor Author

EVAST9919 commented Apr 22, 2024

This is exactly the sort of situation which I'd expect IsMaskedAway to handle.

IsMaskedAway isn't doing much since the content is a grid container, which doesn't have at least a parent container per row, so no optimisations can be performed. By that I mean we are trying to iterate the whole structure and checking whether a cell is masked away and that's per mouse move.

I'm curious why your screenshots are showing 3000ms per frame?

https://streamable.com/75hi6b
Because this is what literally happens on mouse move

@smoogipoo
Copy link
Contributor

which doesn't have at least a parent container per row

It's one container per cell, right? And 95+% of those cells are off-screen. But more so than that IsMaskedAway doesn't care about that because it's at a Drawable level.

Because this is what literally happens on mouse move

Are you using a 1000Hz poll rate mouse?


I'm going to tentatively merge this, but be aware that this is one of the simplest cases I would hope o!f can easily optimise for. If not, it would affect things like comments container, beatmap listing overlay, markdown/wiki, etc.

@EVAST9919
Copy link
Contributor Author

Are you using a 1000Hz poll rate mouse?

I'm not sure, but seems like it, because when window isn't focused and input thread is limited to 60fps - things are much better.

@smoogipoo smoogipoo merged commit a91a7b9 into ppy:master Apr 22, 2024
7 of 11 checks passed
@EVAST9919 EVAST9919 deleted the editor-table-simple branch April 22, 2024 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants