Repository navigation
[hailtop.fs] use parallelism to list directories - #13253
Merged
danking merged 1 commit intoJul 18, 2023
Merged
Conversation
Qin He reported that listing a folder containing around 50k files took 1h15. This new code takes ~14 seconds which is about how long it takes `gcloud storage ls`. There are two improvements: 1. Use `bounded_gather2`. The use of a semaphore in `bounded_gather2`, which is missing from `bounded_gather`, allows it to be used recursively. In particular, suppose we had a semaphore of 50. The outer `bounded_gather2` might need 20 slots to run its 20 paths in parallel. That leaves 30 slots of parallelism left over for its children. By passing the semaphore down, we let our children optimistically use some of that excess parallelism. 2. If we happen to have the `StatResult` for a particular object, we should never again look it up. In particular, getting the `StatResult` for every file in a directory can be done in O(1) requests. Getting the `StatResult` for each of those files individually (using their full paths) is necessarily O(N). If there was at least one glob and also there are no `suffix_components`, then we can use the `StatResult`s that we learned when checking the glog pattern. The latter point is perhaps a bit more clear with examples: 1. `gs://foo/bar/baz`. Since there are no globs, we can make exactly one API request to list `gs://foo/bar/baz`. 2. `gs://foo/b*r/baz`. In this case, we must make one API request to list `gs://foo/`. This gives us a list of paths under that prefix. We check each path for conformance to the glob pattern `gs://foo/b*r`. For any path that matches, we must then list `<the matching path>/baz` which may itself be a directory containing files. Overall we make O(1) API requests to do the glob and then O(K) API requests to get the final `StatResult`s, where K is the number of paths matching the glob pattern. 3. `gs://foo/bar/b*z`. In this case, we must make one API request to list `gs://foo/bar/`. In `main`, we then throw away the `StatResult`s we got from that API request! Now we have to make O(K) requests to recover those `StatResult`s for all K paths that match the glob pattern. This PR just caches the `StatResult`s of the most recent globbing. If there is no suffix to later append, then we can just re-use the `StatResult`s we already have!
Contributor
|
Can you confirm there are already existing tests for each of the code paths used in this code? |
Contributor
Author
|
|
jigold
approved these changes
Jul 18, 2023
jigold
suggested changes
Jul 18, 2023
| *, | ||
| error_when_file_and_directory: bool = True, | ||
| _max_simultaneous_files: int = 50) -> List[StatResult]: | ||
| sema = asyncio.Semaphore(_max_simultaneous_files) |
Contributor
There was a problem hiding this comment.
Do we need to use async with sema?
Contributor
There was a problem hiding this comment.
Actually, I think this is fine and bounded gather2 does that.
jigold
approved these changes
Jul 18, 2023
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
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.
Qin He reported that listing a folder containing around 50k files took 1h15. This new code takes ~16 seconds which is about how long it takes
gcloud storage ls.There are two improvements:
Use
bounded_gather2. The use of a semaphore inbounded_gather2, which is missing frombounded_gather, allows it to be used recursively. In particular, suppose we had a semaphore of50. The outer
bounded_gather2might need 20 slots to run its 20 paths in parallel. That leaves 30 slots of parallelism left over for its children. By passing the semaphore down, we let our children optimistically use some of that excess parallelism.If we happen to have the
StatResultfor a particular object, we should never again look it up. In particular, getting theStatResultfor every file in a directory can be done in O(1) requests. Getting theStatResultfor each of those files individually (using their full paths) is necessarily O(N). If there was at least one glob and also there are nosuffix_components, then we can use theStatResults that we learned when checking the glog pattern.The latter point is perhaps a bit more clear with examples:
gs://foo/bar/baz. Since there are no globs, we can make exactly one API request to listgs://foo/bar/baz.gs://foo/b*r/baz. In this case, we must make one API request to listgs://foo/. This gives us a list of paths under that prefix. We check each path for conformance to the glob patterngs://foo/b*r. For any path that matches, we must then list<the matching path>/bazwhich may itself be a directory containing files. Overall we make O(1) API requests to do the glob and then O(K) API requests to get the finalStatResults, where K is the number of paths matching the glob pattern.gs://foo/bar/b*z. In this case, we must make one API request to listgs://foo/bar/. Inmain, we then throw away theStatResults we got from that API request! Now we have to make O(K) requests to recover thoseStatResults for all K paths that match the glob pattern. This PR just caches theStatResults of the most recent globbing. If there is no suffix to later append, then we can just re-use theStatResults we already have!cc: @daniel-goldstein since you've reviewed this before. Might be of interest.