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

Do not pre-compute MountInfo to reduce readlink calls #834

Merged
merged 1 commit into from
Feb 10, 2024

Conversation

tertsdiepraam
Copy link
Contributor

More speedups! In my previous PR I mentioned being baffled by the readlink and getcwd and I figured out where they came from: File::mount_point_info, which calls std::fs::canonicalize. The easy fix was to not call that when it's not necessary, which turns out to be most of the time!

Here's a benchmark:

❯ hyperfine -L eza eza-old,eza-new "./{eza} -R"
Benchmark 1: ./eza-old -R
  Time (mean ± σ):     744.0 ms ±   9.3 ms    [User: 275.2 ms, System: 465.3 ms]
  Range (min … max):   734.2 ms … 766.0 ms    10 runs
 
Benchmark 2: ./eza-new -R
  Time (mean ± σ):     313.3 ms ±   3.0 ms    [User: 142.2 ms, System: 169.4 ms]
  Range (min … max):   309.7 ms … 318.8 ms    10 runs
 
Summary
  ./eza-new -R ran
    2.37 ± 0.04 times faster than ./eza-old -R

@tertsdiepraam tertsdiepraam force-pushed the fewer-readlink-calls branch 2 times, most recently from 1a60789 to 3c26021 Compare February 9, 2024 18:41
@daviessm
Copy link
Contributor

daviessm commented Feb 9, 2024

Thanks for doing this! I obviously didn't check the performance when I added this functionality. It seemed logical at the time.

@tertsdiepraam
Copy link
Contributor Author

Yeah it does seem logical. It feels strange that canonicalize would be that expensive (even though it has to work like it does). I find that in Rust it's sometimes hard to tell which functions do syscalls. I only found this by looking at flame graphs.

@PThorpe92
Copy link
Member

#558 this always made me curious as to what exactly was the cause of this (obviously not to say it's one thing in particular, in fact this may not have even been merged at the time of the issue). But I agree it's definitely not always obvious.

Big thanks again. Looks like we are heading for a significant perf improvement :D

@cafkafk
Copy link
Member

cafkafk commented Feb 10, 2024

This is so nice, thanks a lot :3

re: it being hard to guess what slows down eza, I've though for a while that we should include some performance testing step in reviews, but I haven't really found the time to figure out how we should do it.

@cafkafk cafkafk merged commit defe8c9 into eza-community:main Feb 10, 2024
16 checks passed
@tertsdiepraam
Copy link
Contributor Author

To be clear, I wasn't blaming eza for that. It's just hard in general. At uutils we've been thinking about tracking the number of syscalls, maybe that could work for you? We haven't set that up yet, because it doesn't feel super important in the end.

1 similar comment
@tertsdiepraam
Copy link
Contributor Author

To be clear, I wasn't blaming eza for that. It's just hard in general. At uutils we've been thinking about tracking the number of syscalls, maybe that could work for you? We haven't set that up yet, because it doesn't feel super important in the end.

@cafkafk
Copy link
Member

cafkafk commented Feb 10, 2024

At uutils we've been thinking about tracking the number of syscalls, maybe that could work for you?

That sounds like an okay solution actually, we should do this.

Also NW didn't think you were blaming eza!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants