[Bug]: ListView performance & a11y

[GeopJr]

This is a sibling of #410

During the cleanup PR #424, we replaced the ListBox with ListView. This was suggested a while ago to solve some of the performance issues #179.

How it was implemented

ListView items can either be generated and bound using UI files or with signal factories. Since many parts of the status widgets are dynamically constructed, I went with signal factories.

There are 4 stages and signals: setup -> bind -> unbind -> teardown
The docs explain them in detail https://valadoc.org/gtk4/Gtk.SignalListItemFactory.html.

This might be where I screwed up, by creating the widget on bind rather than going through the process of setting it up, binding it and unbinding it when needed. However after some more testing and actually implementing it properly, I noticed the exact same issues listed below.

The pros

• Improved the overall performance of the app, like animations, resizing etc
• Made scrolling to a certain post easy
• Got rid of the entity_queue as the scroll position wont change when new items get prepended

The cons

• Main loop blocking when creating status widgets #410 is still very much an issue. When you load a new page / batch it still blocks the main loop. I've been unable to pinpoint it down to anything with Sysprof and other debugging tools. My only speculation is that the widget is too complex and makes use of both dynamically constructing widgets and UI files
• The performance is terrible once it hits the ListView threshold of 200(?) objects. At the point every single scroll change causes many destructions and constructions of widgets
• Status widgets are used a lot as a base for other widgets and since we no longer have access to the widgets directly, we need to abuse the entities (that are cached). For example, thread threadlines need to be in the API.Statuses now
• For unknown reasons, listview items are no longer keyboard navigatable?

Screencast.from.2023-09-12.16-24-54.webm

[GeopJr]

Hey @nekohayo & @bugaevc, whenever you get some free time, could you give me a hand on this?

There's info on sysprofing the status creation widgets on #410.
On listviews (not in 0.4.1 release), you can hit the 200 items limit by searching and opening this post that has over 200 replies https://godforsaken.website/@Shrigglepuss/109610034475247929

All my attempts at solving either have led to nowhere. Sysprofing the status widget construction would point me to non-expensive functions - that even after removing wouldn't solve it. As for listviews, splitting the construction of items to setup -> bind -> unbind -> teardown led to, surprisingly, even worse performance. I'm pretty sure a big portion of the listview performance issues are caused by the other issue since it creates and destroys widgets constantly.

As for keyboard accessibility, I have no idea, everything should be focusable. Nothing really changed in that area between listbox -> listview.

Either way, the results are disappointing and I might have to revert back to listboxes for now :/

[bugaevc]

So I'm not the only one seeing performance issues and it's being worked on? That's great!

I don't think I have the necessary skills to make sense of sysprof (it was only today that I attempted doing that last, still to no avail), but I'll see what I can do.

Creating widgets in bind() certainly sounds wrong; the whole point is that you don't do that.

[GeopJr]

So I'm not the only one seeing performance issues and it's being worked on?

They've been around for a long time unfortunately... On 0.4.1 / the current release that uses ListBoxes, it's very noticeable when loading posts:

Screencast.from.2023-09-14.23-15-44.webm

(notice the scrollbar update its position after a delay)

I don't think I have the necessary skills to make sense of sysprof (it was only today that I attempted doing that last, still to no avail), but I'll see what I can do.

I know Jeff (nekohayo) is skilled at it so I'll wait for his findings! If you notice anything that sticks out on Widgets.Status, let me know!

Creating widgets in bind() certainly sounds wrong; the whole point is that you don't do that.

You are right, I don't think we are ready for ListView until the other issue is fixed. The combination of both setup + bind is too performance intense with it. I'll revert back to ListBox until further notice 😞

[nekohayo]

So this is a branch I would have to build myself (how, with gnome builder I suppose) and then set up / authorize for accounts etc. and test (presuming you can solve the bind issue), or there would be some flatpak I could use for this?

I'm tempted to say that in any case, it might be worth waiting a week or so for the Fedora 39 beta that I could try upgrading to, in order to obtain the much awaited Sysprof 45 that would again give us better data (and nicer presentation)... but sysprof boils down to:

1. install all debuginfo packages for this and all the dependencies
2. press the record button
3. reproduce the slow thing
4. press the stop button!

[bugaevc]

I'm tempted to say that in any case, it might be worth waiting a week or so for the Fedora 39 beta that I could try upgrading to, in order to obtain the much awaited Sysprof 45 that would again give us better data (and nicer presentation)

Unfortunately, Sysprof is F39 (and in Rawhide) is still Sysprof 44, at least as of yesterday morning. What I did was profiled the programs with sysprof-cli, and then opened it in Sysprof 45 installed in a Flatpak (from GNOME nightly).

but sysprof boils down to:

1. install all debuginfo packages for this and all the dependencies
2. press the record button
3. reproduce the slow thing
4. press the stop button!

Yes, that much I understand. The issue is what you then to with that data, how do you find what's causing dropped frames using this:

That being said, if it's really CPU intensive, the "time profiler" tab does display results that are similar to other profiling tools & that I can make sense of.

[GeopJr]

So this is a branch I would have to build myself (how, with gnome builder I suppose) and then set up / authorize for accounts etc. and test (presuming you can solve the bind issue), or there would be some flatpak I could use for this?

You can test against the current release since it occurs there as well but if you need the nightlies, you can get them from here: https://nightly.link/GeopJr/Tuba/workflows/build/main/dev.geopjr.Tuba.Devel-x86_64.zip

I'm tempted to say that in any case, it might be worth waiting a week or so for the Fedora 39 beta that I could try upgrading to, in order to obtain the much awaited Sysprof 45 that would again give us better data

Sounds good to me, thanks!

Yes, that much I understand. The issue is what you then to with that data, how do you find what's causing dropped frames using this

These two blog posts might be helpful:

• https://blogs.gnome.org/chergert/2020/03/14/how-to-use-sysprof-to/
• https://blogs.gnome.org/chergert/2020/03/15/how-to-use-sysprof-to-part-ii/

Specifically, since the problem looks like main loop blocking, the "Finding main loop slow downs" and "Avoiding Main Loop Stalls" sections

[bugaevc]

So from looking at the code, I see that there's the Widgetizable interface (for something that can be made into a widget), and that ContentBase has no idea which kinds of items it is putting into the list view — it's just something widgetizable. With that design, it makes sense that you have to do everything in bind().

I'm not sure if this is the source of all the performance issues (that's actually unlikely), but it definitely shouldn't be done like that. A list view should be used with a specific kind of widget, that widget should be created in setup() and only filled with data in bind(). And actually a builder factory would suit us better than the signal factory.

[bugaevc]

It's spending a lot of time in... CSS invalidation?

[GeopJr]

So from looking at the code, I see that there's the Widgetizable interface (for something that can be made into a widget), and that ContentBase has no idea which kinds of items it is putting into the list view — it's just something widgetizable. With that design, it makes sense that you have to do everything in bind().

It's possible to extend Widgetizable to include setup, bind, unbind,

e.g. for status (incomplete):

// Widgets.Status
public Status.empty () {
  Object ();
}

public void unbind () {
// reset everything that happened in bind ()
}

// Widgetizable
public virtual void unbind (Gtk.Widget widget) {}
public virtual void bind (Gtk.Widget widget) {}

// API.Status
public override void unbind (Gtk.Widget widget) {
  ((Widgets.Status) widget).unbind ();
}

public override void bind (Gtk.Widget widget) {
  ((Widgets.Status) widget).bind = this;
}

public override Gtk.Widget to_widget () {
  return new Widgets.Status.empty ();
}

// Views.ContentBase
...

But it is a bit tedious to do for every Widgetizable 🤔

And actually a builder factory would suit us better than the signal factory.

I have only three concerns:

1. Most widgets use Widgets.Status as a base and either extend it (Follow requests add approve/decline buttons) or leave most of its children unused (Accounts will never use the spoiler stack, spoiler button, booster avatar etc). While there have been attempts at dynamically creating some children of the Status widget (like polls, attachments, reactions, quotes, preview cards... only get constructed when they exist in the Entity) for performance reasons ([Feature request]: Status widget needs to be refactored #57), using a builder factory (from my understanding) would require us to either include everything from every subclass of the Status widget (follow buttons, polls, quotes, actor avatar...) which will probably make every Widget that inherits from Status to be way too expensive or stop inheriting from the Status widget which would lead to a lot of duplicate/similar Builder files that have to always follow the Status' one (Announcements do that already)
2. Actions. The status' actions are both dynamically appended ('Edit' is only added if you are the author of the post and 'View Stats' only if the post has > 0 reblogs or favs) and depend on the Status entity itself ('Open in Browser' uses the bound entity's url, 'Edit' uses the entity itself, 'View Stats' uses the entity id). I don't think it's possible to do using Builder factories :/
3. Expanded posts (/ when you activate a post and it opens in a thread), move some widgets around, adds css classes, changes some labels, adds some others etc. Also don't think it's possible with Builder factories, unless expanded statuses also become their own different widgets

(I might be wrong on the above, I'm going off of the assumption that we won't have access to the widgets built using the Builder factory)

It's spending a lot of time in... CSS invalidation?

🤔 FWIW, removing our custom styles does not seem to have any impact on performance

[bugaevc]

It's possible to extend Widgetizable to include setup, bind, unbind,

It could be possible to go that route, yeah. But my point is that we should try to move away from that abstraction and from that way of structuring the logic altogether. The view that includes the list should know which specific kind of items it contains (not some generic widgetazables, but say, statuses), and only support those, and work with those directly, instead of trying to be generic over it.

At least, in my understanding, that's how the lists framework is supposed to used in gtk4. Maybe that was different with a list box.

Most widgets use Widgets.Status as a base and either extend it (Follow requests add approve/decline buttons) or leave most of its children unused (Accounts will never use the spoiler stack, spoiler button, booster avatar etc).

Well yes, that needs to be thought over. I don't know whether it's better/faster to create and destroy auxiliary subwidgets (like polls and quotes) dynamically each time you're re-binding a Status widget to a status, or to create them upfront and hide them when they're not needed.

which will probably make every Widget that inherits from Status to be way too expensive or stop inheriting from the Status widget which would lead to a lot of duplicate/similar Builder files that have to always follow the Status' one (Announcements do that already)

I currently don't have any follow requests so I can't even check, but it sounds wrong for a follow request to inherit from status? Like it's a different thing entirely?

Actions. The status' actions are both dynamically appended ('Edit' is only added if you are the author of the post and 'View Stats' only if the post has > 0 reblogs or favs) and depend on the Status entity itself ('Open in Browser' uses the bound entity's url, 'Edit' uses the entity itself, 'View Stats' uses the entity id). I don't think it's possible to do using Builder factories :/

Why not? To hide an action, we should just use hidden-when: action-disabled, no need to dynamically append.

But otherwise, the builder factory does not limit us in binding a status entity to the widget? You'd do this (Blueprint syntax for readability):

template ListItem {
  child: Tuba.WidgetsStatus {
    status: bind template.item as <$Tuba.APIStatus>;
  };
}

Expanded posts (/ when you activate a post and it opens in a thread), move some widgets around, adds css classes, changes some labels, adds some others etc. Also don't think it's possible with Builder factories, unless expanded statuses also become their own different widgets

Again, I don't see why not? (Perhaps there is a reason and I'm just not understanding it, you surely have way more experience with the codebase and know what the tradeoffs are and where the gotchas lie.)