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

du: Reuse existing metadata instead of calling path.is_dir() again #6840

Merged

Conversation

jesseschalken
Copy link
Contributor

@jesseschalken jesseschalken commented Nov 3, 2024

Path::is_dir does another call to stat but we can reuse the metadata we already have.

Also Path::is_dir traverses symbolic links which is only right if -L was passed.

~1.13x speedup on my test directory.

Before

> cargo build --release
> measure-command { .\target\release\coreutils.exe du --apparent-size "test folder" }
TotalSeconds      : 11.0101422

After

> cargo build --release
> measure-command { .\target\release\coreutils.exe du --apparent-size "test folder" }
TotalSeconds      : 9.7109226

@jesseschalken jesseschalken changed the title Reuse existing metadata instead of calling path.is_dir() again du: Reuse existing metadata instead of calling path.is_dir() again Nov 3, 2024
@jesseschalken jesseschalken marked this pull request as ready for review November 3, 2024 14:21
@sylvestre
Copy link
Contributor

nice
could you please try to benchmark with hyperfine ? and document your work in src/uu/du/BENCHMARKING.md ?

see other commands for example

@sylvestre
Copy link
Contributor

and i wonder if we could automate this check to make sure we don't regress :)

@sylvestre sylvestre force-pushed the reuse-metadata-instead-of-path-is-dir branch from 9ec85b5 to 99a5f2c Compare November 11, 2024 11:48
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre
Copy link
Contributor

ping ?

@sylvestre sylvestre force-pushed the reuse-metadata-instead-of-path-is-dir branch from 99a5f2c to 8a69922 Compare November 28, 2024 16:50
@sylvestre
Copy link
Contributor

any update? thanks

@sylvestre sylvestre force-pushed the reuse-metadata-instead-of-path-is-dir branch from 8a69922 to 6487347 Compare December 2, 2024 09:01
Copy link

github-actions bot commented Dec 2, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre
Copy link
Contributor

i will document it at some point

@sylvestre sylvestre merged commit 763306c into uutils:main Dec 2, 2024
62 checks passed
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.

2 participants