use ThreadPool to execute spawn_worker(fn) #3836

Lawliet-Chan · 2019-10-16T17:26:44Z

Hi, I use ThreadPool to implement the spawn_worker() instead of std::thread::spawn.
The pool size is set available CPU nums.
[clabot:check]

parity-cla-bot · 2019-10-16T17:26:47Z

It looks like @CrocdileChan hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

Lawliet-Chan · 2019-10-16T17:35:33Z

[clabot:check]

parity-cla-bot · 2019-10-16T17:35:34Z

It looks like @CrocdileChan signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Lawliet-Chan · 2019-10-16T17:35:39Z

 [clabot:check]

…

------------------ 原始邮件 ------------------ 发件人: "Parity CLA Bot"<notifications@github.com>; 发送时间: 2019年10月17日(星期四) 凌晨1:26 收件人: "paritytech/substrate"<substrate@noreply.github.com>; 抄送: "L.lawliet"<1576710154@qq.com>; "Mention"<mention@noreply.github.com>; 主题: Re: [paritytech/substrate] use ThreadPool to execute spawn_worker(fn) (#3836) It looks like @CrocdileChan hasn't signed our Contributor License Agreement, yet. The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence. Wikipedia You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, please reply to this thread with [clabot:check] to prove it. Many thanks, Parity Technologies CLA Bot — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

parity-cla-bot · 2019-10-16T17:35:40Z

It looks like @CrocdileChan signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

Please use tabs not spaces and please avoid reformatting the code that you don't change.

tomusdrw · 2019-10-16T19:22:27Z

core/offchain/src/lib.rs

-	marker::PhantomData,
-	sync::Arc,
-};
+extern crate num_cpus;


We use edition = 2018, this is not needed.

Suggested change

extern crate num_cpus;

tomusdrw · 2019-10-16T19:48:47Z

core/offchain/src/lib.rs

-		assert_eq!(pool.status().ready, 1);
-		assert_eq!(pool.ready().next().unwrap().is_propagateable(), false);
-	}
+    use super::*;


We use tabs in the codebase, please avoid reformattings.

tomusdrw · 2019-10-16T19:53:28Z

core/offchain/Cargo.toml

@@ -16,6 +16,8 @@ futures-timer = "0.4.0"
 hyper = "0.12.35"
 hyper-tls = "0.3.2"
 log = "0.4.8"
+threadpool = "1.0"


Latest version is 1.7, can you update the numbers here?

tomusdrw · 2019-10-16T19:53:36Z

core/offchain/Cargo.toml

@@ -16,6 +16,8 @@ futures-timer = "0.4.0"
 hyper = "0.12.35"
 hyper-tls = "0.3.2"
 log = "0.4.8"
+threadpool = "1.0"
+num_cpus = "1.6"


Suggested change

num_cpus = "1.6"

num_cpus = "1.10"

kianenigma · 2019-10-17T08:46:30Z

core/offchain/src/lib.rs

@@ -39,6 +39,8 @@ use std::{
 	sync::Arc,
 };

+extern crate num_cpus;


should not be needed in 2018 edition?

I'm sorry I don't understand what you mean.

The usage of extern crate xxx should not be needed anymore.

Yup, just remove that line, fix the tests and we are good to go.

There are build results below the comments section on github:
https://gitlab.parity.io/parity/substrate/-/jobs/249361

Actually it's not a test failing, but you need to run cargo check locally and commit changes in Cargo.lock file.

I have committed the change of Cargo.lock . But there is a trouble, when I run 'cargo check' in the path ' ~/substrate/ ' , it shows that futures-io-preview can't compile.

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:292:28
|
292 | -> Poll<Result<&[u8]>>;
| ^ expected lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of argument 2's 2 lifetimes it is borrowed from

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:559:32
|
559 | -> Poll<Result<&[u8]>>
| ^ expected lifetime parameter
...
571 | deref_async_buf_read!();
| ------------------------ in this macro invocation
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of cx's 2 lifetimes it is borrowed from

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:559:32
|
559 | -> Poll<Result<&[u8]>>
| ^ expected lifetime parameter
...
575 | deref_async_buf_read!();
| ------------------------ in this macro invocation
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of cx's 2 lifetimes it is borrowed from

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:584:28
|
584 | -> Poll<Result<&[u8]>>
| ^ expected lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of cx's 2 lifetimes it is borrowed from

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:597:32
|
597 | -> Poll<Result<&[u8]>>
| ^ expected lifetime parameter
...
609 | delegate_async_buf_read_to_stdio!();
| ------------------------------------ in this macro invocation
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of argument 2's 2 lifetimes it is borrowed from

error[E0106]: missing lifetime specifier
--> /Users/ChenXinRan/.cargo/registry/src/code.aliyun.com-738b7dba08a2a41e/futures-io-preview-0.3.0-alpha.19/src/lib.rs:597:32
|
597 | -> Poll<Result<&[u8]>>
| ^ expected lifetime parameter
...
613 | delegate_async_buf_read_to_stdio!();
| ------------------------------------ in this macro invocation
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of argument 2's 2 lifetimes it is borrowed from

error: aborting due to 6 previous errors

But your code seem to be compiling well on the CI machine.. Make sure you have the latest version of rustc and stable/nightly toolchains installed.

Here is my rust version.

rustc 1.38.0-nightly (69656fa4c 2019-07-13)
rustc 1.40.0-nightly (ddf43867a 2019-09-26)
rustup 1.19.0 (2af131cf9 2019-09-08)

Lawliet-Chan · 2019-10-18T09:58:23Z

OH, today the substrate is already updated. Just a moment, I merge the newest Cargo.lock

Lawliet-Chan · 2019-10-18T11:27:54Z

@tomusdrw I don't understand why CI/CD still tests failing. I have merge the newest substrate/Cargo.lock to mine and run 'cargo check'. But it still shows these.
And I've send message to you on the riot.
Shall we talk on the riot?

tomusdrw · 2019-10-18T13:30:46Z

@CrocdileChan I didn't get anything on Riot, my handle @todr:matrix.parity.io.

I've just pushed https://github.com/paritytech/substrate/pull/new/pull-3836 with updated Cargo.lock - you can cherry-pick the commit from there.

Here is the commit: a129240

bkchr · 2019-10-20T19:48:02Z

core/offchain/src/lib.rs

@@ -39,6 +39,8 @@ use std::{
 	sync::Arc,


Suggested change

sync::Arc,

sync::{Arc, Mutex},

bkchr · 2019-10-20T19:48:11Z

core/offchain/src/lib.rs

@@ -39,6 +39,8 @@ use std::{
 	sync::Arc,
 };

+use threadpool::ThreadPool;
+use std::sync::Mutex;


Suggested change

use std::sync::Mutex;

sure, I've already formatted it.

@bkchr @tomusdrw I've already fixed it and CI/CD run success a few days ago. Could you please merge this PR?

It is not only about the CI finishing, also the reviewers need to be happy.

tomusdrw · 2019-10-21T06:57:35Z

core/offchain/src/lib.rs

@@ -36,9 +36,10 @@
 use std::{
 	fmt,
 	marker::PhantomData,
-	sync::Arc,
+	sync::{Arc,Mutex},


Please use parking_lot::Mutex instaed of std::sync::Mutex.

tomusdrw · 2019-10-21T06:57:54Z

core/offchain/src/lib.rs

+	/// Note that we should avoid that if we switch to future-based runtime in the future,
+	/// alternatively:
+	fn spawn_worker(&self, f: impl FnOnce() -> () + Send + 'static) {
+		self.thread_pool.lock().unwrap().execute(f);


When you switch to parking_lot::Mutex, this will be just:

Suggested change

self.thread_pool.lock().unwrap().execute(f);

self.thread_pool.lock().execute(f);

tomaka · 2019-10-21T18:11:01Z

What is the reason of this change? Using a thread pool for threads that we create every 6 seconds seems like a very premature optimization. And now instead of having either 0 or 1 thread alive at any given point in time, we maintain 4 or 8 (your number of CPUs) of them.

tomusdrw · 2019-10-21T19:31:12Z

We don't really know the kind of work users of our library will do in offchain workers. I can easily imagine that they will take way more than 6 seconds. Using fixed-size thread pool is in my mind is cheaper and safer than spawning a new thread for each task, although on the second though having this number set to as high as number of cores indeed seems wasteful for most of the cases we support currently (especially given that we don't run per-module workers in parallel). What's your suggestion then? Do you think we should revert it? Lower the number or make it configurable?

tomaka · 2019-10-21T19:50:31Z

What's your suggestion then?

My suggestion is to not care about performances unless something actually shows that there's a bottleneck. Instead, keep the code as clear and simple as possible.

It's not a big deal to have a threads pool here, but I also don't understand why that kind of changes are made.

tomusdrw · 2019-10-21T19:59:33Z

My suggestion is to not care about performances unless something actually shows that there's a bottleneck.

Sure thing, I agree 100%. But we are building a library here, we won't know all possible cases that the bottleneck might happen. IMHO it's better be safe and cap the number of threads that offchain workers may consume than to allow spawning unlimited number in case someone just has a buggy implementation of a worker.

Instead, keep the code as clear and simple as possible.

I don't find the current implementation that much more convoluted and unclear to justify rejecting sensible external contribution.

tomaka · 2019-10-21T20:12:53Z

IMHO it's better be safe and cap the number of threads that offchain workers may consume than to allow spawning unlimited number in case someone just has a buggy implementation of a worker.

If someone has a buggy implementation of a worker: before this PR we have a resource leak, and after this PR we silently not spawn further workers. Both solutions are kind of crappy.

tomaka · 2019-10-21T20:13:06Z

core/offchain/src/lib.rs

@@ -58,6 +60,7 @@ pub struct OffchainWorkers<Client, Storage, Block: traits::Block> {
 	client: Arc<Client>,
 	db: Storage,
 	_block: PhantomData<Block>,
+	thread_pool: Mutex<ThreadPool>,


The Mutex isn't needed.

ThreadPool isn't Send/Sync

What you said surprised me a lot, so I checked.

https://github.com/rust-threadpool/rust-threadpool/blob/21a70c7e8b19fb73a9f3f44a55ca1b7333d804f9/src/lib.rs#L1212-L1228

I don't see a test for Sync for ThreadPool, but I would be surprised if it wasn't Sync as well.

rust-threadpool/rust-threadpool#96

There is an issue for this.

CrocdileChan added 2 commits October 17, 2019 01:11

use ThreadPool to spawn_worker()

bbd6e42

Merge branch 'master' of github.com:CrocdileChan/substrate

df4d06d

Lawliet-Chan requested a review from tomusdrw as a code owner October 16, 2019 17:26

tomusdrw suggested changes Oct 16, 2019

View reviewed changes

tomusdrw added A5-grumble labels Oct 16, 2019

CrocdileChan added 4 commits October 17, 2019 08:14

Merge branch 'master' of https://github.com/paritytech/substrate

9c87d8b

use ThreadPool to implement spawn_worker(fn)

b7f48f1

use ThreadPool to implement spawn_worker(f)

6394319

update [dependencies] threadpool and num_cpus version

742d72f

kianenigma reviewed Oct 17, 2019

View reviewed changes

tomusdrw added A7-looksgoodtestsfail and removed A5-grumble labels Oct 18, 2019

tomusdrw approved these changes Oct 18, 2019

View reviewed changes

CrocdileChan added 2 commits October 18, 2019 17:05

rm 'extern crate num_cpus'

d09f65d

cargo.lock update

48899be

merge the newest cargo.lock

e5a0bd1

tomusdrw and others added 2 commits October 18, 2019 22:26

Update Cargo.lock

8fedb51

use Mutex to wrap OffchainWorkers.thread_pool

1114fa5

bkchr reviewed Oct 20, 2019

View reviewed changes

format use crate

07cb32d

tomusdrw reviewed Oct 21, 2019

View reviewed changes

use parking_lot::Mutex instead of std::sync::Mutex

a37fc79

tomusdrw approved these changes Oct 21, 2019

View reviewed changes

tomusdrw added A8-looksgood and removed A7-looksgoodtestsfail labels Oct 21, 2019

tomusdrw merged commit b7627c4 into paritytech:master Oct 21, 2019

tomaka reviewed Oct 21, 2019

View reviewed changes

	self.thread_pool.lock().unwrap().execute(f);
	self.thread_pool.lock().execute(f);

use ThreadPool to execute spawn_worker(fn) #3836

use ThreadPool to execute spawn_worker(fn) #3836

Conversation

Lawliet-Chan commented Oct 16, 2019 • edited Loading

parity-cla-bot commented Oct 16, 2019

Lawliet-Chan commented Oct 16, 2019

parity-cla-bot commented Oct 16, 2019

Lawliet-Chan commented Oct 16, 2019 via email

parity-cla-bot commented Oct 16, 2019

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

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

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

Choose a reason for hiding this comment

Lawliet-Chan commented Oct 18, 2019

Lawliet-Chan commented Oct 18, 2019

tomusdrw commented Oct 18, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Oct 21, 2019

tomusdrw commented Oct 21, 2019

tomaka commented Oct 21, 2019

tomusdrw commented Oct 21, 2019

tomaka commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lawliet-Chan commented Oct 16, 2019 •

edited

Loading

tomusdrw commented Oct 18, 2019 •

edited

Loading

tomaka Oct 21, 2019 •

edited

Loading