Task executor extension for runtime host #5249

NikVolf · 2020-03-13T12:39:39Z

This will essentially allow spawning tasks when handling host functions, using the main scheduler.

It is immediately needed for #5023, also for variant of #5023 for block production

And is generally useful in a sense that it will allow parallelise runtime execution (but should be used carefully and join spawned tasks deterministically - one-by-one / or in well-defined order if multiple are joined)

example: see Batch signature verification #5023

bkchr · 2020-03-13T12:46:12Z

primitives/externalities/src/lib.rs

+	/// Return handle that can spawn parallel tasks.
+	///
+	/// Can be none if host does not support parallel tasks.
+	fn spawn_handle(&self) -> Option<&dyn ClonableSpawn>;


I really don't want to add anymore functionality to Externalities. This was the reason Extension's where introduced. I know that extension handling is currently a bit shitty, but please make this an extension.

Fine with me as Extension as well, but just to note and as discussed: this Externalities ability will be quite essential (modulo test implementations of it), while Extension-s are optional and generally not required for code execution.

Ok after some more disussion with @kianenigma there is a strong indication that this functionality will need to be rather configurable, and Extension will definitely help to achieve configurability in much more convenient way.

Mot notably, if we can configure the child threads (or whatever we call them in this context) to not be able to write to storage and instead return the buffer changes, this will give us a big chunk of what we need for transaction parallelism.

@bkchr updated!

kianenigma · 2020-03-13T13:23:07Z

CC @tomusdrw ^^ / #1459

NikVolf · 2020-03-14T06:58:21Z

I will squash / force push this pr, because it is used in another pr, so sorry about that

primitives/core/src/traits.rs

tomusdrw

lgtm, few minor grumbles that I'm not super strong on.

tomusdrw · 2020-03-16T09:27:01Z

primitives/core/src/traits.rs

@@ -212,3 +212,21 @@ impl CallInWasmExt {
 		Self(Box::new(inner))
 	}
 }
+
+/// Something that can spawn tasks and also can be cloned.
+pub trait CloneableSpawn: futures::task::Spawn + Send + Sync {


I'm not a fan of traits that just describe the expected programatic behavior instead of some more high-level code idea.
Also this causes Box<dyn CloneableSpawn> to be quite widespread.

Can't we have a struct that will just wrap this? Like so:

#[derive(Clone)] struct TaskExecutor(Arc<dyn futures::Task::Spawn + Send + Sync>); impl TaskExecutor { pub fn spawn(...) {} }

We don't even need this extra trait cause it's in Arc and I feel it avoids much of code repetition.

I've considered something like that. The problem is we won't be able to have special implementation of clone in task executor this way. It might require little more than just shared reference clone in use cases I think about.

tomusdrw · 2020-03-16T09:29:42Z

primitives/core/src/lib.rs

@@ -70,6 +70,8 @@ mod changes_trie;
 #[cfg(feature = "std")]
 pub mod traits;
 pub mod testing;
+#[cfg(feature = "std")]
+pub mod tasks;


I'm not 100% sure this fits into sp-core, but at the same time we don't really have a better place currently, so I'm fine with this (unless someone has a better idea where to put it).

We probably could put everything tasks related (Also the CloneableSpawn trait) in its own crate, but the current amount of code isn't also that much.

bkchr

Some nitpicks, otherwise looks good.

client/service/src/task_manager.rs

primitives/core/Cargo.toml

bkchr · 2020-03-16T10:42:06Z

primitives/core/src/tasks.rs

+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Module for low-level asynchronous processing.


Isn't this just providing a single threaded executor?

Well, at the moment, yes

bkchr · 2020-03-16T10:43:25Z

primitives/core/src/lib.rs

@@ -70,6 +70,8 @@ mod changes_trie;
 #[cfg(feature = "std")]
 pub mod traits;
 pub mod testing;
+#[cfg(feature = "std")]
+pub mod tasks;


We probably could put everything tasks related (Also the CloneableSpawn trait) in its own crate, but the current amount of code isn't also that much.

primitives/state-machine/Cargo.toml

primitives/externalities/Cargo.toml

NikVolf added the A0-please_review Pull request needs code review. label Mar 13, 2020

bkchr suggested changes Mar 13, 2020

View reviewed changes

NikVolf changed the title ~~Pass task scheduler to Externalities, LocalCallExecutor, Client~~ Host parallelization infrastructure Mar 13, 2020

NikVolf force-pushed the nv-externalities-scheduler branch from 7dfa665 to 2fa2411 Compare March 14, 2020 06:26

NikVolf changed the title ~~Host parallelization infrastructure~~ Task executor extension for runtime host Mar 14, 2020

pepyakin reviewed Mar 14, 2020

View reviewed changes

primitives/core/src/traits.rs Outdated Show resolved Hide resolved

NikVolf force-pushed the nv-externalities-scheduler branch 7 times, most recently from 2c8716f to a11fa28 Compare March 14, 2020 16:17

tomusdrw approved these changes Mar 16, 2020

View reviewed changes

NikVolf force-pushed the nv-externalities-scheduler branch 2 times, most recently from ebb37e7 to 2f61e79 Compare March 16, 2020 10:30

bkchr approved these changes Mar 16, 2020

View reviewed changes

NikVolf force-pushed the nv-externalities-scheduler branch from 2f61e79 to ecee163 Compare March 16, 2020 11:16

create parallel tasks extension

a6a9e5c

NikVolf force-pushed the nv-externalities-scheduler branch from ecee163 to a6a9e5c Compare March 16, 2020 12:14

NikVolf added B0-silent Changes should not be mentioned in any release notes A8-looksgood and removed A0-please_review Pull request needs code review. labels Mar 16, 2020

NikVolf added this to the 2.0 milestone Mar 16, 2020

gavofyork approved these changes Mar 16, 2020

View reviewed changes

gavofyork merged commit 0bd9ffa into master Mar 16, 2020

gavofyork deleted the nv-externalities-scheduler branch March 16, 2020 15:30

General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 17, 2020

create parallel tasks extension (paritytech#5249)

704335a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task executor extension for runtime host #5249

Task executor extension for runtime host #5249

NikVolf commented Mar 13, 2020 •

edited

Loading

bkchr Mar 13, 2020

NikVolf Mar 13, 2020 •

edited

Loading

NikVolf Mar 13, 2020 •

edited

Loading

kianenigma Mar 13, 2020

NikVolf Mar 13, 2020

kianenigma commented Mar 13, 2020 •

edited

Loading

NikVolf commented Mar 14, 2020

tomusdrw left a comment

tomusdrw Mar 16, 2020

NikVolf Mar 16, 2020

tomusdrw Mar 16, 2020

bkchr Mar 16, 2020

bkchr left a comment

bkchr Mar 16, 2020

NikVolf Mar 16, 2020

bkchr Mar 16, 2020

Task executor extension for runtime host #5249

Task executor extension for runtime host #5249

Conversation

NikVolf commented Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

NikVolf Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

NikVolf Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma commented Mar 13, 2020 • edited Loading

NikVolf commented Mar 14, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf commented Mar 13, 2020 •

edited

Loading

NikVolf Mar 13, 2020 •

edited

Loading

NikVolf Mar 13, 2020 •

edited

Loading

kianenigma commented Mar 13, 2020 •

edited

Loading