Fix UI freeze when initiating a copy/move of a big file tree #3151
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The percentage bar was always calculated in terms of number of bytes, not number of units `step`. Mixing the units causes a visual bug where the percentage bar doesn't reflect actual progress. This fix also improves performance since we don't traverse the tree twice to calculate the total size in different units.
Fixes performance bug where the UI could temporarily freeze when copying or moving large trees of files, because the size calculation would never yield.
Renaming is an I/O operation, so it could take a significant amount of time on networked filesystems. We should probably yield to keep the UI running even under such cases.
Contributor
Author
|
Lines 103 to 107 in 21e3c19 Ranger does a lot of I/O here to pre-calculate the total size of files moved, just for the progress bar. Is it really worth it or shouldn't we remove it / make it a setting? On my system it makes a huge difference to just skip this step and go ahead with the move/copy. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before moving or copying files, CopyLoader traverses the full directory trees two times to calculate the total file size. This traversal is now only done once and generatorized to prevent Ranger from freezing when there is a large number of files to move. File moves that require just a rename will also yield now, again to avoid a UI hiccup when there are many items to move or we're on a slow/networked file system.
ISSUE TYPE
x Bug fix
RUNTIME ENVIRONMENT
CHECKLIST
CONTRIBUTINGdocument has been read [REQUIRED]DESCRIPTION
shutil_generatorized.move(): yield after renamingCopyLoader._calculate_size(): yield after calculating the size of a file or listing a directoryCopyLoader.generate(): calculate the file size by traversing the tree only once. Calculateself.percentin terms of bytes since this is whatshutil_generatorizedgives usMOTIVATION AND CONTEXT
Traversing the full tree of files to be moved can take a lot of time, rendering Ranger completely unresponsive. Such work should be generatorized.
We also fix the percentage calculation and no longer traverse the tree twice, see commit message: 3e39b67
shutil_generatorized.move()will yield periodically when moving files, except when a file can be renamed to its destination. Instead it should probably always yield since renaming files is an I/O operation with performance impact depending on how slow the filesystem is or the number of files being renamed.TESTING
Tested by moving directories containing 100K+ files in total, from one folder to another. After the changes, the freezes no longer occur.
Changes should improve the performance of the
:pastecommand. No adverse changes to the rest of the codebase since onlyCopyLoaderusesshutil_generatorized.