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

use ThreadPool to execute spawn_worker(fn) #3836

Merged
tomusdrw merged 13 commits into from
Oct 21, 2019
Merged

use ThreadPool to execute spawn_worker(fn) #3836

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bbd6e42
use ThreadPool to spawn_worker()
Oct 16, 2019
df4d06d
Merge branch 'master' of github.com:CrocdileChan/substrate
Oct 16, 2019
9c87d8b
Merge branch 'master' of https://github.com/paritytech/substrate
Oct 17, 2019
b7f48f1
use ThreadPool to implement spawn_worker(fn)
Oct 17, 2019
6394319
use ThreadPool to implement spawn_worker(f)
Oct 17, 2019
742d72f
update [dependencies] threadpool and num_cpus version
Oct 17, 2019
d09f65d
rm 'extern crate num_cpus'
Oct 18, 2019
48899be
cargo.lock update
Oct 18, 2019
e5a0bd1
merge the newest cargo.lock
Oct 18, 2019
8fedb51
Update Cargo.lock
tomusdrw Oct 18, 2019
1114fa5
use Mutex to wrap OffchainWorkers.thread_pool
Oct 19, 2019
07cb32d
format use crate
Oct 20, 2019
a37fc79
use parking_lot::Mutex instead of std::sync::Mutex
Oct 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/offchain/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ futures-timer = "0.4.0"
hyper = "0.12.35"
hyper-tls = "0.3.2"
log = "0.4.8"
threadpool = "1.7"
num_cpus = "1.10"
offchain-primitives = { package = "substrate-offchain-primitives", path = "./primitives" }
codec = { package = "parity-scale-codec", version = "1.0.0", features = ["derive"] }
parking_lot = "0.9.0"
Expand Down
29 changes: 16 additions & 13 deletions core/offchain/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ use std::{
sync::Arc,

@bkchr bkchr Oct 20, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
sync::Arc,
sync::{Arc, Mutex},

};

extern crate num_cpus;

@kianenigma kianenigma Oct 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should not be needed in 2018 edition?

@Lawliet-Chan Lawliet-Chan Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@kianenigma kianenigma Oct 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@tomusdrw tomusdrw Oct 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Lawliet-Chan Lawliet-Chan Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

@tomusdrw tomusdrw Oct 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Lawliet-Chan Lawliet-Chan Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I see.

@Lawliet-Chan Lawliet-Chan Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@kianenigma kianenigma Oct 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Lawliet-Chan Lawliet-Chan Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

use threadpool::ThreadPool;
use client::runtime_api::ApiExt;
use futures::future::Future;
use log::{debug, warn};
Expand All @@ -58,6 +60,7 @@ pub struct OffchainWorkers<Client, Storage, Block: traits::Block> {
client: Arc<Client>,
db: Storage,
_block: PhantomData<Block>,
thread_pool: ThreadPool,
}

impl<Client, Storage, Block: traits::Block> OffchainWorkers<Client, Storage, Block> {
Expand All @@ -67,6 +70,7 @@ impl<Client, Storage, Block: traits::Block> OffchainWorkers<Client, Storage, Blo
client,
db,
_block: PhantomData,
thread_pool: ThreadPool::new(num_cpus::get()),
}
}
}
Expand Down Expand Up @@ -116,7 +120,7 @@ impl<Client, Storage, Block> OffchainWorkers<
debug!("Spawning offchain workers at {:?}", at);
let number = *number;
let client = self.client.clone();
spawn_worker(move || {
self.spawn_worker(move || {
let runtime = client.runtime_api();
let api = Box::new(api);
debug!("Running offchain workers at {:?}", at);
Expand All @@ -134,19 +138,18 @@ impl<Client, Storage, Block> OffchainWorkers<
futures::future::Either::Right(futures::future::ready(()))
}
}
}

/// Spawns a new offchain worker.
///
/// We spawn offchain workers for each block in a separate thread,
/// since they can run for a significant amount of time
/// in a blocking fashion and we don't want to block the runtime.
///
/// Note that we should avoid that if we switch to future-based runtime in the future,
/// alternatively:
/// TODO [ToDr] (#1458) we can consider using a thread pool instead.
fn spawn_worker(f: impl FnOnce() -> () + Send + 'static) {
std::thread::spawn(f);
/// Spawns a new offchain worker.
///
/// We spawn offchain workers for each block in a separate thread,
/// since they can run for a significant amount of time
/// in a blocking fashion and we don't want to block the runtime.
///
/// 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.execute(f);
}
}

#[cfg(test)]
Expand Down