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

Reuse FileType from DirEntry #833

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

tertsdiepraam
Copy link
Contributor

@tertsdiepraam tertsdiepraam commented Feb 9, 2024

This is a draft because I made this PR more to spark some conversation than to actually change the code. It could still be merged if I clean it up but (spoiler alert) the improvement is very small for now. So here goes.

Note: I also have to double check the deref_links logic. I'm pretty sure I got that wrong, but it wasn't my focus for now. And the error handling is sloppy too.

eza is super fancy, but sometimes quite slow. As seen in some issues and discussions:

(and probably some more, I've only started tracking this recently)

This is not necessarily a problem: as I understand it, eza is meant for fancy output first and fast output second. It fulfills the promise of being fancy very well and that fanciness sets it apart from e.g. GNU (and uutils) ls. Still, the fact that complains are coming in shows that people would like eza to be faster.

Now, I've done a bunch of work on uutils ls, including performance work, so I figured I'd share some of my experience. In my experience, it's quite hard to get ls-like programs to a low number of syscalls. If you want to try this for yourself, compare these commands (on Linux):

strace -c eza
strace -c ls

Here's the top of both, first eza:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 30,10    0,000584           9        59        59 readlink
 25,57    0,000496           8        60           statx
 25,46    0,000494           8        59           getcwd
  8,45    0,000164          16        10           read

and then GNU ls:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
  0,00    0,000000           0         2           read
  0,00    0,000000           0         8           write
  0,00    0,000000           0         7           close
  0,00    0,000000           0        13           mmap

You can see that eza spends a lot of time on syscalls that's probably not necessary!

So, I set out to see if I could reduce that with a similar technique as we used in uutils ls: retrieving metadata and filetype only when necessary. In code, it's similar to how the extended attributes and canonicalized name are computed with a OnceLock. Now there are two options:

  • we create a file from a filename, in which case we have to get the metadata always, or
  • we create a file from a DirEntry, which already contains a FileType, so if we have that, we can often skip getting the metadata.

So I implemented that and the number of statx calls from running eza on its own repository went from 33 to... 23. Not quite what I had hoped. Turns out that's because on every regular file, eza wants to know whether the file is executable, which requires the full metadata. Removing that check drops the statx calls to 0, which I think would lead to a measurable speedup, but I've left that out of this PR, because that would change eza's behaviour.

This all raises some interesting questions (which are probably worth spinning off into an issue):

  • What are all the readlink/getcwd calls for? They take up a lot of time and are probably unnecessary.
  • Can we redesign the theme code such that we can avoid checks that have no influence on the style. For example, only check that a file is executable if a custom style is set for executable files. This would speed up eza even more with --color=never.
  • Should there be a setting which avoids doing expensive coloring beyond the simple filetype? With the previous point this could be implemented quite easily with a theme that only does the cheap checks.

Hope this all makes sense :)

@tertsdiepraam tertsdiepraam changed the title Reuse filetype from DirEntry Reuse FileType from DirEntry Feb 9, 2024
@PThorpe92
Copy link
Member

PThorpe92 commented Feb 11, 2024

There are some very insightful ideas here, particularly around the style/theme-ing. I've been working on the config/theme file, which required a pretty huge refactor in this area, so I think it would be a good time to implement some of these ideas.

Excuse me if this sounds like a stupid idea, as I haven't gotten much sleep. But if the check for an executable file is seemingly what is responsible for fetching the full metadata when we otherwise wouldn't be, if the majority of other files are styled based off their extensions, could we simply apply some base style to the files, and only style executable files by their permission, which we would have anyway as it's part of the --long output.

EDIT: I understand this would indeed change the current behavior, but on windows anyway, they could be styled by file extension instead, although I have no clue about windows in general 😅

@tertsdiepraam
Copy link
Contributor Author

@PThorpe92 I never responded, how rude of me!

Yes, with this change, there will be a single statx call for the styling and the permissions if -l is used. The case I'm describing here is when -l is not used and therefore the statx call is only used for styling.

I'm still unsure what would be a good solution here (maybe there is none except providing themes that would be faster/slower depending on what the user wants).

@yonas
Copy link

yonas commented Sep 1, 2024

How close are we to merging this?

@tertsdiepraam
Copy link
Contributor Author

I'm so confused about what git has done 😄

@tertsdiepraam tertsdiepraam force-pushed the fewer-statx-calls branch 7 times, most recently from 1fe7310 to f43124b Compare October 11, 2024 08:00
@tertsdiepraam
Copy link
Contributor Author

Here are some benchmarks using hyperfine:

[nix-shell:~/code/eza]$ hyperfine "{cmd} -1 /nix/store | wc -l" --parameter-list cmd ls,./eza-main,./eza-new
Benchmark 1: ls -1 /nix/store | wc -l
  Time (mean ± σ):      1.045 s ±  0.297 s    [User: 0.781 s, System: 0.272 s]
  Range (min … max):    0.528 s …  1.289 s    10 runs
 
Benchmark 2: ./eza-main -1 /nix/store | wc -l
  Time (mean ± σ):      1.540 s ±  0.005 s    [User: 0.717 s, System: 0.997 s]
  Range (min … max):    1.531 s …  1.546 s    10 runs
 
Benchmark 3: ./eza-new -1 /nix/store | wc -l
  Time (mean ± σ):      1.057 s ±  0.005 s    [User: 0.676 s, System: 0.556 s]
  Range (min … max):    1.049 s …  1.063 s    10 runs
 
Summary
  ls -1 /nix/store | wc -l ran
    1.01 ± 0.29 times faster than ./eza-new -1 /nix/store | wc -l
    1.47 ± 0.42 times faster than ./eza-main -1 /nix/store | wc -l
[nix-shell:~/code/eza]$ hyperfine "{cmd} -l /nix/store | wc -l" --parameter-list cmd ls,./eza-main,./eza-new
Benchmark 1: ls -l /nix/store | wc -l
  Time (mean ± σ):      1.808 s ±  0.856 s    [User: 0.726 s, System: 1.094 s]
  Range (min … max):    1.278 s …  3.314 s    10 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (3.314 s). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.
 
Benchmark 2: ./eza-main -l /nix/store | wc -l
  Time (mean ± σ):      2.295 s ±  0.023 s    [User: 1.694 s, System: 2.508 s]
  Range (min … max):    2.257 s …  2.328 s    10 runs
 
Benchmark 3: ./eza-new -l /nix/store | wc -l
  Time (mean ± σ):      1.873 s ±  0.028 s    [User: 1.702 s, System: 2.434 s]
  Range (min … max):    1.813 s …  1.905 s    10 runs
 
Summary
  ls -l /nix/store | wc -l ran
    1.04 ± 0.49 times faster than ./eza-new -l /nix/store | wc -l
    1.27 ± 0.60 times faster than ./eza-main -l /nix/store | wc -l

@tertsdiepraam tertsdiepraam marked this pull request as ready for review October 11, 2024 11:16
@cafkafk cafkafk self-requested a review October 11, 2024 14:10
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I just got done reviewing the code, and it looks good to me.

The ~23% speed increase is very appreciated :D

@cafkafk cafkafk merged commit d1df86e into eza-community:main Oct 11, 2024
20 checks passed
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