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

perf: switch file list to vue-virtual-scroller #40955

Closed
wants to merge 1 commit into from

Conversation

pulsejet
Copy link
Member

This patch significantly improves the performance of the file list by using vue-virtual-scroller over the current simplistic implementation. vue-virtual-scroller has excellent performance since it uses CSS transforms to place the virtual elements and has very good component reuse.

The implementation isn't complete yet but I wanted to get some early feedback / thoughts.

It's somewhat hard to capture the improvement in a video, so here's a before and after with a 4x simulated slowdown:

Before:
https://github.com/nextcloud/server/assets/10709794/0d769798-3d85-44ea-a2b8-cd3fba0aeb53

After:
https://github.com/nextcloud/server/assets/10709794/1d2a2a83-0180-4a23-833b-12d6f69099fa

@skjnldsv
Copy link
Member

skjnldsv commented Oct 17, 2023

We actually moved away from it purposely, sorry.

EDIT: let me add a bit more context I feel like I should, but not gonna lie, I'm quite tired with all this Files work 🙈
Please note that you're missing a lot some internal discussions as well as meetings during a few of our company weeks/conferences, which is why you could not have known. so I still thank you for taking the time! 🙇

  • You're missing the latest master, which includes the recycle part of the vue-virtual-scroller which gives all the performance boost you're seeing
  • The absolute positioning and transform is NOT a perf gain. It changes the way the rendering is done and creates a whole lot of other issues, especially with accessibility
  • The vue-virtual-scroller does not support table mode or proper semantic , meaning accessibility wise we are losing a lot of features
  • vue-virtual-scroller is not maintained properly and have tons of other issues opened
  • There are other points, but listing all the issues we faced going for it (initially we went for vue-virtual-scroller as well as vue-virtual-scroll-list

There are still some performance improvements to have , that will come later like

  • preview fetching/caching enhancements (I've changed our approach a few weeks ago and I'd like to step back to my previous one, or find a middle ground as it was a bit more powerful)
  • better scroll position calculation
  • -insert your fancy idea here-

@skjnldsv skjnldsv closed this Oct 17, 2023
@skjnldsv skjnldsv deleted the pulsejet/file-list-perf branch October 17, 2023 19:26
@@ -50,9 +50,9 @@
<!-- Icon or preview -->
<span class="files-list__row-icon" @click="execDefaultAction">
<template v-if="source.type === 'folder'">
<FolderOpenIcon v-if="dragover" />
<FolderOpenIcon v-once v-if="dragover" />
Copy link
Member

Choose a reason for hiding this comment

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

v-once is still a good idea! 👍

@pulsejet
Copy link
Member Author

Ah I see, thanks for the explanation.

I agree vue-virtual-scroller has an accessibility and maintenance problem, but I do belive using transforms gives better performance (I did a side-by-side comparison of using of the various ways to do virtual scrolling a while back and this approach has fewer reflows than anything else). Anyway, I agree with the reasoning of staying with an in-house component 👍🏻, but I think there are some lessons to be learned from vue-virtual-scroller.

You're missing the latest master, which includes the recycle part of the vue-virtual-scroller which gives all the performance boost you're seeing

Indeed it does seem better than before, but still not as fluid. Will try to profile this a bit to understand what's happening.

BTW I might throw out a couple of PRs for some smaller optimizations; feel free to knock these down if they make a lot of sense.

@skjnldsv
Copy link
Member

but I think there are some lessons to be learned from vue-virtual-scroller.

💯

BTW I might throw out a couple of PRs for some smaller optimizations

🤗 🙇

@pulsejet
Copy link
Member Author

I do belive using transforms gives better performance

So I did a deeper dive into this and thought I'd document my findings here.

The reason using absolute positioning is faster is subtle. The current virtual list code does maintain the list of keys to be the same. Whenever the user scrolls down one element, the array renderedItems is reordered, so that the first key (which is now unused) moves to the last. As a result the components in the virtual DOM are reused.

Here's the problem: since the array was reordered, Vue needs to actually remove the first physical node and append it at the end and set up all hooks again. So even when the virtual DOM is untouched, the actual DOM is manipulated as the user scrolls down, which is quite slow.

On the other hand in the case of using absolute positioning, even the actual DOM is untouched and only the CSS changes. Even better when using CSS transforms, since it all happens on the GPU.

As a POC, I tried switching the list to use transforms and I can confirm that it gives the 60fps butter smooth scrolling effect. But the problem is, still, there's no easy way I've found to make this accessible. It might be possible to make the tab order work correctly using some JS, not sure how this'll fare with screenreaders though.

Let me know if any thoughts @skjnldsv

@pulsejet
Copy link
Member Author

To be clear: I'm not suggesting we do this / it's a good idea. Just satisfying my academic curiosity.

@skjnldsv
Copy link
Member

As a POC, I tried switching the list to use transforms and I can confirm that it gives the 60fps butter smooth scrolling effect. But the problem is, still, there's no easy way I've found to make this accessible. It might be possible to make the tab order work correctly using some JS, not sure how this'll fare with screenreaders though.

Considering the points above and the insane amount of accessibility issues we had to fix over the last months, I'm not taking any risks, especially for a subtle perf change :)

But thanks for the research, this makes sense and is indeed quite insightful! 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants