-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Significantly improve overall performance and UI responsiveness #3045
Conversation
The UI responsiveness is awesome, but file caching removal brings back the most annoying (for me) part that i'm waiting for files to load on every navigation. So i'd argue that it should be refined and integrated, not removed |
According to my test result, loading 5000 entries only need less than 2 seconds after this PR, which is almost equal to loading those entries from SQLite cache. Removing the cache is a temporary workaround and I think we can improve the implementation of cache in the future (maybe after this PR). |
5000 entries is very unlikely scenario, we can talk about 20-30 files in folder on average i think? Loading 30 entries from SQLite takes 80 ms at my machine, while loading them from drive takes around 800-1000. Also - SQLite is only second line of cache. Memory cache is significantly faster, it also stores extended properties. Renavigating to visited path is so much faster with this. I know that UI responsiveness is great, but the basic "fast navigation" flow is crucial for productiveness. You can take a look at this PR: #2944 . It implements preemptive caching, which on fast drives preloads child folders into memory cache. There is no problem with clearing incorrect cache on enumeration, it's just one event on |
@hez2010 I think we could store cache entries as 1 row per file/folder. This way we could even get rid of json serialization. Then on cache load we could "preload" 30 entries and then load the rest, just like during ordinal loading. Just to confirm - DataGrid have issues with loading a lot of data at once? Maybe simple partitioning of list would be enough (for example https://stackoverflow.com/a/1396143/939133)? Something like: |
@jakoss
Update: there's no performance issue with DataGrid even dealing with 10k entries. |
I think we can do similar strategy for preloaded list from cache somewhere here: Files/Files/ViewModels/ItemViewModel.cs Lines 814 to 818 in a7479a3
|
62841f2
to
556b761
Compare
I've rebased this PR from master |
dc8a761
to
41d35b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, app is now amazingly responsive
96efb25
to
ee0aefa
Compare
I've added an interval sampler for updating UI which managed to improve the loading performance |
b68d15b
to
f76c98c
Compare
Rebased from main branch. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@gave92 This is expected because the collection is changing from time to time while loading entries, which causes GridView to rerender. In master the UI just freeze while loading so there is no flash. |
This is due to the fact that bulk observable collection fires a CollectionReset event, right? Which causes the gridview to rerender completely. |
@gave92 Yes, the |
With this PR, UI responsiveness should be significantly improved. For test, just open
C:\Windows\System32
and UI almost no longer freezes.Before (UI freeze everywhere, and after entering
System32
the UI froze for several seconds and cannot operate UI):After (Even using debug build is smoother than before, and there's almost no UI freezes even after entering
System32
andWinSxS
, you can operate UI immediately):Change Notes
Task.Run
filesAndFolders
, which is aList<ListedItem>
ApplyFilesAndFoldersChangesAsync
to update UI, which calculate changes incrementally to avoid unnecessary list entry additions as many as possibleBulkObservableCollection<ListedItem> FliesAndFolders
should be only used by Bindings andApplyFilesAndFoldersChangesAsync
Task.Run
to make list sorting as a background task to avoid UI freezingloadExtendedPropsSemaphore
to match the number of CPU processorsmatchingItem
lookup inLoadExtendedItemProperties
to avoid the possibility of enumerating on a modified list which may crash the whole program. AlsomatchingItem
is unnecessary because the parameteritem
already comes fromfilesAndFolders
and it's ok if the item doesn't exist infilesAndFolders
Note:
Contributes to #1769 and #2527 and #3062