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

Use an adaptive number of threads in the verification queue #2445

Merged
merged 12 commits into from
Nov 22, 2016

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Oct 3, 2016

No description provided.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 3, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 3, 2016

an ideal solution would probably just balance the rate of queue draining vs the rate of verification per thread and adaptively allocate the correct number of threads based on that.

this one uses some simple heuristics which seem to work decently well (although there's a bit more churn than I'd like) but it could be pretty simple to alter it to something more sophisticated.

@coveralls
Copy link

coveralls commented Oct 3, 2016

Coverage Status

Coverage increased (+0.01%) to 86.452% when pulling 2d28c70 on adaptive_queue_threads into d205c08 on master.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 3, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Oct 3, 2016

please add tests
at least purely descriptive ones for add_verifier, remove_verifier

@NikVolf NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 3, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.411% when pulling abbf3b3 on adaptive_queue_threads into d205c08 on master.

@gavofyork
Copy link
Contributor

still no tests :(

Copy link
Contributor

@NikVolf NikVolf left a comment

Choose a reason for hiding this comment

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

this heuristic might oscillate between, say, 4 and 5 threads
and each collect_garbage call would be creating threads or wait for one thread to join

trace!(target: "verification", "v_rate_per={}, drained={}, scaling to {} verifiers",
v_count_per, drained, needed);

for _ in num_verifiers..needed {
Copy link
Contributor

@NikVolf NikVolf Oct 6, 2016

Choose a reason for hiding this comment

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

can't extra 2 verifiers be added to the 7 verifiers
so that this check later will fail given ::num_cpus::get() == 8?

    if len == ::num_cpus::get() {
            return;
     }

Copy link
Contributor

@NikVolf NikVolf Oct 6, 2016

Choose a reason for hiding this comment

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

ah, disregard this, they are added 1 by 1
still no need to call add_verifier and lock self.verifiers there once maximum is reached

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 6, 2016

@NikVolf I'll add tests once we figure out the right strategy for the adaptive threads. It's true that we don't want to be adding and removing threads all the time. Maybe a specialized thread pool where we allocate min(num_cpus, 8) threads and park/unpark them as necessary. I've found that with the current changes my block queue is almost always fully verified with less work being done.

I also think the ticks_per_adjustment might work better as 12 so it adjusts once every minute.

@gavofyork gavofyork added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 11, 2016
@rphmeier rphmeier added this to the 1.5 Tenuity milestone Oct 17, 2016
@rphmeier rphmeier mentioned this pull request Nov 4, 2016
@gavofyork gavofyork modified the milestone: 1.5 Tenuity Nov 11, 2016
@rphmeier rphmeier added the A0-pleasereview 🤓 Pull request needs code review. label Nov 17, 2016
@rphmeier rphmeier removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Nov 17, 2016
@rphmeier
Copy link
Contributor Author

Ready for review again.

Changes:

  • allocates the threads up front
  • wakes verifiers and puts them to sleep as necessary
  • simple scaling algorithm just compares amounts of verified and unverified blocks
  • adjustment once every minute

@gavofyork
Copy link
Contributor

tests?

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 20, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Nov 21, 2016

tests :)

@NikVolf could you approve the changes on your review now that I've addressed that grumble?

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 21, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2016
@gavofyork gavofyork merged commit 03d3e58 into master Nov 22, 2016
@gavofyork gavofyork deleted the adaptive_queue_threads branch November 22, 2016 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants