Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pass an executor through the Configuration #4688

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jan 20, 2020

Hopefully fixes the performance issues that Polkadot was having.

One issue that we have in master right now is that all the tasks go through this path:

In other words, everything is single-threaded (except the networking).

In this PR, we now pass a "tasks executor" through the Configuration struct.
It is set up to be a tokio runtime, or the spawn_local function of wasm_bindgen_futures, or a threads pool by default, or local polling if all else fails EDIT: removed fallbacks after review.

Important: this executor is stored within the Service, and all the components of Substrate should then use the Service (through spawn_task or spawn_task_handle or spawn_essential_task) to create tasks.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Jan 20, 2020
@tomaka tomaka requested review from bkchr and expenses January 20, 2020 15:19
err
);
this.to_poll.push(Box::pin(err.into_future().compat().map(drop)));
if let Some(tasks_executor) = this.tasks_executor.as_ref() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this logic? The builder creates a thread builder anyway and browser gives us a spawner as well. This sounds like we can remove the option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user leaves None in the Configuration, we try to create a ThreadPool and this is the path that is reached if spawning the threads failed.

In practice, without this code we will get a panic in the browser if the user forgets to tweak the configuration. I guess it's indeed debatable whether it's worth the additional code.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should go the way of checking at Service creation that a task_executor is provided and otherwise an error is returned. I know that is not the best solution, but IMHO the best we can do for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @bkchr.

@tomaka
Copy link
Contributor Author

tomaka commented Jan 20, 2020

I'm trying of create a version of Polkadot using this branch to see if it indeed fixes the performance issue.
(that's not a blocker for merging though, I think this change is beneficial nonetheless)

Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

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

LGTM apart from that one discussion.

@tomaka
Copy link
Contributor Author

tomaka commented Jan 21, 2020

We deployed this PR to both a Kusama validator and a Kusama sentry node, and CPU usage seems to be mostly the same if not lower than version 0.7.18 (that includes the revert to libp2p 0.13).

@bkchr
Copy link
Member

bkchr commented Jan 21, 2020

@tomaka service tests needs to be updated.

@bkchr bkchr merged commit 6ee1244 into paritytech:master Jan 21, 2020
@tomaka tomaka deleted the probably-fix-perfs branch January 21, 2020 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants