timeout: Fix performance regression using sigtimedwait#9173
timeout: Fix performance regression using sigtimedwait#9173quantum-encoding wants to merge 1 commit intouutils:mainfrom
Conversation
|
GNU testsuite comparison: |
|
6 Failing Tests, three issues, three fixes: 1. Signal Mask Inheritance (main issue)File: Added a .pre_exec(|| {
// Unblock signals that were blocked in parent for sigtimedwait
let mut unblock_set = SigSet::empty();
unblock_set.add(Signal::SIGTERM);
unblock_set.add(Signal::SIGCHLD);
sigprocmask(SigmaskHow::SIG_UNBLOCK, Some(&unblock_set), None)
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?;
Ok(())
})Also added the import: 2. Process Group SignalingFile: Fixed // Changed from: kill(0, signal)
// To: kill(-getpid(), signal)
if unsafe { libc::kill(-libc::getpid(), signal as i32) } == 0 {Test Results All 6 previously failing tests now pass:
|
|
GNU testsuite comparison: |
| let mut siginfo: libc::siginfo_t = std::mem::zeroed(); | ||
| let ret = libc::sigtimedwait( | ||
| &sigset.as_ref() as *const _ as *const libc::sigset_t, | ||
| &mut siginfo as *mut libc::siginfo_t, |
There was a problem hiding this comment.
Why setting siginfo to something that is non-null if it's not used anywhere?
Merging this PR will not alter performance
Comparing Footnotes
|
|
GNU testsuite comparison: |
|
Please provide the full output of hyperfine with the three commands at once |
|
Benchmark 1: /tmp/timeout_old 10 sleep 0.01 Benchmark 2: /tmp/timeout_new 10 sleep 0.01 Benchmark 3: timeout 10 sleep 0.01 Summary |
|
Benchmark 1: /tmp/timeout_old 10 true Benchmark 2: /tmp/timeout_new 10 true Benchmark 3: /usr/bin/timeout 10 true Summary |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
2 similar comments
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
1 similar comment
|
GNU testsuite comparison: |
2087021 to
e8531fa
Compare
|
GNU testsuite comparison: |
|
good morning, i see the failing tests all passed last night on the final CI run. I'm available to help with any other pressing issues, at the maintainers discretion. The Performance Fix The Overflow Bug The fix evolved through several iterations:
The Final Solution: After Rebase Drama |
|
sorry, could you please rebase it ? |
be95cc5 to
21612a2
Compare
| @@ -0,0 +1,27 @@ | |||
| name: Test Timeout on macOS | |||
There was a problem hiding this comment.
i don't think we want a new workflow just for this, sorry
|
i am sorry but it is a lot of added complexity here ... |
…gtimedwait(). macOS: kqueue with EVFILT_SIGNAL. Fixed signal mask inheritance for child processes. Handle overflow for extremely large timeout values
21612a2 to
d7c9ee8
Compare
Resolves #9099
The timeout implementation was using a 100ms polling loop, causing significant performance overhead (~100ms vs GNU's 1.1ms).
This commit replaces the polling approach with POSIX sigtimedwait(), which suspends the process until SIGCHLD or timeout occurs, eliminating the busy-wait overhead.
Key changes:
Performance improvement:
Tested with 100+ iterations, maintains 100% reliability.
Approach
This implementation follows the same general approach as GNU timeout:
timer_create()+sigsuspend()sigtimedwait()with timeout parameterRelated
Similar to approach in PR #9123 by @andreacorbellini, but with careful signal mask restoration to avoid state leakage.