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

Disable parallel verification and skip verifiying already imported txs. #8834

Merged
merged 3 commits into from
Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion miner/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ linked-hash-map = "0.5"
log = "0.3"
parking_lot = "0.5"
price-info = { path = "../price-info" }
rayon = "1.0"
rlp = { path = "../util/rlp" }
trace-time = { path = "../util/trace-time" }
transaction-pool = { path = "../transaction-pool" }
Expand Down
1 change: 0 additions & 1 deletion miner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ extern crate keccak_hash as hash;
extern crate linked_hash_map;
extern crate parking_lot;
extern crate price_info;
extern crate rayon;
extern crate rlp;
extern crate trace_time;
extern crate transaction_pool as txpool;
Expand Down
11 changes: 8 additions & 3 deletions miner/src/pool/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use std::collections::BTreeMap;

use ethereum_types::{H256, U256, Address};
use parking_lot::RwLock;
use rayon::prelude::*;
use transaction;
use txpool::{self, Verifier};

Expand Down Expand Up @@ -179,8 +178,14 @@ impl TransactionQueue {

let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone());
let results = transactions
.into_par_iter()
.map(|transaction| verifier.verify_transaction(transaction))
.into_iter()
Copy link
Contributor

Choose a reason for hiding this comment

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

@tomusdrw Did you ever look at the size of the work for this and how par_iter() distributes it? Like if it's work-stealing it might actually cause more overhead to parallelise if the work-pieces are small enough (In some rayon examples for instance he shows a 0.95x speedup (so it's much worse) on a 4 core machine). It might be worth testing with rayons Chunks iterator to chunk it up into larger work-pieces for each CPU to tackle.

.map(|transaction| {
if self.pool.read().find(&transaction.hash()).is_some() {
bail!(transaction::Error::AlreadyImported)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the primary cause of the high CPU usage the parallel verification or verifying already imported txs. Or a combination of both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it seems like a combination of even more things, this brings the load down, but not 100% to the level of the one of 1.10. I'm still figuring out other culprits.


verifier.verify_transaction(transaction)
})
.map(|result| result.and_then(|verified| {
self.pool.write().import(verified)
.map(|_imported| ())
Expand Down
9 changes: 9 additions & 0 deletions miner/src/pool/tests/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

use std::sync::{atomic, Arc};

use ethereum_types::{U256, H256, Address};
use rlp::Rlp;
use transaction::{self, Transaction, SignedTransaction, UnverifiedTransaction};
Expand All @@ -25,6 +27,7 @@ const MAX_TRANSACTION_SIZE: usize = 15 * 1024;

#[derive(Debug, Clone)]
pub struct TestClient {
verification_invoked: Arc<atomic::AtomicBool>,
account_details: AccountDetails,
gas_required: U256,
is_service_transaction: bool,
Expand All @@ -35,6 +38,7 @@ pub struct TestClient {
impl Default for TestClient {
fn default() -> Self {
TestClient {
verification_invoked: Default::default(),
account_details: AccountDetails {
nonce: 123.into(),
balance: 63_100.into(),
Expand Down Expand Up @@ -88,6 +92,10 @@ impl TestClient {
insertion_id: 1,
}
}

pub fn was_verification_triggered(&self) -> bool {
self.verification_invoked.load(atomic::Ordering::SeqCst)
}
}

impl pool::client::Client for TestClient {
Expand All @@ -98,6 +106,7 @@ impl pool::client::Client for TestClient {
fn verify_transaction(&self, tx: UnverifiedTransaction)
-> Result<SignedTransaction, transaction::Error>
{
self.verification_invoked.store(true, atomic::Ordering::SeqCst);
Ok(SignedTransaction::new(tx)?)
}

Expand Down
34 changes: 34 additions & 0 deletions miner/src/pool/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -796,3 +796,37 @@ fn should_include_local_transaction_to_a_full_pool() {
// then
assert_eq!(txq.status().status.transaction_count, 1);
}

#[test]
fn should_avoid_verifying_transaction_already_in_pool() {
// given
let txq = TransactionQueue::new(
txpool::Options {
max_count: 1,
max_per_sender: 2,
max_mem_usage: 50
},
verifier::Options {
minimal_gas_price: 1.into(),
block_gas_limit: 1_000_000.into(),
tx_gas_limit: 1_000_000.into(),
},
PrioritizationStrategy::GasPriceOnly,
);
let client = TestClient::new();
let tx1 = Tx::default().signed().unverified();

let res = txq.import(client.clone(), vec![tx1.clone()]);
assert_eq!(res, vec![Ok(())]);
assert_eq!(txq.status().status.transaction_count, 1);
assert!(client.was_verification_triggered());

// when
let client = TestClient::new();
let res = txq.import(client.clone(), vec![tx1]);
assert_eq!(res, vec![Err(transaction::Error::AlreadyImported)]);
assert!(!client.was_verification_triggered());

// then
assert_eq!(txq.status().status.transaction_count, 1);
}
4 changes: 3 additions & 1 deletion miner/src/pool/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ impl Default for Options {
}

/// Transaction to verify.
#[cfg_attr(test, derive(Clone))]
pub enum Transaction {
/// Fresh, never verified transaction.
///
Expand All @@ -75,7 +76,8 @@ pub enum Transaction {
}

impl Transaction {
fn hash(&self) -> H256 {
/// Return transaction hash
pub fn hash(&self) -> H256 {
match *self {
Transaction::Unverified(ref tx) => tx.hash(),
Transaction::Retracted(ref tx) => tx.hash(),
Expand Down