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

Add virtualised list container #6312

Merged
merged 7 commits into from
Jun 26, 2024
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 24, 2024

RFC.

Game-side, we have a few lists/tables which perform dreadfully when stressed by a large number of rows/columns. Two of them reside in the editor, namely: the timing table and the verify issue list table.

The primary overheads in both are:

  • All of the rows' content is constructed, loaded, and placed in the hierarchy eagerly, which causes a long freeze on first show.
  • Even though 99.999999% of the content is not visible, it is still present in various places where it is causing trouble. While masking does cull some of it, the big killer is input handling - if any ancestor of a table row might handle e.g. mouse input because they are all alive, all of them get shoved into the input queue and then all need to be checked on every mouse movement. (See Improve input handling performance of TableContainer #6282.) Mitigation of this has been applied via dumb hacks like having the background of the table be a separate drawable under it and have it handle input. I was sorta hoping ShouldBeConsideredForInput could maybe fix this but it regretfully does not (it reduces some overhead but not enough to be acceptable yet).
  • For mutable lists, 99% of usage sites will not handle the args carefully, but rather just clear everything and reconstruct from scratch, which is obviously rather expensive for long lists.

What this pull is intended to introduce is a sort of UITableView but for osu!framework usage out-of-the-box.

  • Exposes a "virtualised list" data-oriented interface. You derive from VirtualisedListContainer, and give a model and a drawable for it via the generic specs.
  • The list item must be poolable and must have a bindable model on it (can't use ModelBackedDrawable because I can't inherit from both PoolableDrawable and that, so IHasCurrentValue it is)
  • List items are assumed to have fixed heights so that layout can be ballparked in advance without items being physically present in the hierarchy.

For testing there is the test scene added here, but there's also this branch (needs to be cleaned up yet, very PoC). In my vague "performance testing" (read: looking at frame graph times), on a stress test like https://osu.ppy.sh/beatmapsets/1238185#mania/2574372 this can improve FPS by factors of 5 to even an order of magnitude:

  • timing - 170fps -> 1000fps
  • verify - 13fps -> 400fps

(both tested on release configuration, with static mouse).

The pooling takes care of the drawable construction overheads, and the lifetime games this container plays help with input handling (if a child isn't alive, it doesn't get looked at with respect to input at all). Collection changes are handled somewhat efficiently (better than reconstructing everything from scratch, at least).

The one case that is still not as fast as I perhaps would have hoped for is active scrolling, but it's still better than it was.

@peppy peppy self-requested a review June 26, 2024 04:38

for (int i = 0; i < e.NewItems!.Count; ++i)
{
Items.Insert(Math.Max(e.NewStartingIndex, 0) + i, new ItemRow((TData)e.NewItems![i]!, pool)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I guess this is the best we can do without a lot more work (potentially avoiding using a FillFlowContainer altogether?), but we're still living with one (very lightweight) drawable per row here. Based on your profiling results it seems like this is still going to be enough to help for the cases we're looking at fixing, but something worth noting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I was aware that there was room for improvement still technically if we wanted to, but decided in the end that the tradeoff in implementation complexity did not seem worth it at this time.

@peppy peppy merged commit 8ab3816 into ppy:master Jun 26, 2024
21 checks passed
@bdach bdach deleted the virtualised-list-container branch June 26, 2024 07:19
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.

None yet

2 participants