Opened 6 years ago

Closed 4 years ago

#14675 closed bug (fixed)

[HaikuDepot] switching between Featured and All packages tabs is slow

Reported by: diver Owned by: waddlesplash
Priority: normal Milestone: R1/beta2
Component: Applications/HaikuDepot Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Platform: All

Description

hrev52462.

Just updated Haiku to see new tabbed HaikuDepot and noticed its being slow at switching between the tabs.

Change History (24)

comment:1 by waddlesplash, 6 years ago

Owner: changed from stippi to apl-haiku
Status: newassigned

comment:2 by diver, 6 years ago

Owner: changed from apl-haiku to waddlesplash

This was introduced in hrev52449 (e00a489b80a7219bf80658d157548eb0aa4a97d0) Reverting this commit fixes extensive slowdown in switching between Featured view and all packages.

comment:3 by diver, 6 years ago

BTW, now that I switched to the old look I find it much better than tabbed view.

comment:4 by waddlesplash, 6 years ago

Owner: changed from waddlesplash to apl-haiku

That change should not have been any more or less performant than previously; it just changed the layout from a BCardView to a BTabView. The problem is not in code that I wrote at all.

BTW, now that I switched to the old look I find it much better than tabbed view.

The reason we switched to the tab view was that people were ignoring the "Show only featured packages" checkbox because they didn't understand it / didn't even see it. Having the tabs makes the dichotomy much more obvious. (I prefer it in general, and I think humdinger does too.)

in reply to:  4 comment:5 by humdinger, 6 years ago

Replying to waddlesplash:

(I prefer [tabs] in general, and I think humdinger does too.)

I think the tabs do look better as they are not as "in-your-face" as the checkbox that used to be in the middle. OTOH, people were able to miss the "in-your-face" checkbox, so I expect the tabs to fair even less well in that regard...
The tabs have the additional advantage to be extendable with more tabs, if we see a need. (Maybe a dynamically named "Pending/Downloading/Installed" for the changed packages in the curent session.)

comment:6 by diver, 5 years ago

Milestone: UnscheduledR1/beta2

I'd like to nominate this bug for beta2 milestone.

HaikuDepot, which once was blazing fast, became really sluggish nowadays to the point I stopped using it completely.

However, we don't want to make bad impression on regular users and newcomers of our package management system. #11674 is likely another candidate.

comment:7 by apl-haiku, 5 years ago

I have investigated this. It seems that there were a couple of areas that could be improved and I was able to get some code clean-up done, but the majority of the computational time taken is in the re-layout of the views of the "prominent" and "all" package lists.

Still to investigate; this has the potential to be a contributing factor to the slow initial loading performance.

Most likely the best (first) path forward would be to implement the FeaturedPackagesView as a single monolithic view rather than as a composite of dynamically laid-out views in order to avoid or simplify the computational cost of the layout-process. This is probably a medium-sized job and low time-risk.

The PackageListView could also be re-written in a more performant way as well, but would be more difficult owing to the inheritance from the more complex BColumnListView. Some compromises may be able to be made in order to reduce the complexity of this task; does it need to be a table?

I have some related improvements in a local branch which I can only put into a PR after PR 2467 is merged. These related improvements make a minor contribution to the performance, but do clean up the code quite a bit around the tab-switching. These changes are "B2 safe".

I suggest that any further and deeper work on this proceeds after B2 is released because it is likely to be disruptive, needs to be well considered and will take time to settle.

comment:8 by waddlesplash, 5 years ago

I have investigated this. It seems that there were a couple of areas that could be improved and I was able to get some code clean-up done, but the majority of the computational time taken is in the re-layout of the views of the "prominent" and "all" package lists.

I wonder why switching to a tab view made this so much slower vs. the card view, then. I guess the tab view invalidates the child's layout on each tab switch; can we avoid that? That will help restore performance.

comment:9 by apl-haiku, 5 years ago

It seems that (by manual sampling) most of the time seems to be consumed here;

...
BFont::GetStringWidths(..)
BFont::StringWidth(..)
BTitledColumn::GetPreferredWidth(BField* .., BView* ..)
PackageColumn::GetPreferredWidth(BField* .., BView* ..)
BPrivate::OutlineView::GetColumnPreferredWidth(BColumn* ..)
BColumnListView::PreferredSize()
BViewLayoutItem::PreferredSize()
...

in reply to:  9 comment:10 by X512, 5 years ago

Replying to apl-haiku:

BTitledColumn::GetPreferredWidth(BField* .., BView* ..)
PackageColumn::GetPreferredWidth(BField* .., BView* ..)

It is likely full list traverse is done at each insertion making O(n^2) time complexity.

Last edited 5 years ago by X512 (previous) (diff)

comment:11 by CodeforEvolution, 5 years ago

If StringWidth() is being used a lot, maybe WidthBuffer could be used: https://github.com/haiku/haiku/blob/master/src/kits/interface/textview_support/WidthBuffer.cpp

WidthBuffer caches StringWidth() value calculations and avoids extra app_server trips.

comment:12 by waddlesplash, 5 years ago

The better solution is to not recompute the entire layout every time an item is added, and also not to recompute the layout upon switching tabs.

comment:13 by apl-haiku, 5 years ago

See the comment trail here. I am going to try another approach as suggested there. Short term, I am going to switch to re-factor FeaturedPackagesView first. I have started that process.

comment:14 by waddlesplash, 5 years ago

Why not just fix ColumnListView to not recompute the preferred column size at every insertion?

comment:15 by apl-haiku, 5 years ago

Yes I will get back to that; both views have performance problems. I was looking at #15889 which lead me back here on the FeaturedPackagesView class.

comment:16 by apl-haiku, 5 years ago

I am re-coding FeaturedPackagesView using manual drawing and it is now responding quickly; work progressing on that. This is quite a big change and so if somebody else feels like taking on the work of...

...fix ColumnListView to not recompute the preferred column size at every insertion?

As per comments in here -- then that could happen concurrently with what I am doing.

comment:17 by waddlesplash, 5 years ago

Is that really the best solution? It would seem like making BStringView more performant (possibly by doing caching like BTextView does) would be the better solution?

comment:18 by apl-haiku, 5 years ago

FeaturedPackagesView -- I think it is the best solution. The auto-layout is great for layout of a window of controls and views and is performant enough, but auto laying out what is essentially a fixed layout thousands of times in a list isn't really making much sense.

comment:19 by waddlesplash, 4 years ago

Owner: changed from apl-haiku to waddlesplash
Status: assignedin-progress

So, as it turns out, I was indeed correct that using draw-calls is not at all the best solution, and the problem was indeed in BTabView (or actually BCardView, which is what BTabView uses in layout mode): https://review.haiku-os.org/c/haiku/+/2677/2

Switching tabs is basically instantaneous after this, and resizing the window is still very fast. So the draw-calls, and also the fixes to ColumnListView, are not needed, indeed.

comment:20 by waddlesplash, 4 years ago

Loading packages is still not very fast due to how often the layout is invalidated, but that is a separate ticket. I think there is a way to "batch" invalidations, which is what should be done there.

comment:21 by apl-haiku, 4 years ago

Yes I agree, but I think all three things would be good to get done here and would make a big difference.

  • Change from dynamic to hand-coded layout in FeaturedPackagesView
  • Stop tabs-changes from doing relayout
  • Stop BColumnListView from readjusting the column widths

I'm working on FeaturedPackagesView, but am moving house (it's complex) and so time is scant, but if you can help with the others things then that would be terrific.

comment:22 by waddlesplash, 4 years ago

Change from dynamic to hand-coded layout in FeaturedPackagesView

I still maintain this is a bad idea, in general, and is just a hack to get around underlying performance problems that we should solve.

Stop tabs-changes from doing relayout

Yes, that is what my change above does, and it fixes this ticket completely.

Stop BColumnListView from readjusting the column widths

This was just happening due to BCardLayout re-layouts recomputing PreferredSize(), which is what recomputes preferred column sizes. The actual column widths were not changing. We may still want to change BColumnListView here, but at least this is no longer a pressing problem.

comment:23 by apl-haiku, 4 years ago

Obviously this ticket will be fixed by your change.

I still maintain this is a bad idea, in general, and is just a hack to get around underlying performance problems that we should solve.

In the wider sense, considering other tickets as well -- each View representing a single package undertakes layout independently. If there are 1000 packages in that scrolled View then it is performing that layout 1000 times even though the layout is exactly the same in each case. This automated layout approach is very inefficient and the layout end result is largely static. I don't think that using the automated layout system makes sense this situation at all but I would be interested to hear your approach to how the performance problems with FeaturedPackagesView should be resolved while still using the automated layout system.

Yes, that is what my change above does, and it fixes this ticket completely.

It will fix this specific ticket, but there are other tickets where a solution to the performance problems of BColumnListView will continue to be an issue and I suspect the suggestion from X512 will be good to explore further.

comment:24 by waddlesplash, 4 years ago

Resolution: fixed
Status: in-progressclosed

Fixed in hrev54185.

Note: See TracTickets for help on using tickets.