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

time: small implementation cleanups #6517

Merged
merged 11 commits into from May 1, 2024

Conversation

paolobarbolini
Copy link
Contributor

Motivation

While reading how the timer driver works, I found several things that could be cleaned up.

Solution

I've included those fixes in this PR.

`StateCell::mark_pending` already uses a loop to retry the
compare exchange operation if it fails.

The logic within the loop would handle a spurious failure
like any other failure, so `compare_exchange_weak` makes
the implementation more efficient for some platforms.
This replaces a manual implementation of
`Instant::saturating_duration_since` with the actual one.

Clippy already catches this for primitive integer types via the
`manual_saturating_arithmetic` lint, as demonstrated by the following
example:

```rs
fn saturating_sub(val: usize, n: usize) -> usize {
    val.checked_sub(n).unwrap_or(0)
}
```
As explained by the comment removed by this commit earlier Rust
versions didn't have an ergonomic way of building arrays of any
size of non-Copy types. Rust 1.63 stabilized `array::from_fn`,
fixing the above issue and by making it easy to construct arrays
from a constructor.
@github-actions github-actions bot added the R-loom-time-driver Run loom time driver tests on this PR label Apr 25, 2024
This replaces `Wheel::levels`, currently represented as a
`Vec` of 6 elements, into an array.

This should help eliminate some bounds checks from accesses
to `levels`.

This changes the size of `Wheel` from 48 bytes to 32 bytes
on x86_64. The initial implementation didn't Box the array,
resulting into `Wheel` having a size of 6264 bytes and loom
tests overflowing their stack.
Replaces the manual implementation of a waker list with `WakeList`.
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Apr 26, 2024
@Darksonn
Copy link
Contributor

cc @wathenjiang

@wathenjiang
Copy link
Contributor

wathenjiang commented Apr 28, 2024

All LGTM!

However, it should be pointed out that in the current implementation, the behavior of waker_list: [Option<Waker>; 32] is FIFO. However, for crate::util::WakeList, it is LIFO.

I think we should ensure that the order of this FIFO remains unchanged.

@paolobarbolini
Copy link
Contributor Author

However, it should be pointed out that in the current implementation, the behavior of waker_list: [Option<Waker>; 32] is FIFO. However, for crate::util::WakeList, it is LIFO.

I think we should ensure that the order of this FIFO remains unchanged.

Good catch! I've opened a separate PR to fix it #6521

@Darksonn
Copy link
Contributor

Order of wakers is generally not something we guarantee, but I think having WakeList become FIFO is reasonable.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Darksonn Darksonn merged commit 28439e2 into tokio-rs:master May 1, 2024
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-time Module: tokio/time R-loom-time-driver Run loom time driver tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants