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

walk: Add a cache for DirEntry metadata #863

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

tavianator
Copy link
Collaborator

This is a reimplementation of #402, using once_cell::unsync::OnceCell instead of a hand-rolled Lazy type. It gives some performance benefits when multiple filters require metadata:

$ hyperfine ./fd-{before,after}" -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null"
Benchmark #1: ./fd-before -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null
  Time (mean ± σ):     449.3 ms ±   5.1 ms    [User: 246.9 ms, System: 354.8 ms]
  Range (min … max):   441.1 ms … 455.1 ms    10 runs
 
Benchmark #2: ./fd-after -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null
  Time (mean ± σ):     392.5 ms ±   3.8 ms    [User: 230.1 ms, System: 287.9 ms]
  Range (min … max):   388.0 ms … 399.3 ms    10 runs
 
Summary
  './fd-after -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null' ran
    1.14 ± 0.02 times faster than './fd-before -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null'

without much affect on perf in other cases:

$ hyperfine ./fd-{before,after}" -j1 --search-path=../../linux --size=+1k >/dev/null"
Benchmark #1: ./fd-before -j1 --search-path=../../linux --size=+1k >/dev/null
  Time (mean ± σ):     383.8 ms ±   5.6 ms    [User: 212.7 ms, System: 296.1 ms]
  Range (min … max):   377.5 ms … 393.3 ms    10 runs
 
Benchmark #2: ./fd-after -j1 --search-path=../../linux --size=+1k >/dev/null
  Time (mean ± σ):     385.2 ms ±   4.1 ms    [User: 228.4 ms, System: 280.5 ms]
  Range (min … max):   378.3 ms … 393.3 ms    10 runs
 
Summary
  './fd-before -j1 --search-path=../../linux --size=+1k >/dev/null' ran
    1.00 ± 0.02 times faster than './fd-after -j1 --search-path=../../linux --size=+1k >/dev/null'

strace confirms a reduction in statx() calls:

$ strace -fc ./fd-before -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
...
 18.32    0.927790           3    254795     24015 statx
...
$ strace -fc ./fd-after -j1 --search-path=../../linux --size=+1k --owner=tavianator >/dev/null
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
...
 18.92    0.954145           5    181209     24015 statx
...

Together with #853 it would be possible to share the Metadata with the colorization which would be an additional benefit, but it's not done in this PR.

@tavianator
Copy link
Collaborator Author

There is a minor behaviour change (which you can see from the changed tests): --changed-* now examines the symlink's modification time itself rather than always following links. I think the new behaviour is better and the old behaviour was arguably a bug.

@tavianator tavianator force-pushed the cache-metadata branch 4 times, most recently from 9238757 to 635c53d Compare October 11, 2021 20:11
@tavianator
Copy link
Collaborator Author

Perhaps I can make it build on Windows after 4 tries? :P

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Cool - thank you very much!

@sharkdp
Copy link
Owner

sharkdp commented Oct 12, 2021

I have two remarks:

  • Can we please add an entry to CHANGELOG.md? For the caching itself and also for the bugfix concerning symlink modification times.
  • It might be a good idea to wait with merging this until Some readability refactorings #833 has been merged. What do you guys think?

@tavianator
Copy link
Collaborator Author

Yeah I'm happy to wait for #833. I'll add the changelog.

@sharkdp
Copy link
Owner

sharkdp commented Oct 12, 2021

Thank you. I merged #833.

@tavianator
Copy link
Collaborator Author

Rebased and added the CHANGELOG entry

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.

3 participants