refactor: make checkqueue manage the threads by itself (also removed some boost dependencies)

[ken2812221]

This PR removes use of the following object in checkqueue

• boost::condition_variable
• boost::mutex
• boost::unique_lock

Instead of using boost::thread_group::interrupt_all to trigger boost::thread_interrupted exception, it should call InterruptScriptCheck() or scriptcheckqueue.Interrupt() to interrupt the checkqueue thread.

Also, it adds three functions:

• StartScriptCheck()
• InterruptScriptCheck()
• StopScriptCheck()

for clearer logic for script check threads.

[practicalswift]

According to PR title boost/thread/condition_variable.hpp should go as well? :-)

[ken2812221]

I only remove them in checkqueue, the another place to use it is in scheduler.

[practicalswift]

Concept ACK

Towards a Boost free future! :-)

[Empact]

nit: could just call this interrupted

[Empact]

If an if only has a single-statement then-clause, it can appear
on the same line as the if, without braces. In every other case,
braces are required, and the then and else clauses must appear
correctly indented on a new line.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

[ken2812221]

clang-format doesn't catch this. Done manually.

[ch4ot1c]

Pluralize to threads

[ch4ot1c]

utACK

[sipa]

utACK 9aacef638f0c2159930640762daf524942052a9d

[donaloconnor]

utACK 9aacef6

[donaloconnor]

tiny nit: only one master thread so maybe condMaster.notify_one() is more consistent. No difference to code though.

[JeremyRubin]

Weak concept Ack.

Necessary step to get rid of boost, but conflicts with some unmerged work (#9938, JeremyRubin#1) which does the same thing.

Overall, this appears to be correct.

Before merge I'd like to see some benchmarks, the checkqueue code is very performance sensitive, sometimes the boost condition variables and mutexs difference from std's can be substantial. (I'm not kidding -- the time to wake-up and condition variable sleep behavior is one of the major performance issues with this code).

[JeremyRubin]

Add the condition as a callback to wait.

e.g., cond.wait(lock, [&] { return interrupted || !queue.empty(); })

[ken2812221]

Addressed comments

[ken2812221]

@JeremyRubin
benchmark result on my Ubuntu 18.04 virtual machine:
master 816fab9

# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 83.1845, 0.000461328, 0.000692877, 0.000668382

This PR 625c55a70c376af9a94a2bb118c39685d3b82b33

# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 66.268, 0.000451829, 0.000491473, 0.000473479

[JeremyRubin]

That's great, thanks for checking that -- much bigger change than I expected it to be.

Looking at the numbers, it seems not only to be faster on the microbenchmark, but more consistent.

This merits running two additional tests, if you have energy for it:

1. Time to reindex (with assumevalid up to last month or something so it doesn't take forever
2. Two nodes (one with this PR one without) connected behind a third node (on latest release), with debug bench set. Collect the logs on ConnectBlock.
   • Also collect a general system profile of which processes are using what on the cpu.

The second test is important because one thing I noticed when working on this code previously is that it was trivially easy to get speedups on this code at the expense of general performance. E.g., getting rid of condition variables (and just spinning until jobs came in) was much faster, but meant that the OS scheduler would be giving our CheckQueue workers run-time when there was nothing to do, impacting other processes on the computer (most likely because of causing more cache invalidation, rather than the impact of losing a few cycles of a process switch). Hence my suspicion: the 20% speedup is because the standard condition variables are not using system sleep (which plays better with the OS). This can actually negatively impact the process too as our main thread might be getting context switched out while we are trying to add jobs or something rather than waiting for notify.

[JeremyRubin]

I think you could assert(! (fMaster && nTodo == 0)) here.

If fMaster were ever true here, we would deadlock.

Hence, there is no point to check that...

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

[ken2812221]

After tested a few times, return false or somthing else? seem to make the node have to reindex next time.

[JeremyRubin]

The issue could be that when you interrupt, you don't want to make fMaster return false causing the block to be marked false (which could be written into cache somehow).

Is interrupt guaranteed to wait until the current job is done? Making it guaranteed to wait probably could help fix that behavior... One way of doing that is to separate out when you wake up if there's an active job or not, and don't allow to quit when there is an active job until it's done.

[JeremyRubin]

I'd recommend adapting the quitting logic from https://github.com/JeremyRubin/bitcoin/blob/aa847431e266fb08289edbac3665ac7c93a81b8a/src/checkqueue.h if you want a reference, which should be more or less correct (albeit with a lot of extensions you can ignore).

[JeremyRubin]

This version is not correct either if I'm correct about the block getting flushed with validity cached during shutdown.

Now you might incorrectly cache that a block was valid when it may not have been.

You need to finish processing the block, then shut down. Or, if a shutdown is triggered, abandon the block in the master (you could make a trivalent return: false, true, shutdown).

[JeremyRubin]

Actually I think that's not right, I think this version will just hang because nTodo will be != 0 and queue.empty() will be true, so the master would sleep and never get woken?

[ken2812221]

@JeremyRubin master would never wait/sleep if interrupted is true. It would keep looping until nTodo == 0. Oh, I know what you mean.

[JeremyRubin]

Your best bet IMO is to have a CheckQueueControl mutex which is separate from the CheckQueue mutex. Make interrupting get both...

[ken2812221]

@JeremyRubin
How about now? I think the logic here would be more clear. It would keep checking until finish.
If there are new tasks added into queue after the worker threads ended, it would be only master worker checking the tasks unfortunately. However I think this is better than interrupt the master thread. Alternatively, we can detach these worker threads.

I am going to test this to see if there is anything broken.

[JeremyRubin]

Where's the recursive lock?

[ken2812221]

In test_bitcoin, it constuct CCheckQueueControl and call Interrupt in same thread

[JeremyRubin]

Interrupt should be called in the destructor of the test which should happen after any queuecontrols are destructed though?

[ken2812221]

Interrupt should be called in the destructor of the test which should happen after any queuecontrols are destructed though?

I think it would be better to make the checkqueue to control their worker threads instead of calling Thread() in somewhere.

[maflcko]

Could try a git rebase -Xignore-all-space df36ddf9ce8a4dcf0d7f324e57a13abb6cf6a57c?

[practicalswift]

@ken2812221 Why the close? :-)

[ken2812221]

The CI was failed and I don't have time to find the bugs out recently, so I closed it, I might reopen this when I has more time working on this.