Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increasing thread sleeps for socket listener thread and per-connection threads #2773

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Conversation

DavidBurkett
Copy link
Contributor

This significantly decreases cpu usage, especially for nodes with >20 peers. There's very little reason for the thread sleeps to be 1 ms. The only (theoretical) advantage is during mining, but as long as miners have a sufficient number of peers (~10+), there should be no practical effect.

@mcdallas
Copy link
Contributor

Some results: I increased mine from 1ms to 10ms and cpu usage went from 90% to 30%

@@ -244,7 +244,7 @@ fn poll<H>(
let _ = thread::Builder::new()
.name("peer".to_string())
.spawn(move || {
let sleep_time = time::Duration::from_millis(1);
let sleep_time = time::Duration::from_millis(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw @mcdallas reported 10ms test result, are we sure the 5ms here has the same improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't be the same improvement, but it will be close. Setting the conn thread sleep too high could have negative consequences, especially for miners. We can continue to tweak it as necessary, but this seems a happy middle ground for now.

@garyyu
Copy link
Contributor

garyyu commented Apr 23, 2019

Perhaps we should merge all p2p threads into one single thread, in the future. But for the moment, it's good to have this quick and simple improvement 👍

@hashmap hashmap merged commit 0e6a249 into mimblewimble:master Apr 23, 2019
@garyyu
Copy link
Contributor

garyyu commented Apr 23, 2019

After 7 hours test, with this 5ms modification, before upgrading:

Total Peers:          112
3414 Total Connection:  116
3414 Total Established: 115

62.8%  CPU LOAD for Grin  (not stable, but a typical value)

After upgrading:

Total Peers:          164
3414 Total Connection:  164
3414 Total Established: 164

51.8% CPU LOAD for Grin (typical value)

Because the peers number different, we can't directly compare the improvement, but an estimated value 80% improvement here, anyway it deserves to have this PR, thanks @DavidBurkett

@0xmichalis
Copy link
Contributor

I am seeing a ~50% cpu usage reduction in GCP - nice one @DavidBurkett !
grin-cpu

@antiochp antiochp added this to the 1.1.0 milestone Jun 5, 2019
@antiochp antiochp added the release notes To be included in release notes (of relevant milestone). label Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release notes To be included in release notes (of relevant milestone).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants