[Bug]: ListView performance & a11y about tuba HOT 14 OPEN

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 :/

from tuba.

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.

from tuba.

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:

(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 😞

from tuba.

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!

from tuba.

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.

from tuba.

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

from tuba.

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.

from tuba.

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

from tuba.

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,

// 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 (#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

from tuba.

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.)

from tuba.

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.

It makes sense but it's also kind-of limiting. For example profiles need to have a cover/header and since we can't have boxes in the scrolledwindow anymore, the cover ended up being a Widgetizable https://github.com/GeopJr/Tuba/blob/main/src/Views/Profile.vala#L2

Notifications timeline can also have different widgets, one for status related events and another for profiles (if we move them to a different widget) (used for "x user followed you" events)

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?

It's a profile / account which inherits from status + 2 new buttons for accept/decline

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?

Sounds good! I need to take a deeper dive into ListViews, but from my (limited) understanding, the factories act like the dropdown ones where you can only bind objects to them / you can't create actions for them, no idea about signals. What I gathered from https://blog.gtk.org/2020/06/07/scalable-lists-in-gtk-4/ and the linked examples, when using factories, the widgets are exclusively handled by the UI files, there won't be a Widgets.Status anymore and for something like expanded widgets to work we'd have to include for example two privacy indicator icons, one at the top right and one at the bottom (for expanded) and conditionally show them based on the object we bind to the factory

from tuba.

For example profiles need to have a cover/header and since we can't have boxes in the scrolledwindow anymore, the cover ended up being a Widgetizable

It should be very possible to put more widgets (such as profile headers or whatever) into the Gtk.ScrolledWindow next to the list and have them scroll together; it would just require some more fiddling with the adjustments. In fact, I might have a prototype of a ScrollableBin widget that enables you to write this (again, Blueprint syntax):

ScrolledWindow {
  Box {
    orientation: vertical;

    $TubaWidgetsCover {
      ...
    }

    ScrollableBin {
      ListView {
        ...
      }
    }
  }
}

...and have it just work. But making it a list view item is also a reasonable approach.

Notifications timeline can also have different widgets, one for status related events and another for profiles

In general, I think this is something that the scalable lists framework should provide explicit support for: having heterogeneous items (and item views instantiated for them), so that it can recycle/reuse views of the appropriate kind.

In Android's RecyclerView, this is called getItemViewType(), whose return value is then passed into onCreateViewHolder() (aka setup () in Gtk.SignalListItemFactory). In UIKit, you register a NIB for cells of a type (indetified by an identifier) with -[UITableView registerNib:forCellReuseIdentifier:], and then later call -[UITableView dequeueReusableCellWithIdentifier:].

So maybe this is something that we should discuss with GTK developers, to get GTK to support the same. Otherwise we'd indeed have either to create and tear down item view on each bind (which goes against how the scalable lists framework is intended to be used), or put a GtkStack into each item widget; neither one seems desirable.

That being said, one optimization we could do without changes to GTK is only recreating the view on bind() if the kind of view differs, and otherwise rebinding the existing view. Assuming most of the items are actually of a single type (Status), this should get us most of the way there.

It's a profile / account which inherits from status + 2 new buttons for accept/decline

I see, thanks.

A design question then: should a profile card look exactly like a status? Is that not confusing?

Sounds good! I need to take a deeper dive into ListViews, but from my (limited) understanding, the factories act like the dropdown ones where you can only bind objects to them / you can't create actions for them, no idea about signals. What I gathered from https://blog.gtk.org/2020/06/07/scalable-lists-in-gtk-4/ and the linked examples, when using factories, the widgets are exclusively handled by the UI files, there won't be a Widgets.Status anymore and for something like expanded widgets to work we'd have to include for example two privacy indicator icons, one at the top right and one at the bottom (for expanded) and conditionally show them based on the object we bind to the factory

Uhh, I don't understand why you'd get that impression? It's just Gtk.Builder, you can put stock widgets like labels there, or your can put your own Tuba.Widgets.Status, or any combination of that. In the end, with either kind of factory, you're just creating a widget tree. The builder factory just makes it more declarative and pushes you towards using GObject property bindings / Gtk.Expression to set everything up when the item is created, compared to the signal factory which kind of expects you to do more at bind () time (though it's still possible to use bindings and expressions in the signal factory, and avoid connecting to bind ()). It's the same trade-off as creating widgets in code vs using UI files, outside of the lists framework.

Anyways, as we've seen above, the real reason for the slowness seems to be CSS (in)validation; we should be looking into what's causing that.

from tuba.

It should be very possible to put more widgets (such as profile headers or whatever) into the Gtk.ScrolledWindow next to the list and have them scroll together; it would just require some more fiddling with the adjustments.

During the migration to ListView, anything that wouldn't match the structure of GtkScrolledWindow -> AdwClampScrollable -> ListView (like boxes in between) would cause either the ListView being cut/cropped at the top and bottom or with the ListView items' size. The structure you mentioned is similar to the one before the migration! Unsure if ScrollableBin takes account of the issues.

A design question then: should a profile card look exactly like a status? Is that not confusing?

I don't blame tootle for having it like that since it matches Mastodon web

but it makes sense to be a different one, similar to the mobile app's, I'll explore it after the next release!

Uhh, I don't understand why you'd get that impression?

Ah, I thought the whole widget had to be included in the builder file for the expressions to work. Thanks for the clarification!

from tuba.

NOTE:

Main has reverted the ListView change (partially) for the upcoming release, further development on the ListView issue has to enable it in Meson and revert some UI file and CSS changes as described on #504 (comment)

from tuba.