Open starypatyk opened 1 year ago
Baseline results from my dev environment (331 files in a folder). When entering a folder the app executes the following API calls:
Request | Execution time [ms] | # of DB queries |
---|---|---|
WebDAV PROPFIND - check ETags (always) | 45 | 33 |
WebDAV PROPFIND - full (only when folder changed) | 1300 | 1699 |
Share information (always) | 918 | 2006 |
Total | 2263 | 3738 |
Thanks for your awesome help and it was very nice to meet you at our conf!
Updated results from my dev environment (331 files in a folder) - with changes from https://github.com/nextcloud/server/pull/34471, https://github.com/nextcloud/server/pull/34508 and https://github.com/nextcloud/server/pull/34918.
Request | Execution Time [ms] (before) | Execution Time [ms] (after) | # of DB queries (before) | # of DB queries (after) |
---|---|---|---|---|
WebDAV PROPFIND - check ETags | 45 | 50 | 33 | 34 |
WebDAV PROPFIND - full | 1300 | 250 | 1699 | 41 |
Share information | 918 | 50 | 2006 | 25 |
Total | 2263 | 350 | 3738 | 100 |
Awesome work and super nice improvements with the update(s) @starypatyk 🚀
After upgrading my production server to 25.0.1 and applying changes from https://github.com/nextcloud/server/pull/34918 manually (as these are not yet merged), I have found that there is still room for improvements on the client side. :wink:
I have updated the list in the first comment above and plan to submit additional issues/PRs soon.
Thanks @starypatyk , this is very useful information.
Reading file information from the local SQLite database (FileDataStorageManager::getFolderContent) takes much longer than expected. Profiling suggests that most of the time is spent inside createFileInstance method in two operations:
DB code is quite inefficcient in the app in general; and it's also not helping that all DB calls go through a ContentResolver and get executed in the main thread. I introduced Room recently (#10977) to incrementally deal with some of that; it should at least get rid of the getColumnIndex
load. But it needs to actually be applied to the files table access code first.
The ugly truth is that in order to have a properly working DB (see #10213 ) at some point it will be unavoidable to change DB structure to remove serialized fields, and similar stuff, and replace them with proper SQL relations.
@AlvaroBrey thanks for your comments. You have encouraged me to try Room: #11098. :wink: Seems quite promising.
@starypatyk thanks for your engagement and focus on performance! :tada:
A short summary of the client-side improvements.
Hopefully #11098 and #11100 will get merged soon. With these changes, in my test environment (331 files in a folder), the total time spent in OCFileListFragment::listDirectory
decreased roughly 3 times. This is in the main thread, so most affecting UX.
listDirectory | Avg time [ms] | # of executions | Total time [ms] |
---|---|---|---|
Original code | 626 | 3 | 1878 |
With #11098 and #11100 | 341 | 2 | 682 |
However, I still see some room for improvement. :wink:
The FileDataStorageManager::saveSharesInFolder
method currently executes a lot of delete and insert operations. First it first removes share data for all files in a folder (separately for each file) and then inserts (usually back) the shares retrieved from the server.
I suggest to refactor this method to first read information for all shares in the folder (hopefully using a single DB query), compare the local data with the data coming from the server and execute inserts/updates/deletes only for relevant changes.
This will have an additional benefit - when no changes are detected, it should be possible to skip the EVENT_SINGLE_FOLDER_SHARES_SYNCED
notification and avoid UI refresh after reading shares.
When reading file data from the local DB, still a significant portion of the time is spent in FileDataStorageManager::createFileInstance
. See the profile below. I would suggest delaying execution of the time consuming operations - i.e. fromJson
, File.exists
and getDefaultSavePathFor
until the results are actually read from the OCFile
object. I suspect that in many cases the relevant properties: storagePath
, sharees
, imageDimension
might not be required for a long time - e.g. until the user actually scrolls down to the relevant file.
I plan to work on these two areas.
Thanks for this great analysis! I suggest to start with 2., as for 1. it might be that we have at some point a better way, see https://github.com/nextcloud/server/issues/8477
Regarding point 2, I've also toyed with the idea of a master-detail view of the files list. That would be having something like a OCFileReference
which is a reduced view of an OCFile
with only the bare minimum info necessary, that then would get completed when needed.
However, I'm afraid that you'll run into many structural problems due to tech debt when trying to accomplish this sort of things. The app's components are very tightly bound to OCFile and related classes. Furthermore, when delaying costly operations (like json parsing) you'll probably find that you now have lag when rendering stuff, as (again due to tech debt) the app executes the vast majority of its logic in the UI thread. Or you may find that the costly operation is run later, but several times in different classes and threads :shrug:
Btw, the entire file.exists()
thing may be completely skippable at this point, or maybe fenced behind an app version check so it's only executed for old versions, judging by the comment there.
As always, reach out if you need any help! And good luck
Btw, the entire
file.exists()
thing may be completely skippable at this point, or maybe fenced behind an app version check so it's only executed for old versions, judging by the comment there.
Yeah, that comment is there since at least 2014. We recently dropped support for app versions < 2.0 (in #10977), which is from 2017. The comment says:
// try to find existing file and bind it with current account;
// with the current update of SynchronizeFolderOperation, this won't be
// necessary anymore after a full synchronization of the account
I'm pretty sure this does not apply anymore, although tests are needed. Both attributes are set on different parts of createFileInstance
directly from the DB, so we may be able to just remove this block entirely.
Regarding JSON: if this is too expensive, let us only parse it once, and store it in real DB. With Room support this is hopefully now easier? One wold be ShareeUser, which is a 1:N.
@AlvaroBrey @tobiasKaminsky - thanks for your comments and suggestions.
I have started working on point 2 - delaying JSON parsing and potentially removing File.exists
check. I plan to submit a PR soon.
I have noticed a few issues related to performance/usability when navigating between folders - esp. when a folder contains larger number of files. In my testing I use a folder with about 300 photos.
I plan to create separate issues/PRs for each of the items below and use this issue to track the overall progress.
Initial attempt to improve this (#11181) abandoned.Simpler approach proposed: #11251.