-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
macros: join! start by polling a different future each time poll_fn is polled #4624
Conversation
ea44be5
to
55fc346
Compare
3a000b1
to
ebc452c
Compare
tokio/src/macros/join.rs
Outdated
// How many futures were passed to join!. | ||
const FUTURE_COUNT: u32 = $crate::count!( $($count)* ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the join!
macro previously worked with more than 64 branches, then this will break any such code. Please double check whether the old version had a branch limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can pass 125 branches to the old version without changing the recursion limit 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to make the new join!
version accept 125 branches too and kept hitting the macro recursion limit. I think it its because of the way the join!
input is normalized.
// ===== Normalize =====
(@ { ( $($s:tt)* ) $($t:tt)* } $e:expr, $($r:tt)* ) => {
$crate::join!(@{ ($($s)* _) $($t)* ($($s)*) $e, } $($r)*)
};
// ===== Entry point =====
( $($e:expr),* $(,)?) => {
$crate::join!(@{ () } $($e,)*)
};
I implemented join!
as a proc macro, it was easier to avoid the recursion limit this way. Would a proc macro be ok? (we are accepting way more branches at the moment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to avoid a procedural macro if possible. In general, I recommend you ask about an implementation strategy first before implementing it, especially if it's a larger change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend you ask about an implementation strategy first before implementing it, especially if it's a larger change
I think i read that in the CONTRIBUTING.md file, i will make sure to ask first next time. I chose to implement a proc macro without asking because i can use the practice if it is not accepted.
I went back to the version using just a declarative macro and was playing around with it the last couple of days but i'm pretty much stuck at the moment.
I keep hitting the recursion limit when i pass 125 futures to join!
(the maximum supported without changing the recursion limit at the moment) because after join!
normalizes its input, it is 126 levels deep in macro recursion and i need to call a macro (count!
) once again.
Any ideas i could try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about having the normalization count the number of branches?
// One `_` for each branch in the `join!` macro. This is not used once
// normalization is complete.
( $($count:tt)* )
+ // The expression `0+1+1+ ... +1` equal to the number of branches.
+ ( $($total:tt)* )
// Normalized join! branches
$( ( $($skip:tt)* ) $e:expr, )*
// ===== Normalize =====
- (@ { ( $($s:tt)* ) $($t:tt)* } $e:expr, $($r:tt)* ) => {
- $crate::join!(@{ ($($s)* _) $($t)* ($($s)*) $e, } $($r)*)
+ (@ { ( $($s:tt)* ) ( $($n:tt)* ) $($t:tt)* } $e:expr, $($r:tt)* ) => {
+ $crate::join!(@{ ($($s)* _) ($($n)* + 1) $($t)* ($($s)*) $e, } $($r)*)
};
// ===== Entry point =====
( $($e:expr),* $(,)?) => {
- $crate::join!(@{ () } $($e,)*)
+ $crate::join!(@{ () (0) } $($e,)*)
};
}
Then you could use the count in this way:
// Safety: nothing must be moved out of `futures`. This is to satisfy
// the requirement of `Pin::new_unchecked` called below.
let mut futures = ( $( maybe_done($e), )* );
let mut skip_next_time: u32 = 0;
poll_fn(move |cx| {
const COUNT: u32 = $($total)*;
let mut is_pending = false;
let mut to_run = COUNT;
let mut skip = skip_next_time;
skip_next_time = if skip+1 == COUNT { 0 } else { skip+1 };
loop {
$(
if skip == 0 {
if to_run == 0 {
break;
}
to_run -= 1;
// Extract the future for this branch from the tuple.
let ( $($skip,)* fut, .. ) = &mut futures;
// Safety: future is stored on the stack above
// and never moved.
let mut fut = unsafe { Pin::new_unchecked(fut) };
// Try polling
if fut.poll(cx).is_pending() {
is_pending = true;
}
} else {
skip -= 1;
}
)*
}
...
}).await
Also, I think the same issue exists for the try_join!
macro, so if the above works, it would be good to fix that one too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works 👍
I should have thought about that 🤦
7c832ae
to
6e8f985
Compare
6e8f985
to
895a24e
Compare
tokio/src/macros/try_join.rs
Outdated
for _ in 0..FUTURE_COUNT { | ||
$( | ||
{ | ||
const INDEX: u32 = $($branch_index)*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually poll every future every time as far as I can tell, like the loop
approach I suggested does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could also add a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test, i think the futures are getting polled in the expected order. Let me know if i am missing something.
I can switch to the approach you suggested if you prefer.
I added the cargo expand output to the PR description if you want to take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I misread it. Anyway, the current loop is quite wasteful because you go through every future with a smaller index every time you poll something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
tokio/tests/macros_join.rs
Outdated
let mut how_many_times_i_got_to_run = 0; | ||
|
||
for _ in 0..5 { | ||
tokio::time::sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to use time in tests, then you need to use Tokio's pause-time feature. Having a test that takes 100ms to run is way too long. Our test suite would take forever if we had many tests like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopped using tokio::time
96a7164
to
a7611c6
Compare
a7611c6
to
da8da00
Compare
In general, I think this looks quite good! Are you able to add a comment on the skip variables that explain why they exist? (in both the |
da8da00
to
269ade6
Compare
There was a problem hiding this 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.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.18.2` -> `1.19.1` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.18.2` -> `1.19.1` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.19.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.1) [Compare Source](tokio-rs/tokio@tokio-1.19.0...tokio-1.19.1) ##### 1.19.1 (June 5, 2022) This release fixes a bug in `Notified::enable`. ([#​4747]) [#​4747]: tokio-rs/tokio#4747 ### [`v1.19.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.0) [Compare Source](tokio-rs/tokio@tokio-1.18.2...tokio-1.19.0) ##### 1.19.0 (June 3, 2022) ##### Added - runtime: add `is_finished` method for `JoinHandle` and `AbortHandle` ([#​4709]) - runtime: make global queue and event polling intervals configurable ([#​4671]) - sync: add `Notified::enable` ([#​4705]) - sync: add `watch::Sender::send_if_modified` ([#​4591]) - sync: add resubscribe method to broadcast::Receiver ([#​4607]) - net: add `take_error` to `TcpSocket` and `TcpStream` ([#​4739]) ##### Changed - io: refactor out usage of Weak in the io handle ([#​4656]) ##### Fixed - macros: avoid starvation in `join!` and `try_join!` ([#​4624]) ##### Documented - runtime: clarify semantics of tasks outliving `block_on` ([#​4729]) - time: fix example for `MissedTickBehavior::Burst` ([#​4713]) ##### Unstable - metrics: correctly update atomics in `IoDriverMetrics` ([#​4725]) - metrics: fix compilation with unstable, process, and rt, but without net ([#​4682]) - task: add `#[track_caller]` to `JoinSet`/`JoinMap` ([#​4697]) - task: add `Builder::{spawn_on, spawn_local_on, spawn_blocking_on}` ([#​4683]) - task: add `consume_budget` for cooperative scheduling ([#​4498]) - task: add `join_set::Builder` for configuring `JoinSet` tasks ([#​4687]) - task: update return value of `JoinSet::join_one` ([#​4726]) [#​4498]: tokio-rs/tokio#4498 [#​4591]: tokio-rs/tokio#4591 [#​4607]: tokio-rs/tokio#4607 [#​4624]: tokio-rs/tokio#4624 [#​4656]: tokio-rs/tokio#4656 [#​4671]: tokio-rs/tokio#4671 [#​4682]: tokio-rs/tokio#4682 [#​4683]: tokio-rs/tokio#4683 [#​4687]: tokio-rs/tokio#4687 [#​4697]: tokio-rs/tokio#4697 [#​4705]: tokio-rs/tokio#4705 [#​4709]: tokio-rs/tokio#4709 [#​4713]: tokio-rs/tokio#4713 [#​4725]: tokio-rs/tokio#4725 [#​4726]: tokio-rs/tokio#4726 [#​4729]: tokio-rs/tokio#4729 [#​4739]: tokio-rs/tokio#4739 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1394 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Motivation
Fixes: #4612
Since
tokio::join!
runs the futures in the same task, the futures were are the same budget which makes it possible for one future to consume the whole budget leaving nothing for the other futures to consume.Starvation example taken from #4612
Solution
Start by polling a different future each time the future generated by
poll_ fn
is polled. This way, each future will get the chance to make progress even if there are futures that consume the whole budget.Given futures
A
,B
andC
passsed tojoin!
The polling will look like this:
The first time the future generated by
poll_fn
is polled:The second time:
The third time:
The fourth time (we are back at the start):
Cargo expand example