Improve performance of adding and removing files #1949
Conversation
Previously, adding 1300-ish files took around 260ms, and looked like this in Firefox's performance tab:  All of the downward peaks are `setState()` calls and GC. Now it takes around 60ms, and looks like this:  Here, most of the time is spent generating file IDs and guessing file types. Those would be areas to look at next.
After the last commit, `addFiles()` still spends a lot of time in `emit()` and `hideAllPanels()`. The reason is that `addFiles()` still emits all the hundreds of file-added events, and the Dashboard responds to each with `hideAllPanels()`, which does a state update. But this all happens synchronously, and the state almost certainly did not change since the last `file-added` event that fired a millisecond ago. This adds a check to avoid the state update if it is not necessary.  Adding 1300 files takes about 40ms now. With this change, the `addFiles()` call is no longer the slowest part—now preact rendering all the items is!
Replaces some clever things with more mundane and faster things! Now, generateFileID is a bunch of string concatenations. Now, getFileNameAndExtension uses `lastIndexOf()` instead of a regex.  Adding 1300 files takes about 25ms.
| module.exports = (props) => { | ||
| const noFiles = props.totalFileCount === 0 | ||
| const dashboardFilesClass = classNames( | ||
| 'uppy-Dashboard-files', | ||
| { 'uppy-Dashboard-files--noFiles': noFiles } | ||
| ) | ||
|
|
||
| // 190px height + 2 * 5px margin | ||
| const rowHeight = 200 |
There was a problem hiding this comment.
Maybe we could add a comment in CSS that the height is tied to this property in JS as well?
There was a problem hiding this comment.
It actually also needs different heights for different uppy-size--xyz classes too, will also add that. 190 + 2*5px is only for uppy-size--lg
|
This is AMAZING. Really fast with 10,000 files on Firefox and Chrome, Mac OS.
|
|
|
|
AMAZING again, “cancel all” is instantaneous now. We should also probably change |
|
Great job!
Virtualizing doesn't work very well for me, Chrome MacOS + 3055 files + width 531px results in files getting invisible for me:
They become visible again once I scroll past some point, and then they disappear again. |
|
|
||
| if (errors.length > 0) { | ||
| const err = new Error('Multiple errors occured while adding files') | ||
| err.errors = errors |
There was a problem hiding this comment.
Hm, if one error occurs after we try to add one file, - we will get a generic Error: Multiple errors occured while adding files, with no indication of what happened.
That could work if we were catching and logging error.errors everywhere after we use .addFiles, but we don't yet do that.
Should we log each error ourselves in addFiles() instead? That would also make:
try {
this.uppy.addFiles(descriptors)
} catch (err) {
this.uppy.log(err)
}
tradition unnecessary?
PS. Looks like it's occurrRred (https://en.wiktionary.org/wiki/occured).
|
I think resolving the potential accessibility issues with VirtualList is gonna take some time so I'll prob pull it out of this PR into a different one, so we can get the add/removeFiles improvements in soon |
|
@goto-bus-stop, have you tried something like https://github.com/bvaughn/react-window? Looks like it works well with tabs. |
goto-bus-stop
changed the title
|
💪 🚀 |
This PR contains a bunch of performance improvements. I just
applied them and tested the happy path, so we need to check if
everything still works 🙈
If we want to get this in earlier we could land the
addFile()optimizations first, and the virtualization stuff later with more
testing.
Overview:
core.addFiles()Previously, adding 1300 files took around 260ms, and looked like
this in Firefox's performance tab:
All of the downward peaks are
setState()calls and GC. The firstcommit adds an
addFiles()method that takes an array of files, anddoes setup work for all of them before doing a single
setState()call.
Now it takes around 60ms, and looks like this:
Here, most of the time is spent generating file IDs and guessing file
types. Those would be areas to look at next.
hideAllPanels()After the last commit,
addFiles()still spends a lot of time inemit()andhideAllPanels(). The reason is thataddFiles()stillemits all the hundreds of file-added events, and the Dashboard responds
to each with
hideAllPanels(), which does a state update. But this allhappens synchronously, and the state almost certainly did not change
since the last
file-addedevent that fired a millisecond ago.This adds a check to avoid the state update if it is not necessary.
Adding 1300 files takes about 40ms now.
With this change, the
addFiles()call is no longer the slowestpart—now preact rendering all the items is!
generateFileIDandgetFileNameAndExtensionReplaces some clever things with more mundane and faster things!
Now, generateFileID is a bunch of string concatenations.
Now, getFileNameAndExtension uses
lastIndexOf()instead of a regex.Adding 1300 files takes about 25ms.
Virtualizing<FileList />This is using an adaptation of preact-virtual-list to render only the
file items that are visible or close enough to be visible.
I didn't properly measure the performance impact of this, so that's
unclear.
Lazy thumbnail generationThis patch adds a
lazy: trueoption to the ThumbnailGenerator plugin,which will cause it to wait for
thumbnail:requestevents beforegenerating a thumbnail for a file. When a
<FileItem />is mounted, itemits one of these events.
If the
waitForThumbnailsBeforeUploadoption is set, lazy generation isdisabled.