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

Fast extension matching #87

Merged
merged 6 commits into from
Aug 19, 2024
Merged

Conversation

tavianator
Copy link
Contributor

@tavianator tavianator commented Jul 5, 2024

I say "fast" but I didn't actually benchmark it yet. Benchmarks below. It's definitely better from a time complexity perspective.

This is basically how I do it in bfs, except I used aho-corasick rather than write my own trie implementation.

Includes #86.

@matrixhead
Copy link
Contributor

woah 😃, this is much better!

@tavianator
Copy link
Contributor Author

So it looks like this is about a 4% single-threaded perf win for fd --color=always:

Command Mean [s] Min [s] Max [s] Relative
./fd-master -j1 --color=always --search-path ~/code/bfs/bench/corpus/rust 1.488 ± 0.009 1.471 1.500 1.04 ± 0.01
./fd-pr87 -j1 --color=always --search-path ~/code/bfs/bench/corpus/rust 1.424 ± 0.011 1.408 1.441 1.00

With multiple threads it's neutral, I guess suffix matching is off the critical path:

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master --color=always --search-path ~/code/bfs/bench/corpus/rust 126.3 ± 3.2 117.8 130.5 1.00
./fd-pr87 --color=always --search-path ~/code/bfs/bench/corpus/rust 127.9 ± 4.0 120.6 135.3 1.01 ± 0.04

@tavianator
Copy link
Contributor Author

The win is more significant for larger LS_COLORS. E.g. with LS_COLORS=$(vivid generate molokai):

Command Mean [s] Min [s] Max [s] Relative
./fd-master -j1 --color=always --search-path ~/code/bfs/bench/corpus/rust 1.571 ± 0.016 1.551 1.594 1.14 ± 0.02
./fd-pr87 -j1 --color=always --search-path ~/code/bfs/bench/corpus/rust 1.382 ± 0.013 1.370 1.404 1.00

@sharkdp
Copy link
Owner

sharkdp commented Jul 16, 2024

Thank you very much, this looks great.

Doing an "integration" benchmark on fd is a nice test & verification of your work, but I wonder if it would be more helpful to define two separate micro-benchmarks inside this crate, e.g. using criterion. I did something similar for another project yesterday and it usually just takes a few minutes to set this up (see tutorial here: https://bheisler.github.io/criterion.rs/book/user_guide/migrating_from_libtest.html#the-benchmark).

I imagine one benchmark would measure how long it takes to initialize a LsColors instance. And the other would focus on how fast we can match and colorize a given path. We could parametrize this on multiple inputs, e.g. short/medium/long LS_COLORS, and a few different classes of paths (short/long, with-match/without-match).

I'm not saying that you should do this, I just think it would give us a clearer picture of the situation. Performance work on this crate is extremely valuable. Initialization time has a direct impact on the startup time of many CLI programs. And the time to match a given path is crucial in programs like fd that need to print thousands of paths. Sorry if I digress, but I find it entertaining to think about the collectively saved time when doing even tiny performance improvements on those functions. And this seems to be a lot more than a tiny improvement. But at the moment, I can't really tell how this divides into initialization time and matching time.

@tavianator
Copy link
Contributor Author

Yeah some microbenchmarks would be nice. I'm a little busy right now though, but I can keep that on the back burner unless you get to it first!

@sharkdp sharkdp merged commit 430c591 into sharkdp:master Aug 19, 2024
48 checks passed
@tavianator tavianator deleted the fast-extensions branch August 19, 2024 15:46
@tavianator tavianator mentioned this pull request Sep 10, 2024
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