ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to #9620

niklasad1 · 2018-09-21T08:01:31Z

In the verification queue we spawn up maximum worker threads to do the work and don't honor if num-verifiers i.e, we still spawn the maximum number of threads which consume extra memory. Thus, this PR changes that!

However, there is one catch though when --scale-verifiers is specified then
we must spawn all the threads because all threads are created upon initialization AFAIK and eventually might be scaled (this could probably be fixed but out-of-scope for this PR)

OT:
In my opinion, it doesn't make sense to use both num-verifiers and
scale-verifiers they are kind of contradictory!

Examples how this works

See added test cases

ethcore/src/verification/queue/mod.rs

ordian · 2018-09-21T14:47:08Z

ethcore/src/verification/queue/mod.rs

+			cmp::min(cli_max_verifiers, cmp::min(num_cpus, MAX_VERIFIERS))
+		};
+
+		let state = Arc::new((Mutex::new(State::Work(cli_max_verifiers)), Condvar::new()));


Should this be max_verifiers?

What I actually meant is cli_max_verifiers should be <= max_verifiers

I need to rework this logic cli_max_verifiers can be bigger than max_verifiers. I will add tests 🦀🦀

dvdplm · 2018-09-25T19:10:07Z

ethcore/src/verification/queue/mod.rs

+		}
+
+		#[test]
+		fn worker_threads_specifyed_to_zero_should_set_to_one() {


typo: s/specifyed/specified/

dvdplm · 2018-09-25T19:11:13Z

ethcore/src/verification/queue/mod.rs

+			let spec = Spec::new_test();
+			let engine = spec.engine;
+			let mut config = Config::default();
+			config.verifier_settings.num_verifiers = 1;


Shouldn't this be 0?

dvdplm · 2018-09-25T19:18:44Z

ethcore/src/verification/queue/mod.rs

+			let engine = spec.engine;
+			let mut config = Config::default();
+			config.verifier_settings.num_verifiers = 5;
+			config.verifier_settings.scale_verifiers = true;


Maybe add a helper to reduce the boiler plate code a bit, e.g.:

#[cfg(test)] fn test_config(verifiers: usize, scale: bool) -> Config { let spec = Spec::new_test(); let engine = spec.engine; let mut config = Config::default(); config.verifier_settings.num_verifiers = verifiers; config.verifier_settings.scale_verifiers = scale; config }

dvdplm

minor grumbles

dvdplm · 2018-09-25T19:21:10Z

ethcore/src/verification/queue/mod.rs

+			let queue = BlockQueue::new(config, engine, IoChannel::disconnected(), true);
+			queue.scale_verifiers(8);
+
+			assert_eq!(queue.num_verifiers(), 8);


Same comment as above: isn't this dependent on the test running with less than 9 cpus?

That's a good question and the answer is yes. I coded this because we assume that the maximum number of CPUs is 8 by the constant MAX_VERIFIERS but I think it doesn't make sense when thinking about it now!

I will change this test and remove the constant!

In the verification queue we spawn up worker threads to do the work. However, if `num-verifiers` is specified we still spawn the maximum number of threads which consume extra memory. There is one catch though when `--scale-verifiers` is specified then we can't do it because all threads are created upon initilization AFAIK. In my opinion, is doesn't to use both `num-verifiers` and `scale-verifiers` they are kind of contradictory!

* Address grumbles in new tests * Remove hardcoded `MAX_VERIFIERS` constant and replace it by relying entirely on `num_cpu` crate instead inorder to support CPUs that have more cores/logical cores

niklasad1 · 2018-09-25T21:50:55Z

ethcore/src/verification/queue/mod.rs

@@ -39,9 +39,6 @@ pub mod kind;
 const MIN_MEM_LIMIT: usize = 16384;
 const MIN_QUEUE_LIMIT: usize = 512;

-// maximum possible number of verification threads.
-const MAX_VERIFIERS: usize = 8;


This is removed because it assumes max 8 CPUs now instead use on num_cpus::get()

niklasad1 · 2018-09-25T21:52:46Z

ethcore/src/verification/queue/mod.rs

+		fn worker_threads_scaling_with_specifed_num_of_workers() {
+			let num_cpus = ::num_cpus::get();
+			// only run the test with at least 2 CPUs
+			if num_cpus > 1 {


This test is only useful on multicore CPUs

niklasad1 · 2018-09-25T21:53:23Z

ethcore/src/verification/queue/mod.rs

+			if num_cpus > 1 {
+				let spec = Spec::new_test();
+				let engine = spec.engine;
+				let config = get_test_config(num_cpus - 1, true);


This can't overflow because num_cpus is at least 2

…mon-deps * origin/master: ethereum libfuzzer integration small change (#9547) cli: remove reference to --no-ui in --unlock flag help (#9616) remove master from releasable branches (#9655) ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to (#9620) RPC: parity_getBlockReceipts (#9527) Remove unused dependencies (#9589) ignore key_server_cluster randomly failing tests (#9639) ethcore: handle vm exception when estimating gas (#9615) fix bad-block reporting no reason (#9638) Use static call and apparent value transfer for block reward contract code (#9603) HF in POA Sokol (2018-09-19) (#9607) bump smallvec to 0.6 in ethcore-light, ethstore and whisper (#9588) Add constantinople conf to EvmTestClient. (#9570)

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. F7-footprint 🐾 An enhancement to provide a smaller (system load, memory, network or disk) footprint. M4-core ⛓ Core client code / Rust. labels Sep 21, 2018

5chdn added this to the 2.2 milestone Sep 21, 2018

5chdn removed the F7-footprint 🐾 An enhancement to provide a smaller (system load, memory, network or disk) footprint. label Sep 21, 2018

ordian reviewed Sep 21, 2018

View reviewed changes

ethcore/src/verification/queue/mod.rs Outdated Show resolved Hide resolved

ordian reviewed Sep 21, 2018

View reviewed changes

niklasad1 added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 21, 2018

niklasad1 force-pushed the ethcore/VeriQueue-honor-cli-arg-num-workers branch 2 times, most recently from ce074ab to 1c6afc6 Compare September 21, 2018 20:14

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 21, 2018

dvdplm reviewed Sep 25, 2018

View reviewed changes

dvdplm suggested changes Sep 25, 2018

View reviewed changes

niklasad1 added 3 commits September 25, 2018 21:27

Fix nits in logic and add tests for verification

97d335a

refactor(verification queue) - rm hardcoded const

ab7d576

* Address grumbles in new tests * Remove hardcoded `MAX_VERIFIERS` constant and replace it by relying entirely on `num_cpu` crate instead inorder to support CPUs that have more cores/logical cores

niklasad1 force-pushed the ethcore/VeriQueue-honor-cli-arg-num-workers branch from a4312e4 to ab7d576 Compare September 25, 2018 21:46

niklasad1 commented Sep 25, 2018

View reviewed changes

dvdplm approved these changes Sep 26, 2018

View reviewed changes

ordian approved these changes Sep 26, 2018

View reviewed changes

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2018

5chdn merged commit 3216b14 into master Sep 26, 2018

5chdn deleted the ethcore/VeriQueue-honor-cli-arg-num-workers branch September 26, 2018 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to #9620

ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to #9620

niklasad1 commented Sep 21, 2018 •

edited

ordian Sep 21, 2018

ordian Sep 21, 2018

niklasad1 Sep 21, 2018

dvdplm Sep 25, 2018

dvdplm Sep 25, 2018

dvdplm Sep 25, 2018 •

edited

dvdplm left a comment

dvdplm Sep 25, 2018

niklasad1 Sep 25, 2018

niklasad1 Sep 25, 2018 •

edited

niklasad1 Sep 25, 2018

niklasad1 Sep 25, 2018

ethcore/VerificationQueue don't spawn up extra worker-threads when explictly specified not to #9620

ethcore/VerificationQueue don't spawn up extra worker-threads when explictly specified not to #9620

Conversation

niklasad1 commented Sep 21, 2018 • edited

Examples how this works

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

dvdplm Sep 25, 2018 • edited

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Sep 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to #9620

ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to #9620

niklasad1 commented Sep 21, 2018 •

edited

dvdplm Sep 25, 2018 •

edited

niklasad1 Sep 25, 2018 •

edited