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

Round up when converting timeout to ms for epoll_wait() #1615

Merged
merged 3 commits into from
Sep 3, 2022

Conversation

yump
Copy link
Contributor

@yump yump commented Sep 3, 2022

Fixes #1614

I'm a total beginner to rust, so there may be a better way to do this. I'm also unfamiliar with async I/O, so it's entirely possible that crossterm is misusing the API. But the epoll_wait manpage does say that the timeout will be rounded up.

Comment on lines 90 to 101
// as_millis() truncates, so round up to the nearest millisecond as the documentation
// says can happen. This avoids turning submillisecond timeouts into immediate returns
// unless the caller explicitly requests that by specifying a zero timeout.
.map(|to| {
to.as_millis()
+ if to.subsec_nanos() % 1_000_000 == 0 {
0
} else {
1
}
})
.map(|to| cmp::min(to, MAX_SAFE_TIMEOUT) as libc::c_int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the the remainder, we can just get the subsec_nanos directly (just a read of a field). Also let's put into a single map closure. Something like the following should would

Suggested change
// as_millis() truncates, so round up to the nearest millisecond as the documentation
// says can happen. This avoids turning submillisecond timeouts into immediate returns
// unless the caller explicitly requests that by specifying a zero timeout.
.map(|to| {
to.as_millis()
+ if to.subsec_nanos() % 1_000_000 == 0 {
0
} else {
1
}
})
.map(|to| cmp::min(to, MAX_SAFE_TIMEOUT) as libc::c_int)
.map(|to| cmp::min(
to.as_millis()
// as_millis() truncates, so round up to the nearest millisecond as the documentation
// says can happen. This avoids turning submillisecond timeouts into immediate returns
// unless the caller explicitly requests that by specifying a zero timeout.
+ if to.subsec_nanos() == 0 { 0 } else { 1 }
, MAX_SAFE_TIMEOUT) as libc::c_int)

This will need some manual formatting of the code and the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Closures merged and re-rustfmt'ed.

The remainder is not needed for timeouts between 0 and 1 ms, but without it one extra ms would be added to any exact-integer-ms timeout. The biggest relative error is that a 1ms timeout gets doubled to 2ms. If the timeouts callers specify are a smooth random variable that's no problem, but humans tend to pick nice round numbers.

The docs do say overrun is possible, and I recognize the desire to avoid a divide instruction, however the syscall a few lines later is probably a lot more expensive, especially on a CPU requiring spectre mitigations.

But it's your API and I have no reason to fight about it, so the remainder can go if you still want it gone.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, good catch. Let's do something like the following (will need formatting again):

            .map(|to| {
                let to_ms = to.as_millis();
                // as_millis() truncates, so round up to the nearest millisecond as the
                // documentation says can happen. This avoids turning submillisecond
                // timeouts into immediate returns unless the caller explicitly requests
                // that by specifying a zero timeout.
                let to_ms = to_ms + if to_ms == 0 && to.subsec_nanos() != 0 { 1 } else { 0 };
                cmp::min(MAX_SAFE_TIMEOUT, to_ms) as libc::c_int 
            })

Don't need the remainder and we only do it if the timeout is less than 1 millisecond, but not zero.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Any chance you want to apply the same patch to Windows? Related code:

mio/src/sys/windows/iocp.rs

Lines 225 to 231 in c6b5f13

fn duration_millis(dur: Option<Duration>) -> u32 {
if let Some(dur) = dur {
std::cmp::min(dur.as_millis(), u32::MAX as u128) as u32
} else {
u32::MAX
}
}

If not it's no problem.

@yump
Copy link
Contributor Author

yump commented Sep 3, 2022

Any chance you want to apply the same patch to Windows? Related code:

mio/src/sys/windows/iocp.rs

Lines 225 to 231 in c6b5f13

fn duration_millis(dur: Option<Duration>) -> u32 {
if let Some(dur) = dur {
std::cmp::min(dur.as_millis(), u32::MAX as u128) as u32
} else {
u32::MAX
}
}

If not it's no problem.

Same patch applied to Windows, on another branch. I don't have any Windows machines, but cargo test --all-features passes under Linux.

@Thomasdezeeuw Thomasdezeeuw merged commit a07049d into tokio-rs:master Sep 3, 2022
@Thomasdezeeuw
Copy link
Collaborator

Thanks @yump. If you want to create another pr with the Windows change our CI will test it. You can check it locally using make check_all_target (might want to change the $TARGETS to just Windows) using cargo check.

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.

poll() returns immdiately with timeouts less than one millisecond
2 participants