Skip to content

files-list: performance optimizations #40958

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.

Sign up for GitHub

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

Merged
skjnldsv merged 2 commits into from
Oct 19, 2023
Merged

files-list: performance optimizations #40958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4876eac
perf(files): performance optimizations
pulsejet Oct 18, 2023
4db0388
fix(files): drop visible and adjust drag-to-scroll feature
skjnldsv Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions apps/files/src/components/FileEntry.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
-->

<template>
<tr :class="{'files-list__row--visible': visible, 'files-list__row--active': isActive, 'files-list__row--dragover': dragover, 'files-list__row--loading': isLoading}"
<tr :class="{'files-list__row--dragover': dragover, 'files-list__row--loading': isLoading}"
data-cy-files-list-row
:data-cy-files-list-row-fileid="fileid"
:data-cy-files-list-row-name="source.basename"
Expand All @@ -37,8 +37,7 @@
<span v-if="source.attributes.failed" class="files-list__row--failed" />

<!-- Checkbox -->
<FileEntryCheckbox v-if="visible"
:display-name="displayName"
<FileEntryCheckbox :display-name="displayName"
:fileid="fileid"
:is-loading="isLoading"
:nodes="nodes" />
Expand Down Expand Up @@ -67,8 +66,7 @@
:files-list-width="filesListWidth"
:loading.sync="loading"
:opened.sync="openedMenu"
:source="source"
:visible="visible" />
:source="source" />

<!-- Size -->
<td v-if="!compact && isSizeAvailable"
Expand All @@ -95,8 +93,7 @@
class="files-list__row-column-custom"
:data-cy-files-list-row-column-custom="column.id"
@click="openDetailsIfAvailable">
<CustomElementRender v-if="visible"
:current-view="currentView"
<CustomElementRender :current-view="currentView"
:render="column.render"
:source="source" />
</td>
Expand Down Expand Up @@ -146,10 +143,6 @@ export default Vue.extend({
},

props: {
visible: {
type: Boolean,
default: false,
},
isMtimeAvailable: {
type: Boolean,
default: false,
Expand Down
22 changes: 9 additions & 13 deletions apps/files/src/components/FileEntry/FileEntryActions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@
-
-->
<template>
<td v-show="visible"
class="files-list__row-actions"
<td class="files-list__row-actions"
data-cy-files-list-row-actions>
<!-- Render actions -->
<CustomElementRender v-for="action in enabledRenderActions"
Expand All @@ -33,10 +32,9 @@
class="files-list__row-action--inline" />

<!-- Menu actions -->
<NcActions v-if="visible"
ref="actionsMenu"
:boundaries-element="getBoundariesElement()"
:container="getBoundariesElement()"
<NcActions ref="actionsMenu"
:boundaries-element="getBoundariesElement"
:container="getBoundariesElement"
:disabled="isLoading || loading !== ''"
:force-name="true"
:force-menu="enabledInlineActions.length === 0 /* forceMenu only if no inline actions */"
Expand Down Expand Up @@ -70,6 +68,7 @@ import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'
import NcIconSvgWrapper from '@nextcloud/vue/dist/Components/NcIconSvgWrapper.js'
import NcLoadingIcon from '@nextcloud/vue/dist/Components/NcLoadingIcon.js'

import CustomElementRender from '../CustomElementRender.vue'
Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023

Choose a reason for hiding this comment

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

missing import

import logger from '../../logger.js'

// The registered actions list
Expand All @@ -83,6 +82,7 @@ export default Vue.extend({
NcActions,
NcIconSvgWrapper,
NcLoadingIcon,
CustomElementRender,
},

props: {
Expand All @@ -102,10 +102,6 @@ export default Vue.extend({
type: Object as PropType<Node>,
required: true,
},
visible: {
type: Boolean,
default: false,
},
gridMode: {
type: Boolean,
default: false,
Expand Down Expand Up @@ -150,7 +146,7 @@ export default Vue.extend({

// Enabled action that are displayed inline with a custom render function
enabledRenderActions() {
if (!this.visible || this.gridMode) {
if (this.gridMode) {
return []
}
return this.enabledActions.filter(action => typeof action.renderInline === 'function')
Expand Down Expand Up @@ -182,9 +178,7 @@ export default Vue.extend({
this.$emit('update:opened', value)
},
},
},

methods: {
/**
* Making this a function in case the files-list
* reference changes in the future. That way we're
Expand All @@ -193,7 +187,9 @@ export default Vue.extend({
getBoundariesElement() {
return document.querySelector('.app-content > table.files-list')
},
},

methods: {
actionDisplayName(action: FileAction) {
if (this.filesListWidth < 768 && action.inline && typeof action.title === 'function') {
// if an inline action is rendered in the menu for
Expand Down
8 changes: 4 additions & 4 deletions apps/files/src/components/FileEntry/FileEntryPreview.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
<template>
<span class="files-list__row-icon">
<template v-if="source.type === 'folder'">
<FolderOpenIcon v-if="dragover" />
<FolderOpenIcon v-once v-if="dragover" />
<template v-else>
<FolderIcon />
<FolderIcon v-once />
<OverlayIcon :is="folderOverlay"
v-if="folderOverlay"
class="files-list__row-icon-overlay" />
Expand All @@ -41,13 +41,13 @@
@error="backgroundFailed = true"
@load="backgroundFailed = false">

<FileIcon v-else />
<FileIcon v-once v-else />

<!-- Favorite icon -->
<span v-if="isFavorite"
class="files-list__row-icon-favorite"
:aria-label="t('files', 'Favorite')">
<FavoriteIcon />
<FavoriteIcon v-once />
Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023

Choose a reason for hiding this comment

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

Icons don't need re-rendering

</span>
</span>
</template>
Expand Down
12 changes: 3 additions & 9 deletions apps/files/src/components/FileEntryGrid.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
-->

<template>
<tr :class="{'files-list__row--visible': visible, 'files-list__row--active': isActive, 'files-list__row--dragover': dragover, 'files-list__row--loading': isLoading}"
<tr :class="{'files-list__row--active': isActive, 'files-list__row--dragover': dragover, 'files-list__row--loading': isLoading}"
data-cy-files-list-row
:data-cy-files-list-row-fileid="fileid"
:data-cy-files-list-row-name="source.basename"
Expand All @@ -37,8 +37,7 @@
<span v-if="source.attributes.failed" class="files-list__row--failed" />

<!-- Checkbox -->
<FileEntryCheckbox v-if="visible"
:display-name="displayName"
<FileEntryCheckbox :display-name="displayName"
:fileid="fileid"
:is-loading="isLoading"
:nodes="nodes" />
Expand Down Expand Up @@ -69,8 +68,7 @@
:grid-mode="true"
:loading.sync="loading"
:opened.sync="openedMenu"
:source="source"
:visible="visible" />
:source="source" />
</tr>
</template>

Expand Down Expand Up @@ -115,10 +113,6 @@ export default Vue.extend({

inheritAttrs: false,
props: {
visible: {
type: Boolean,
default: false,
},
source: {
type: [Folder, NcFile, Node] as PropType<Node>,
required: true,
Expand Down
17 changes: 10 additions & 7 deletions apps/files/src/components/FilesListVirtual.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,18 +259,17 @@ export default Vue.extend({
event.preventDefault()
event.stopPropagation()

// If reaching top, scroll up
const firstVisible = this.$refs.table?.$el?.querySelector('.files-list__row--visible') as HTMLElement
const firstSibling = firstVisible?.previousElementSibling as HTMLElement
if ([firstVisible, firstSibling].some(elmt => elmt?.contains(event.target as Node))) {
const tableTop = this.$refs.table.$el.getBoundingClientRect().top
const tableBottom = tableTop + this.$refs.table.$el.getBoundingClientRect().height

// If reaching top, scroll up. Using 100 because of the floating header
if (event.clientY < tableTop + 100) {
this.$refs.table.$el.scrollTop = this.$refs.table.$el.scrollTop - 25
return
}

// If reaching bottom, scroll down
const lastVisible = [...(this.$refs.table?.$el?.querySelectorAll('.files-list__row--visible') || [])].pop() as HTMLElement
const nextSibling = lastVisible?.nextElementSibling as HTMLElement
if ([lastVisible, nextSibling].some(elmt => elmt?.contains(event.target as Node))) {
if (event.clientY > tableBottom - 50) {
this.$refs.table.$el.scrollTop = this.$refs.table.$el.scrollTop + 25
}
},
Expand Down Expand Up @@ -312,6 +311,8 @@ export default Vue.extend({
&::v-deep {
// Table head, body and footer
tbody {
will-change: scroll-position, padding;
Copy link
Member

@skjnldsv skjnldsv Oct 18, 2023

Choose a reason for hiding this comment

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

Not sure about padding. Is it even a valid value for will-change ? 🤔

Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well will-change takes <custom-ident> as a valid value so it's all up to the browser. I don't think there are any real optimizations for padding changes right now though 🤷🏻

contain: layout paint style;
Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023

Choose a reason for hiding this comment

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

Some CSS tricks to improve rendering performance

display: flex;
flex-direction: column;
width: 100%;
Expand All @@ -320,6 +321,7 @@ export default Vue.extend({

/* Hover effect on tbody lines only */
tr {
contain: strict;
&:hover,
&:focus {
background-color: var(--color-background-dark);
Expand All @@ -329,6 +331,7 @@ export default Vue.extend({

// Before table and thead
.files-list__before {
contain: strict;
display: flex;
flex-direction: column;
}
Expand Down
16 changes: 9 additions & 7 deletions apps/files/src/components/VirtualList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
<component :is="dataComponent"
v-for="({key, item}, i) in renderedItems"
:key="key"
:visible="(i >= bufferItems - 1 || index <= bufferItems) && (i <= shownItems - bufferItems)"
:source="item"
:index="i"
v-bind="extraProps" />
Expand Down Expand Up @@ -211,7 +210,7 @@ export default Vue.extend({
}

// Adding scroll listener AFTER the initial scroll to index
this.$el.addEventListener('scroll', this.onScroll)
this.$el.addEventListener('scroll', this.onScroll, { passive: true })
Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023

Choose a reason for hiding this comment

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

Passive listeners are faster


this.$_recycledPool = {} as Record<string, any>
},
Expand All @@ -232,11 +231,14 @@ export default Vue.extend({
},

onScroll() {
const topScroll = this.$el.scrollTop - this.beforeHeight
const index = Math.floor(topScroll / this.itemHeight) * this.columnCount
// Max 0 to prevent negative index
this.index = Math.max(0, index)
this.$emit('scroll')
this._onScrollHandle ??= requestAnimationFrame(() => {
Copy link
Member Author

@pulsejet pulsejet Oct 18, 2023

Choose a reason for hiding this comment

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

Throttling this call to once per frame. Notably, this.$el.scrollTop is a getter and causes layout thrashing, so this is the most important change here.

Copy link
Member

@skjnldsv skjnldsv Oct 18, 2023

Choose a reason for hiding this comment

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

That's something I wanted to experiment with! Nice take at this issue! It make sense :)

this._onScrollHandle = null;
const topScroll = this.$el.scrollTop - this.beforeHeight
const index = Math.floor(topScroll / this.itemHeight) * this.columnCount
// Max 0 to prevent negative index
this.index = Math.max(0, index)
this.$emit('scroll')
});
},
},
})
Expand Down
4 changes: 2 additions & 2 deletions dist/files-main.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/files-main.js.map
View file
Open in desktop

Large diffs are not rendered by default.