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: Avoid unnecessary call to GetFileInformationByHandleEx #6841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jesseschalken
Copy link
Contributor

@jesseschalken jesseschalken commented Nov 3, 2024

We don't need to call GetFileInformationByHandleEx to track blocks if --apparent-size was passed.

This removes blocks from Stat and instead moves the handling of --apparent-size into Stat::new so it can avoid calling GetFileInformationByHandleEx if necessary.

~1.65x speed up on my test folder.

Before

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

After

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

@jesseschalken jesseschalken changed the title Avoid unnecessary call to GetFileInformationByHandleEx du: Avoid unnecessary call to GetFileInformationByHandleEx Nov 3, 2024
@jesseschalken jesseschalken force-pushed the avoid-unnecessary-get_size_on_disk branch from 9baeb9c to ef082f2 Compare November 3, 2024 12:59
@jesseschalken jesseschalken marked this pull request as ready for review November 3, 2024 13:32
@jesseschalken jesseschalken force-pushed the avoid-unnecessary-get_size_on_disk branch 2 times, most recently from cbc106b to cacc8c5 Compare November 3, 2024 14:03
Copy link

github-actions bot commented Nov 3, 2024

GNU testsuite comparison:

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

let file_info = get_file_info(path);

Ok(Self {
path: path.to_path_buf(),
is_dir: metadata.is_dir(),
size: if path.is_dir() { 0 } else { metadata.len() },
blocks: size_on_disk / 1024 * 2,
size: if options.apparent_size {
Copy link
Contributor

Choose a reason for hiding this comment

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

almost duplicate code, maybe move it above the windows / not windows

@sylvestre sylvestre force-pushed the avoid-unnecessary-get_size_on_disk branch from cacc8c5 to f77bb24 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)

@sylvestre
Copy link
Contributor

some tests fails:

--- TRY 3 STDOUT:        coreutils::tests test_du::test_du_basics ---

running 1 test
run: D:\a\coreutils\coreutils\target\debug\coreutils.exe du
test test_du::test_du_basics ... FAILED

failures:

failures:
    test_du::test_du_basics

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2806 filtered out; finished in 0.17s


--- TRY 3 STDERR:        coreutils::tests test_du::test_du_basics ---
thread 'test_du::test_du_basics' panicked at tests\by-util\test_du.rs:63:5:
assertion `left == right` failed
  left: "1\t.\\subdir\\deeper\\deeper_dir\n1\t.\\subdir\\deeper\n9\t.\\subdir\\links\n9\t.\\subdir\n9\t.\n"
 right: "0\t.\\subdir\\deeper\\deeper_dir\n0\t.\\subdir\\deeper\n8\t.\\subdir\\links\n8\t.\\subdir\n8\t.\n"
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library/std\src\panicking.rs:662
   1: core::panicking::panic_fmt
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library/core\src\panicking.rs:74
   2: core::panicking::assert_failed_inner
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library/core\src\panicking.rs:412
   3: core::panicking::assert_failed<ref$<str$>,ref$<str$> >
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library\core\src\panicking.rs:367
   4: tests::test_du::_du_basics
             at .\tests\by-util\test_du.rs:63
   5: tests::test_du::test_du_basics
             at .\tests\by-util\test_du.rs:39
   6: tests::test_du::test_du_basics::closure$0
             at .\tests\by-util\test_du.rs:26
   7: core::ops::function::FnOnce::call_once<tests::test_du::test_du_basics::closure_env$0,tuple$<> >
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library\core\src\ops\function.rs:250
   8: core::ops::function::FnOnce::call_once
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14\library/core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

  TRY 3 FAIL [   0.188s] coreutils::tests test_du::test_du_basics_subdir

@sylvestre
Copy link
Contributor

ping ?

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