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

Refactor async task spawns #2271

Closed
wants to merge 6 commits into from
Closed

Conversation

creativcoder
Copy link
Contributor

@creativcoder creativcoder commented Nov 24, 2022

Summary of changes
Changes introduced in this pull request:

  • Refactors async tasks spawns based on feedback from tokio-console
  • Replace FutureUnordered with JoinSet

Reference issue to close (if applicable)

Closes

Other information and links

@lemmih
Copy link
Contributor

lemmih commented Nov 25, 2022

This clashes with the changes in #2269, right?

@creativcoder
Copy link
Contributor Author

Noticed it now, can we have async task changes in this PR?, i thought that was supposed to just tweak tokio-console

@hanabi1224
Copy link
Contributor

Sure I can remove the JoinSet change from the other PR

@creativcoder creativcoder force-pushed the creativcoder/tokio-console-opt branch from 8e33b5b to 0482c02 Compare November 28, 2022 14:30
validations.push(validation_fn);
let consensus = consensus.clone();
let state_manager = state_manager.clone();
let validation_fn = tokio::task::spawn_blocking(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to move on a spawn_blocking here? validate_block is an async function, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tokio-console shows warning when using spawn with a "⚠ n tasks have lost their waker" at this line. Replacing spawn with spawn_blocking makes the warns go away.

Update: I no longer receive those warns even with spawn after repeated runs with tokio-console, so I'm thinking of reverting this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the real root cause of this "have lost their wake" warning. Some other people are complaining of it too, see tokio-rs/console#149.

@creativcoder creativcoder force-pushed the creativcoder/tokio-console-opt branch 2 times, most recently from fdd4ea4 to 798f945 Compare December 1, 2022 12:38
@creativcoder creativcoder marked this pull request as ready for review December 1, 2022 13:04
@creativcoder creativcoder force-pushed the creativcoder/tokio-console-opt branch from c51b7cd to c63b9c3 Compare December 1, 2022 13:06
@@ -1163,16 +1163,16 @@ async fn validate_tipset<DB: Blockstore + Store + Clone + Send + Sync + 'static,
debug!("Tipset keys: {:?}", full_tipset_key.cids);

for b in blocks {
let validation_fn = tokio::task::spawn(validate_block::<_, C>(
let validation_fn = tokio::task::spawn(validate_block(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you're spawning things twice?

Suggested change
let validation_fn = tokio::task::spawn(validate_block(
let validation_fn = validate_block(


// Base fee check
let smoke_height = state_manager.chain_config().epoch(Height::Smoke);
let v_base_tipset = Arc::clone(&base_tipset);
let v_block_store = state_manager.blockstore().clone();
let v_block = Arc::clone(&block);
validations.push(tokio::task::spawn_blocking(move || {
let base_fee_check = tokio::task::spawn_blocking(move || {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too fond of this pattern with the double spawns. A small helper function for waiting on blocking spawns might be prettier. Or we could create our own structure for having collections of validators. That way, collect_errs wouldn't be a free-standing function.

@LesnyRumcajs LesnyRumcajs deleted the creativcoder/tokio-console-opt branch April 25, 2023 09:55
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.

4 participants