Skip to content

feat: implement quicker backend/posix walk algorithm#1889

Merged
benmcclelland merged 1 commit intomainfrom
ben/fast-walk
Mar 19, 2026
Merged

feat: implement quicker backend/posix walk algorithm#1889
benmcclelland merged 1 commit intomainfrom
ben/fast-walk

Conversation

@benmcclelland
Copy link
Copy Markdown
Member

@benmcclelland benmcclelland commented Feb 23, 2026

Rewrite the posix Walk implementation to avoid the extra ReadDir per directory that was noted as a TODO in the old code. The new algorithm holds all traversal state in a walkState struct and uses processDir to interleave sibling entries in correct S3 lexicographic order without a second syscall.

Key changes:
prefix optimisation: jump directly into the deepest matching directory rather than scanning from the root on every call

marker short-circuit: skip entire subtrees that are lexically before the marker, making paginated listing faster

Fixes #1845
Fixes #1855

@benmcclelland
Copy link
Copy Markdown
Member Author

@tonyipm I added a benchmark test in here, and got the following results (but didnt run many times, so these may not be statistically significant):

Benchmark main (old) new Δ
WalkFlat 11.26 ms 12.12 ms +7.6% slower
WalkFlatDelim 11.10 ms 12.06 ms +8.6% slower
WalkFlatPaged 22.94 ms 26.99 ms +17.6% slower
WalkDeep 13.21 ms 12.34 ms -6.6% faster
WalkDeepDelim 87.2 µs 70.5 µs -19.1% faster
WalkWide 28.68 ms 27.71 ms -3.4% faster
WalkWideDelim 154.3 µs 127.1 µs -17.6% faster
WalkWithPrefix 220.2 µs 269.6 µs +22.4% slower
WalkWithPrefixDelim 38.37 µs 33.95 µs -11.5% faster
WalkWithMarker 1.384 ms 1.029 ms -25.6% faster
geomean 1.741 ms 1.676 ms -3.7%

Can you try running the tests, or adding to the benchmarks to see if this really is speeding up the cases in question. Also, we should consider if this is slowing down any cases that we might need to address.

@benmcclelland
Copy link
Copy Markdown
Member Author

tested again with some larger directories, these are relative to current main:

Benchmark main new Δ p-value
WalkFlatPaged 25.0 ms 23.4 ms -6.6% p=0.004 ✓
WalkDeepDelim 95.9 µs 77.6 µs -19.0% p=0.002 ✓
WalkWideDelim 171 µs 135 µs -21.2% p=0.002 ✓
WalkWithPrefix 256 µs 289 µs +12.7% p=0.004 ✓
WalkWithPrefixDelim 46.7 µs 39.0 µs -16.6% p=0.004 ✓
WalkWithMarker 1.64 ms 1.08 ms -34.4% p=0.041 ✓
WalkFlat 12.6 ms 11.9 ms ~ p=0.394 (noise)
WalkFlatDelim 12.0 ms 12.0 ms ~ p=0.699 (noise)
WalkDeep 14.1 ms 13.4 ms ~ p=0.485 (noise)
WalkWide 32.1 ms 31.4 ms ~ p=0.818 (noise)
geomean 1.95 ms 1.74 ms -10.8%  

@tonyipm
Copy link
Copy Markdown
Contributor

tonyipm commented Feb 24, 2026

Thanks Ben I will take a look - in my own performance testing I found it consistently faster than the old algo and where performance was similar I ran a profile and found nearly all the elapsed time was in os.ReadDir. I will see if I can run up your perf tests and profile them.

@benmcclelland
Copy link
Copy Markdown
Member Author

Thanks Ben I will take a look - in my own performance testing I found it consistently faster than the old algo and where performance was similar I ran a profile and found nearly all the elapsed time was in os.ReadDir. I will see if I can run up your perf tests and profile them.

I didn't do extensive testing, but just a few cases of "large" directories vs fan out into multiple directories. I should probably fire up some tests with really large directories since thats the real case we are trying to solve here. Under a million files seems too fast on my system for any useful analysis.

@benmcclelland
Copy link
Copy Markdown
Member Author

@tonyipm I wanted to make sure to revisit this PR. I think the speed up in the WalkFlatPaged and WalkWithMarker test may be worth landing this. I was curious if you had done any more testing and have any further results?

@benmcclelland benmcclelland force-pushed the ben/fast-walk branch 2 times, most recently from 820be45 to d35229b Compare March 13, 2026 19:12
@tonyipm
Copy link
Copy Markdown
Contributor

tonyipm commented Mar 17, 2026

Thanks for the update Ben. I have done a lot of performance testing with this for our product workload, concentrating on the wide and deep directory structures that our customers have, and I find it consistently faster for those workloads. These cases have run times in seconds and minutes rather than ms and microseconds, and the improvement gets significantly greater the wider / deeper the directory. Thanks for bringing this into the main product code, this will make things a lot easier for us!

@benmcclelland
Copy link
Copy Markdown
Member Author

I found another regression that i added a fix for in this. I need to do a little more testing to make sure nothing else missed here.

@benmcclelland benmcclelland force-pushed the ben/fast-walk branch 3 times, most recently from 3609882 to 80dba22 Compare March 17, 2026 17:38
Rewrite the posix Walk implementation to avoid the extra ReadDir per
directory that was noted as a TODO in the old code.  The new algorithm
holds all traversal state in a walkState struct and uses processDir to
interleave sibling entries in correct S3 lexicographic order without a
second syscall.

Key changes:
prefix optimisation: jump directly into the deepest matching directory
rather than scanning from the root on every call

marker short-circuit: skip entire subtrees that are lexically before
the marker, making paginated listing faster

Co-authored-by: Ben McClelland <ben.mcclelland@versity.com>
@benmcclelland
Copy link
Copy Markdown
Member Author

benmcclelland commented Mar 19, 2026

@tonyipm I think I have all the issues resolved that I could find in this latest version now. Do you want to do a final review of these changes?

Also, I added a bunch of tests based on my regression and new issue testing.

Copy link
Copy Markdown
Contributor

@tonyipm tonyipm left a comment

Choose a reason for hiding this comment

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

Looks good, approved.

@benmcclelland benmcclelland merged commit 88e6396 into main Mar 19, 2026
120 checks passed
@benmcclelland benmcclelland deleted the ben/fast-walk branch March 19, 2026 23:00
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] - list objects not returning correct results with delimiter and start-after defined

2 participants