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

Add RPC call setscriptthreadsenabled: allow to temp. throttle CPU usage #12965

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jonasschnelli
Copy link
Member

jonasschnelli commented Apr 12, 2018

Bitcoin Core has been designed to synchronise/verify as fast as possible. This is usually desirable, though, on systems where other applications require a reasonable amount of CPU time (ex. desktop systems) the CPU usage maximisation of Bitcoin Core may be intrusive.

This PR adds two RPC calls:

  • setscriptthreadsenabled allows to disable/re-enable the script verification threads during runtime.
  • scriptthreadsinfo show information about the script verification threads (enabled/disabled and num threads)

This would be a base requirement for a "cpu throttle" feature in the GUI allowing one to temporary "throttle" verification (and therefore make the system usable for other tasks while syncing in the background)

The concept-draft for long-term resource profile:
https://gist.github.com/jonasschnelli/a3eb47147069b99d7c63d7da997b4225

ToDo:

  • Add tests
  • Release notes
@promag
Copy link
Member

promag left a comment

Concept ACK.

I think this is almost only useful when changed in the UI right?

Could have a couple of tests to exercise these new calls and the error.

Edit: you already have tests in the TODO 😄

{
if (request.fHelp || request.params.size() != 1) {
throw std::runtime_error(
"setscriptthreadsenabled\n"

This comment has been minimized.

@promag

promag Apr 12, 2018

Member

Missing parameter state.

if (request.fHelp || request.params.size() != 1) {
throw std::runtime_error(
"setscriptthreadsenabled\n"
"\nDisable/enable script verification threads and therefore reducing CPU usage on multicore systems.\n"

This comment has been minimized.

@promag

promag Apr 12, 2018

Member

Invert, Enable/disable script ...?

"setscriptthreadsenabled\n"
"\nDisable/enable script verification threads and therefore reducing CPU usage on multicore systems.\n"
"Disabling script verification threads may result in a significant slow-down during synchronisation.\n"
"Has no effect on single core machines or if started with -par=<-<numcores>\n"

This comment has been minimized.

@promag

promag Apr 12, 2018

Member

Should be if started with -par=1?

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Apr 13, 2018

Couple comments on the concept:

  • Maybe add a time argument (e.g., disable for 5 hours)
  • Maybe add code to differentiate between sync and receiving a new block. I can see why you might want to background sync, but when you are getting a new block, the pause won't be that long and it is really good for the network if this is faster
  • Perhaps also good to control how many threads get used? E.g., use 2 threads out of 4
  • Why is this better than just restarting the node with fewer threads?
@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Apr 13, 2018

@promag

I think this is almost only useful when changed in the UI right?

I think its useful for bitcoind users as well. Assume you want to do another CPU intense task on your system, you could setscriptthreadsenabled false, do your task and setscriptthreadsenabled true.

@JeremyRubin
Agree with all your features... I guess they could be implemented in another PR.

Why is this better than just restarting the node with fewer threads?

Restarting Bitcoin-Core just to reduce it's script verification threads seems unideal. A restart is always painful (dbcache, partial re-validation).

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/04/svt branch to dfab6c6 Apr 18, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Apr 18, 2018

Needed rebase

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 21, 2018

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.