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

Fix user and kernel times on Windows #538

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

clemenswasser
Copy link
Contributor

@clemenswasser clemenswasser commented Aug 8, 2022

Description

By using a job object and attaching it to the spawned process before launch, we can record the total user and kernel times of it and all of its child processes. Sadly extracting the raw handle of the main_thread_handle is still nightly only 😢 (see: rust-lang/rust#96723) and required, in order to resume a initially suspended process. Therefore this new CPUTimer implementation is guarded behind a new nightly cargo feature and not enabled by default.

Example

Before:

> cargo run --quiet -- 'cargo fmt'
Benchmark 1: cargo fmt
  Time (mean ± σ):     175.6 ms ±   1.1 ms    [User: 0.9 ms, System: 8.4 ms]
  Range (min … max):   173.3 ms … 177.3 ms    16 runs

After:

> cargo run --quiet -- 'cargo fmt'
Benchmark 1: cargo fmt
  Time (mean ± σ):     175.8 ms ±   1.9 ms    [User: 91.5 ms, System: 62.5 ms]
  Range (min … max):   173.5 ms … 180.7 ms    16 runs

Fixes: #368

@clemenswasser clemenswasser force-pushed the fix-windows-timer branch 3 times, most recently from 97116dc to 8354ef6 Compare August 8, 2022 15:47
@sharkdp
Copy link
Owner

sharkdp commented Aug 15, 2022

Thank you very much for your contribution and for the clear implementation & documentation!

Sadly extracting the raw handle of the main_thread_handle is still nightly only cry (see: rust-lang/rust#96723) and required, in order to resume a initially suspended process.

Just to make sure: the linked ticket says "Finding that thread from the process handle can be a bit convoluted", which sounds like it is actually possible to retrieve the handle without that nightly-only feature (potentially using some low-level winapi code). I would appreciate if we could quickly look into that. It would be fantastic if we could make this work on stable Rust.

Therefore this new CPUTimer implementation is guarded behind a new nightly cargo feature and not enabled by default.

Thank you. I think I understand the intention behind the feature gate. On the other hand, it's not like the current CPUTimer implementation is really a reasonable alternative that someone would like to choose. It's just broken.

In this sense, I think it would be great if the absence of the new feature gate would completely disable the user/kernel time feature on Windows. What do you think?

Minor point: calling this feature nightly is a bit confusing IMO. How about using a more descriptive name (e.g. windows_user_and_system_times).

@clemenswasser
Copy link
Contributor Author

@sharkdp thanks for your comments. I'll look into them tomorrow. I already found a undocumented ntdll function NtResumeProcess, which resumes a process by HANDLE, which we could potentially use and before that I played around with a implementation using CreateToolhelp32Snapshot for finding the main thread, but using NtResumeProcess should definitely be better in terms of performance and code size 😁.

@clemenswasser
Copy link
Contributor Author

clemenswasser commented Aug 16, 2022

With the latest commit it works on stable using the undocumented but well known ntdll.dll!NtResumeProcess which exists since Windows XP. When using nightly the feature windows_process_extensions_main_thread_handle can be set to use the unstable feature with the same name. The dependency on once_init is not a problem, since clap was already using it anyway.
Also I don't know why the ActiveProcesses == 0 assertion triggered on https://github.com/sharkdp/hyperfine/runs/7864408316...
For me locally this never triggered for simple test cases like cargo run -- 'cargo fmt'

@clemenswasser
Copy link
Contributor Author

@sharkdp ping. Can I do something to help merge this?

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

No - this is great! Thank you very much.

@sharkdp sharkdp merged commit ac8c114 into sharkdp:master Sep 2, 2022
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.

Windows user & system times incorrect
2 participants