ethcore-io retries failed work steal #9651

debris · 2018-09-25T22:40:49Z

No description provided.

debris · 2018-09-25T22:41:43Z

ethcore/src/engines/tendermint/mod.rs

@@ -693,7 +693,6 @@ impl Engine<EthereumMachine> for Tendermint {
 	}

 	fn stop(&self) {
-		self.step_service.stop()


stop doesn't nee to be called as it is already called on drop

debris · 2018-09-25T22:42:36Z

util/io/src/service_mio.rs

@@ -430,7 +430,7 @@ impl<Message> IoChannel<Message> where Message: Send + Sync + 'static {
 /// General IO Service. Starts an event loop and dispatches IO requests.
 /// 'Message' is a notification message type
 pub struct IoService<Message> where Message: Send + Sync + 'static {
-	thread: Mutex<Option<JoinHandle<()>>>,
+	thread: Option<JoinHandle<()>>,


since rust 1.29 JoinHandle is Send + Sync

debris · 2018-09-25T22:43:41Z

util/io/src/worker.rs

-					chase_lev::Steal::Data(work) => Worker::do_work(work, channel.clone()),
-					_ => break,
+					deque::Steal::Data(work) => Worker::do_work(work, channel.clone()),
+					deque::Steal::Retry => {},


this was a bug, if worker lost the race for stealing data, it was incorrectly killed

The break is from the inner while loop, so it wasn't really killed, but rather put on sleep, until there is more work to do. I wonder whether this change can increase CPU usage because of busy loop.

I'm pretty sure it's going to increase CPU usage, but should also improve performance, cause previously if a thread lost the race it would go to sleep vs re-trying and get the work done. If it went to sleep other threads had to finish whatever they were doing and then pick up the new task.

there is a case when Worker is being dropped. If steal returns Retry, work will never be resumed

andresilva

LGTM.

andresilva · 2018-09-26T08:59:41Z

ethcore/src/engines/tendermint/mod.rs

@@ -693,7 +693,6 @@ impl Engine<EthereumMachine> for Tendermint {
 	}

 	fn stop(&self) {


I think we can remove this method altogether, there's a default empty implementation in the Engine trait.

niklasad1

LGTM, but it would be good to benchmark how much more CPU this change this will introduce as Andronik said!

tomusdrw · 2018-09-28T09:52:52Z

util/io/src/worker.rs

-					chase_lev::Steal::Data(work) => Worker::do_work(work, channel.clone()),
-					_ => break,
+					deque::Steal::Data(work) => Worker::do_work(work, channel.clone()),
+					deque::Steal::Retry => {},


I'm pretty sure it's going to increase CPU usage, but should also improve performance, cause previously if a thread lost the race it would go to sleep vs re-trying and get the work done. If it went to sleep other threads had to finish whatever they were doing and then pick up the new task.

niklasad1 · 2018-09-28T13:29:36Z

Let's merge it?

5chdn · 2018-09-29T20:25:07Z

We have plenty of CPU available :p

* ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

…mon-deps * origin/master: CI: Remove unnecessary pipes (#9681) test.sh: use cargo --target for platforms other than linux, win or mac (#9650) ci: fix push script (#9679) Hardfork the testnets (#9562) Calculate sha3 instead of sha256 for push-release. (#9673) ethcore-io retries failed work steal (#9651) fix(light_fetch): avoid race with BlockNumber::Latest (#9665) Test fix for windows cache name... (#9658) refactor(fetch) : light use only one `DNS` thread (#9647)

* parity-version: bump beta to 2.1.2 * docs(rpc): push the branch along with tags (#9578) * docs(rpc): push the branch along with tags * ci: remove old rpc docs script * Remove snapcraft clean (#9585) * Revert " add snapcraft package image (master) (#9584)" This reverts commit ceaedbb. * Update package-snap.sh * Update .gitlab-ci.yml * ci: fix regex 🙄 (#9597) * docs(rpc): annotate tag with the provided message (#9601) * Update ropsten.json (#9602) * HF in POA Sokol (2018-09-19) (#9607) poanetwork/poa-chain-spec#86 * fix(network): don't disconnect reserved peers (#9608) The priority of && and || was borked. * fix failing node-table tests on mac os, closes #9632 (#9633) * ethcore-io retries failed work steal (#9651) * ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam * remove master from releasable branches (#9655) * remove master from releasable branches need backporting in beta fix https://gitlab.parity.io/parity/parity-ethereum/-/jobs/101065 etc * add except for snap packages for master * Test fix for windows cache name... (#9658) * Test fix for windows cache name... * Fix variable name. * fix(light_fetch): avoid race with BlockNumber::Latest (#9665) * Calculate sha3 instead of sha256 for push-release. (#9673) * Calculate sha3 instead of sha256 for push-release. * Add pushes to the script. * Hardfork the testnets (#9562) * ethcore: propose hardfork block number 4230000 for ropsten * ethcore: propose hardfork block number 9000000 for kovan * ethcore: enable kip-4 and kip-6 on kovan * etcore: bump kovan hardfork to block 9.2M * ethcore: fix ropsten constantinople block number to 4.2M * ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream * ci: fix push script (#9679) * ci: fix push script * Fix copying & running on windows. * CI: Remove unnecessary pipes (#9681) * ci: reduce gitlab pipelines significantly * ci: build pipeline for PR * ci: remove dead weight * ci: remove github release script * ci: remove forever broken aura tests * ci: add random stuff to the end of the pipes * ci: add wind and mac to the end of the pipe * ci: remove snap artifacts * ci: (re)move dockerfiles * ci: clarify job names * ci: add cargo audit job * ci: make audit script executable * ci: ignore snap and docker files for rust check * ci: simplify audit script * ci: rename misc to optional * ci: add publish script to releaseable branches * ci: more verbose cp command for windows build * ci: fix weird binary checksum logic in push script * ci: fix regex in push script for windows * ci: simplify gitlab caching * docs: align README with ci changes * ci: specify default cargo target dir * ci: print verbose environment * ci: proper naming of scripts * ci: restore docker files * ci: use docker hub file * ci: use cargo home instead of cargo target dir * ci: touch random rust file to trigger real builds * ci: set cargo target dir for audit script * ci: remove temp file * ci: don't export the cargo target dir in the audit script * ci: fix windows unbound variable * docs: fix gitlab badge path * rename deprecated gitlab ci variables https://docs.gitlab.com/ee/ci/variables/#9-0-renaming * ci: fix git compare for nightly builds * test: skip c++ example for all platforms but linux * ci: add random rust file to trigger tests * ci: remove random rust file * disable cpp lib test for mac, win and beta (#9686) * cleanup ci merge * ci: fix tests * fix bad-block reporting no reason (#9638) * ethcore: fix detection of major import (#9552) * sync: set state to idle after sync is completed * sync: refactor sync reset * Don't hash the init_code of CREATE. (#9688) * Docker: run as parity user (#9689) * Implement CREATE2 gas changes and fix some potential overflowing (#9694) * Implement CREATE2 gas changes and fix some potential overflowing * Ignore create2 state tests * Split CREATE and CREATE2 in gasometer * Generalize rounding (x + 31) / 32 to to_word_size * make instantSeal engine backwards compatible, closes #9696 (#9700) * ethcore: delay ropsten hardfork (#9704) * fix (light/provider) : Make `read_only executions` read-only (#9591) * `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear * CI: Skip docs job for nightly (#9693) * ci: force-tag wiki changes * ci: force-tag wiki changes * ci: skip docs job for master and nightly * ci: revert docs job checking for nightly tag * ci: exclude docs job from nightly builds in gitlab script

* ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

* parity-version: bump stable to 2.0.7 * HF in POA Sokol (2018-09-19) (#9607) poanetwork/poa-chain-spec#86 * fix failing node-table tests on mac os, closes #9632 (#9633) * fix(light_fetch): avoid race with BlockNumber::Latest (#9665) * CI: Remove unnecessary pipes (#9681) * ci: reduce gitlab pipelines significantly * ci: build pipeline for PR * ci: remove dead weight * ci: remove github release script * ci: remove forever broken aura tests * ci: add random stuff to the end of the pipes * ci: add wind and mac to the end of the pipe * ci: remove snap artifacts * ci: (re)move dockerfiles * ci: clarify job names * ci: add cargo audit job * ci: make audit script executable * ci: ignore snap and docker files for rust check * ci: simplify audit script * ci: rename misc to optional * ci: add publish script to releaseable branches * ci: more verbose cp command for windows build * ci: fix weird binary checksum logic in push script * ci: fix regex in push script for windows * ci: simplify gitlab caching * docs: align README with ci changes * ci: specify default cargo target dir * ci: print verbose environment * ci: proper naming of scripts * ci: restore docker files * ci: use docker hub file * ci: use cargo home instead of cargo target dir * ci: touch random rust file to trigger real builds * ci: set cargo target dir for audit script * ci: remove temp file * ci: don't export the cargo target dir in the audit script * ci: fix windows unbound variable * docs: fix gitlab badge path * rename deprecated gitlab ci variables https://docs.gitlab.com/ee/ci/variables/#9-0-renaming * ci: fix git compare for nightly builds * test: skip c++ example for all platforms but linux * ci: add random rust file to trigger tests * ci: remove random rust file * disable cpp lib test for mac, win and beta (#9686) * cleanup ci merge * parity: bump clib * ci: fix tests * ci: disable c++ example * Docker: run as parity user (#9689) * CI: Skip docs job for nightly (#9693) * ci: force-tag wiki changes * ci: force-tag wiki changes * ci: skip docs job for master and nightly * ci: revert docs job checking for nightly tag * ci: exclude docs job from nightly builds in gitlab script * fix (light/provider) : Make `read_only executions` read-only (#9591) * `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear * ethcore: fix detection of major import (#9552) * sync: set state to idle after sync is completed * sync: refactor sync reset * parity: revert clib bump and fix tests * Fix path to parity.h (#9274) * Fix path to parity.h * Fix other paths as well * ethcore-io retries failed work steal (#9651) * ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

ethcore-io uses newer version of crossbeam && retries failed work steal

f15d957

debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 25, 2018

debris requested a review from tomusdrw September 25, 2018 22:40

debris commented Sep 25, 2018

View reviewed changes

debris added the B0-patchthis 🕷 label Sep 25, 2018

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

ethcore-io non-mio service uses newer crossbeam

3e68363

andresilva approved these changes Sep 26, 2018

View reviewed changes

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

niklasad1 approved these changes Sep 28, 2018

View reviewed changes

tomusdrw approved these changes Sep 28, 2018

View reviewed changes

ordian approved these changes Sep 28, 2018

View reviewed changes

5chdn merged commit ebaa43f into master Sep 29, 2018

5chdn deleted the ethcore-io-improvements branch September 29, 2018 20:25

5chdn mentioned this pull request Sep 30, 2018

Backports for beta 2.1.2 #9649

Merged

28 tasks

5chdn pushed a commit that referenced this pull request Sep 30, 2018

ethcore-io retries failed work steal (#9651)

0eee5da

* ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

5chdn added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Oct 10, 2018

5chdn mentioned this pull request Oct 10, 2018

Backports for stable 2.0.7 #9648

Merged

13 tasks

andresilva pushed a commit that referenced this pull request Oct 10, 2018

ethcore-io retries failed work steal (#9651)

e7af1c0

* ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ethcore-io retries failed work steal #9651

ethcore-io retries failed work steal #9651

debris commented Sep 25, 2018

debris Sep 25, 2018

debris Sep 25, 2018

debris Sep 25, 2018 •

edited

Loading

ordian Sep 26, 2018 •

edited

Loading

tomusdrw Sep 28, 2018

debris Sep 28, 2018

andresilva left a comment

andresilva Sep 26, 2018

niklasad1 left a comment

tomusdrw Sep 28, 2018

niklasad1 commented Sep 28, 2018

5chdn commented Sep 29, 2018

		@@ -693,7 +693,6 @@ impl Engine<EthereumMachine> for Tendermint {
		}

		fn stop(&self) {

ethcore-io retries failed work steal #9651

ethcore-io retries failed work steal #9651

Conversation

debris commented Sep 25, 2018

debris Sep 25, 2018

Choose a reason for hiding this comment

debris Sep 25, 2018

Choose a reason for hiding this comment

debris Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

ordian Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

tomusdrw Sep 28, 2018

Choose a reason for hiding this comment

debris Sep 28, 2018

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

andresilva Sep 26, 2018

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

tomusdrw Sep 28, 2018

Choose a reason for hiding this comment

niklasad1 commented Sep 28, 2018

5chdn commented Sep 29, 2018

debris Sep 25, 2018 •

edited

Loading

ordian Sep 26, 2018 •

edited

Loading