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

Slow loading state of file list #7038

Closed
4 tasks
kulmann opened this issue May 23, 2022 · 13 comments
Closed
4 tasks

Slow loading state of file list #7038

kulmann opened this issue May 23, 2022 · 13 comments
Assignees
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Topic:Performance Type:Bug Something isn't working

Comments

@kulmann
Copy link
Member

kulmann commented May 23, 2022

Steps to reproduce

  1. Navigate into a folder with 1000 files

Expected behaviour

When the network request (propfind) is done the file list is visible and usable in a timely manner.

Actual behaviour

The file list takes ages to render and even a little bit longer until the first interaction is possible (e.g. context menu).

Action items / hints

  • check if loading previews blocks the UI (especially on scroll)
  • analyse what exactly takes so long after the initial propfind is done. we thought that the lazy loading of cells would improve render speed a lot, but it did not
  • There seem to be a lot of events, causing (partial) re-renders (-> have a look at events in vue dev tools)
  • Collecting all the different tr/td attributes in OcTable is apparently not cached, this might be a huge issue
@kulmann kulmann added Type:Bug Something isn't working Priority:p1-urgent Consider a hotfix release with only that fix labels May 23, 2022
@kulmann
Copy link
Member Author

kulmann commented May 24, 2022

Probably best to solve this in a narrow scope: owncloud/owncloud-design-system#1867

@pascalwengerter
Copy link
Contributor

pascalwengerter commented May 26, 2022

I've done some digging and also opened #7056 (which doesn't even seem to work now, apparently caching made it seem like it worked the other day), pretty convinced that while the OcTable/ResourceTable rendering adjustments do have some positive impact the real culprint seems to be the loadIndicators logic. From how I understood it, we

  • load resources (network request)
  • save resources to store (triggers Vue rendering)
  • purge parts of the store related to share status of resources (triggers Vue rendering)
  • load indicators (network request)
  • save indicators to store by running a for-loop over all available resource (triggers Vue rendering)

There's a comment hinting at room for performance improvements by changing that logic, but there's a lower-hanging fruit from what I've seen:

We don't seem to run into the continue condition when checking whether we need to actually modify the store per resource by saving indicators to it - this leads to an insane amount of store commits that ultimately don't actually change the resource, see screenshot below. Every thin line is another couple of milliseconds, adding up rather quickly due to all the implications of modifying the store

Screenshot 2022-05-26 at 12 33 35

Ideally for now, we can reduce the continue-logic to if (!indicators.length) { continue } to not modify the store if there is no shareIndicators to be added/loaded

We could also discuss if we can request the share-status with the initial resource request from the backend maybe? That'd be the most performant solution I can imagine

@kulmann
Copy link
Member Author

kulmann commented May 26, 2022

classic ui does it the right way already: using the "shareTypes" field from the propfind (= direct shares) and then doing an ocs shares request for each parent folder up to the root (= indirect shares). We should adopt the same.

@kulmann
Copy link
Member Author

kulmann commented May 26, 2022

Apart from that I'm still convinced that the rendering itself is slow. See the ODS PR for lazy loaded icons documentation page using the OcTable without any reactivity in the data. 😅

@pascalwengerter
Copy link
Contributor

Apart from that I'm still convinced that the rendering itself is slow. See the ODS PR for lazy loaded icons documentation page using the OcTable without any reactivity in the data. 😅

Played around by replacing the table subcomponents with (mostly) canonical HTML tags and while it looses the shimmering lazy-loading, rendering then happens _ultra_fast. Might also help that I removed the 6*2275 onclick-listeners on the OcIcon instances 😄

@kulmann
Copy link
Member Author

kulmann commented May 30, 2022

Apart from that I'm still convinced that the rendering itself is slow. See the ODS PR for lazy loaded icons documentation page using the OcTable without any reactivity in the data. 😅

Played around by replacing the table subcomponents with (mostly) canonical HTML tags and while it looses the shimmering lazy-loading, rendering then happens _ultra_fast. Might also help that I removed the 6*2275 onclick-listeners on the OcIcon instances 😄

image

@pascalwengerter
Copy link
Contributor

Apart from that I'm still convinced that the rendering itself is slow. See the ODS PR for lazy loaded icons documentation page using the OcTable without any reactivity in the data. 😅

Played around by replacing the table subcomponents with (mostly) canonical HTML tags and while it looses the shimmering lazy-loading, rendering then happens _ultra_fast. Might also help that I removed the 6*2275 onclick-listeners on the OcIcon instances 😄

image

owncloud/owncloud-design-system@6143188 not part of master since it's a breaking change

@tbsbdr
Copy link
Contributor

tbsbdr commented Jun 17, 2022

@pascalwengerter sounds like promising research ✨ Where is this currently on your prio list? I would be happy to provide a fast fileslisting for users as it affects the experience all the time.

CC @d7oc reading the thread above doesn't sound like what we discussed but maybe I'm wrong.

@tbsbdr
Copy link
Contributor

tbsbdr commented Jun 27, 2022

Where is this currently on your prio list?

ping @pascalwengerter

@tbsbdr
Copy link
Contributor

tbsbdr commented Jul 22, 2022

wohaa, really good progress again!

  • for 1.000 (and even 2.000 files) scrolling feels already fast enough. would still need some testing, buts fast enough on my machine I would say. awesome!
  • the time after the login screen until I see the first file (with 1.000 items-pagination) takes ~5.6 seconds on my side which feels already to long.

... just sharing my experience

Screenshot 000049@2x

Screenshot.000048.mp4

@tbsbdr
Copy link
Contributor

tbsbdr commented Sep 6, 2022

as remedy until we do the vue3 update: lets remove the pagination options:

  • 1000
  • all

@JammingBen
Copy link
Collaborator

as remedy until we do the vue3 update: lets remove the pagination options:

  • 1000
  • all

#7597

How do further progress with this ticket? While this is still "an open thing", I think we're good in regards to GA.

@kulmann
Copy link
Member Author

kulmann commented Sep 7, 2022

@JammingBen can be closed when #7597 is merged. The vue3 update is its own issue and after that's done we can create a new ticket for performance improvements as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Topic:Performance Type:Bug Something isn't working
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants