avoid isfile calls on find as much as possible #676
Merged
Conversation
This file contains 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
If `walk` is also returning the same file, it should already have been included in the `out`. If `out` is not empty, we can safely assume that it is either a directory or a file. If it's empty, it could be that it is either an empty directory or, the path is a file and was not included from the `walk` results. This time we will make a `isfile` call. This is unfortunate but better than before, where we were making an additional for each find. This was noticed in DVC, which was making remote cache querying slower when traversing through the caches (512 calls instead of 256 :( ).
|
It looks good and passes tests; let me think about your argument, whether is covers all cases. |
|
Looking at my own code, I'm not totally certain what that condition is supposed to achieve. Could you please make sure that there's a test that does |
skshetry
commented
Jun 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In
find, ifwalkalso includes thepathif it's a file, it should already be included in theout.So, if
outis not empty, we can safely assume that it is either a directory or a file. So, we don't needto make an extra
isfilecall.Though, if the path was an empty directory or if the walk does not return the file, we'll still be making an
isfilecall, which is unfortunate but better than before, where we were making an additional call for each find.This was noticed in DVC when trying to migrate to WebdavFS, which was making remote cache querying noticeably slower when traversing through the caches.
Please let me know if I'm missing something here, or anything else that needs to be considered here. :)