Skip to content
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

ListObjects: don't recurse beyond prefix #904

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

lunixbochs
Copy link
Contributor

This fixes the biggest performance issue with ListObjects for me - it would recurse into all subdirectories, even when using a delimiter.

There are still a few issues:

  • Large directories take forever to list.
  • Parent directories are still walked for no reason - perhaps the posix backend should scope the os.DirFS() to the last directory in the prefix rather than starting at the root, and stitch the paths back together in p.fileToObj. I looked at doing that but wasn't sure if I'd hit a prefix = "" edge case.

Also fixes #903

@versity-github
Copy link
Collaborator

This won't automatically run in continuous integration without approval. A member of the Versity organization must allow it.

@@ -85,8 +84,16 @@ func Walk(ctx context.Context, fileSystem fs.FS, prefix, delimiter, marker strin
// building to match. So only skip if path isn't a prefix of prefix
// and prefix isn't a prefix of path.
if prefix != "" &&
!strings.HasPrefix(path+string(os.PathSeparator), prefix) &&
!strings.HasPrefix(prefix, path+string(os.PathSeparator)) {
!strings.HasPrefix(path+"/", prefix) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set a const in this file to "/" and use that instead of using "/" everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this, it reduces readability, fs.FS is never going to use something other than a slash, and several other files in this repo also hardcode "/"

backend/walk.go Show resolved Hide resolved
@lunixbochs
Copy link
Contributor Author

The new logic should work with any delimiter:

if delimiter != "" &&
    strings.HasPrefix(path+"/", prefix) &&
    strings.Contains(strings.TrimPrefix(path+"/", prefix), delimiter) {
    skipflag = fs.SkipDir

If the delimiter is non-empty, and our path is beyond the prefix, and the delimiter is still in the directory path after trimming the prefix from it, we shouldn't recurse.

I fixed the pagination issue by not doing an early return, instead opting to set a skipflag variable which is used instead of return nil for the rest of the walk closure.

@benmcclelland benmcclelland merged commit 56a2d04 into versity:main Oct 29, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] - os.PathSeparator incorrect for io/fs
3 participants