-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve performance/usability when navigating between folders #10783
Comments
Baseline results from my dev environment (331 files in a folder). When entering a folder the app executes the following API calls:
|
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 nextcloud/server#34471, nextcloud/server#34508 and nextcloud/server#34918.
|
Awesome work and super nice improvements with the update(s) @starypatyk 🚀 |
After upgrading my production server to 25.0.1 and applying changes from nextcloud/server#34918 manually (as these are not yet merged), I have found that there is still room for improvements on the client side. 😉 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.
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 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. 😉 Seems quite promising. |
@starypatyk thanks for your engagement and focus on performance! 🎉 |
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
However, I still see some room for improvement. 😉
I plan to work on these two areas. |
Thanks for this great analysis! |
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 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 🤷 Btw, the entire As always, reach out if you need any help! And good luck |
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:
I'm pretty sure this does not apply anymore, although tests are needed. Both attributes are set on different parts of |
Regarding JSON: |
@AlvaroBrey @tobiasKaminsky - thanks for your comments and suggestions. I have started working on point 2 - delaying JSON parsing and potentially removing |
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 (Defer expensive operations until results are read from OCFile #11181) abandoned.Simpler approach proposed: Avoid JSON deserialization in common trivial cases #11251.The text was updated successfully, but these errors were encountered: