timeout: Fix performance regression using sigtimedwait #9173
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?
CodSpeed Performance ReportMerging #9173 will not alter performanceComparing Summary
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: |
Resolves uutils#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: - Block SIGCHLD/SIGTERM before spawning child - Use sigtimedwait() for efficient signal-based waiting - Properly save and restore signal mask on all exit paths - Handle EINTR correctly with timeout recalculation Performance improvement: - Before: 106.9ms ± 2.0ms - After: 1.2ms ± 0.1ms - GNU: 1.1ms ± 0.1ms Tested with 100+ iterations, maintains 100% reliability.
… SIGTERM (6 failing tests fixed)
Replace io::Error::new(io::ErrorKind::Other, ...) with the more concise io::Error::other() helper method in kqueue implementations. Fixes clippy::io_other_error warnings on macOS builds.
…ersion to seconds overflows. Instead of failing, cap the duration at time_t::MAX seconds, which is the maximum valid timeout value.
…d overflow errors. If value exceeds safe limits, cap at time_t::MAX seconds instead of failing.
…nite) for kqueue, cap at i32::MAX/2 for sigtimedwait
…e_time, preventing the EINVAL error by capping huge values at i64::MAX seconds instead of letting them overflow during string-to-Duration conversion
…Now the function only intercepts the specific case we're trying to fix
quantum-encoding
force-pushed
the
fix-timeout-performance
branch
from
2087021 to
e8531fa
Compare
… is a huge overflow case, then cap it
|
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 |
3 participants
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.