refactor: Drop boost::thread and boost::chrono

ken2812221 commented at 11:07 pm on October 15, 2018: contributor

Based on #14464 and #14480. Add interruptible thread class to replace boost::thread

The remaining usage of libboost_thread is boost::shared_mutex

fanquake added the label Refactoring on Oct 15, 2018

fanquake added this to the "In progress" column in a project

ken2812221 commented at 0:28 am on October 16, 2018: contributor

Apparently I wrote a MSVC-only code. I am going to investigate it.

ken2812221 renamed this:
~~refactor: Drop boost::thread and boost::chrono~~
[WIP] refactor: Drop boost::thread and boost::chrono
on Oct 16, 2018

meshcollider commented at 2:17 am on October 16, 2018: contributor

Related #8631

ken2812221 force-pushed on Oct 16, 2018

ken2812221 renamed this:
~~[WIP] refactor: Drop boost::thread and boost::chrono~~
refactor: Drop boost::thread and boost::chrono
on Oct 16, 2018

ken2812221 commented at 4:37 pm on October 16, 2018: contributor

This is ready for review

in src/threadinterrupt.cpp:72 in 95fd49ae4c outdated

67+    }
68+    std::lock_guard<std::mutex> lock(g_interrupt_flag->m_mutex);
69+    if (g_interrupt_flag->m_interrupted) {
70+        throw ThreadInterrupted();
71+    }
72+}

practicalswift commented at 10:01 am on October 17, 2018:

Missing newline at end of file :-)

in src/threadinterrupt.h:71 in 95fd49ae4c outdated

66+private:
67+    std::thread m_internal;
68+    std::unique_ptr<InterruptFlag> m_interrupt_flag;
69+};
70+
71+class ThreadInterrupted : std::exception

practicalswift commented at 10:26 am on October 17, 2018:

Is the nonpublic inheritance here intentional? If so, make the inheritance explicitly private and add a comment to clarify that this is intentional. If not, make it public :-)

Background:

ThreadInterrupted won’t be catched by say catch (const std::exception& e) since ThreadInterrupted has nonpublic inheritance from std::exception.

Example:

 0[cling]$ #include <iostream>
 1[cling]$ class PublicInheritanceException : public std::exception {};
 2[cling]$ try {
 3             throw PublicInheritanceException();
 4         } catch (const std::exception& e) {
 5             std::cout << "exception catched\n";
 6         }
 7exception catched
 8[cling]$ class PrivateByDefaultInheritanceException : std::exception {};
 9[cling]$ try {
10             throw PrivateByDefaultInheritanceException();
11         } catch (const std::exception& e) {
12             std::cout << "exception catched\n";
13         }
14*program terminates*

ken2812221 commented at 3:03 pm on October 17, 2018: contributor

@donaloconnor It should use mutex because it’s used by the condition_variable.

For reference, you can find boost implementation for pthread, it acquires lock before it reads interrupt_requested.

If it can drop InterruptibleSleep in the future, then it’s fine to switch to atomic.

ken2812221 force-pushed on Oct 17, 2018

ken2812221 force-pushed on Oct 18, 2018

donaloconnor commented at 7:50 am on October 18, 2018: contributor

@ken2812221 ah yes for the sleep. Thanks for the link to boosts impl.

ken2812221 force-pushed on Oct 18, 2018

ken2812221 commented at 11:36 am on October 18, 2018: contributor

Actually, I think it’s fine to make it use both atomic and mutex at the same time. Then we don’t have to lock mutex in InterruptionPoint.

ken2812221 force-pushed on Oct 18, 2018

donaloconnor commented at 11:52 am on October 18, 2018: contributor

In this case it might work but in practice it can cause race conditions. The value of the atomic can change from the moment the cond var is signalled and the predicate is checked (to work with spurious wakeups). Eg: false->true->notify+wakeup->false->predcheck.

The last false flag can never happen here (at least my understanding of the code) I guess so maybe it’s okay.

ken2812221 force-pushed on Oct 18, 2018

ken2812221 commented at 5:05 pm on October 18, 2018: contributor

@donaloconnor I am not sure people would allow this solution. Now the flag contains two variable:

atomic bool read in InterruptionPoint
non-atomic bool read in InterruptionSleep

donaloconnor commented at 10:18 pm on October 18, 2018: contributor

I think it’s not worth the complexity tbh. Sorry. I don’t know if the perf improvement is worth the extra complexity without measurements. Maybe we should just stick to what boost did as you had in original. I think using the atomic in the cond var is okay for this case because it’s state only goes from false->true.

I’d prefer if others chimed in to give feedback on this..

Re: spurious wakeups in the sleep. Do you think that’s an issue? The sleep could get ended prematurely. Probably not a big deal. I don’t know how much the code relies on accurate sleeps.

practicalswift commented at 10:47 pm on October 18, 2018: contributor

What about splitting the work in two parts?

Start with the low-risk Boost replacement: that change is likely worth it in itself as long as no deviations from what Boost was doing are introduced.

When that work is finished: continue with the risker performance improvement change you are considering.

Does that make sense?

ken2812221 force-pushed on Oct 19, 2018

ken2812221 force-pushed on Oct 20, 2018

DrahtBot commented at 9:55 am on October 20, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14624 (Some simple improvements to the RNG code by sipa)
#14605 (Return of the Banman by dongcarl)
#14252 (build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) by practicalswift)
#13910 (Trivial: Log progress while verifying blocks at level 4 by domob1812)
#13743 (refactor: Replace boost::bind with std::bind by ken2812221)
#13389 (Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() by n2yen)
#13382 (util: Don’t throw in GetTime{Millis,Micros}(). Mark as noexcept. by practicalswift)
#12980 (Allow quicker shutdowns during LoadBlockIndex() by jonasschnelli)
#12557 ([WIP] 64 bit iOs device support by Sjors)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ken2812221 force-pushed on Oct 20, 2018

ken2812221 force-pushed on Oct 27, 2018

ken2812221 force-pushed on Oct 30, 2018

refactor: Make CCheckQueue interruptible

This commit add a Interrupt function for CCheckQueue that it can handle interrupt by itself instead of relying on boost thread interrupt

b00312a91d

refactor: Drop boost in CCheckQueue

replace boost::mutex with debuggable Mutex
replace boost::condition_variable with std::condition_variable
add const specifier to fMaster and nBatchSize
add clang thread safety attributes
move init value of member of CCheckQueue from constructor to definition

cac4c12f7e

refactor: Make CCheckQueue worker threads be owned by itself

Manage CCheckQueue workder threads by itself. Do not expose the threads outside the object.

6bc643cad0

refactor: Drop boost::this_thread::interruption_point and boost::thread_interrupted in main thread 2c50583d35

utils: Add interruptible thread class cd1c7a8d51

tests: Add interruptible thread tests 88aa77a150

refactor: Replace boost::thread with InterruptibleThread f8d86b46c7

msvc: Use wildcard on src files for bench_bitcoin.exe 8f4cbf38a5

build: Remove boost::chrono from build system 692dee0963

ken2812221 force-pushed on Nov 2, 2018

DrahtBot added the label Needs rebase on Nov 5, 2018

DrahtBot commented at 11:16 am on November 5, 2018: member

ken2812221 closed this on Dec 3, 2018

ken2812221 deleted the branch on Dec 3, 2018

fanquake moved this from the "In progress" to the "Later" column in a project

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke added the label Up for grabs on Oct 30, 2019

MarcoFalke commented at 9:01 pm on March 6, 2020: member

fyi, Boost::Chrono has been removed

MarcoFalke removed the label Up for grabs on Jan 30, 2021

DrahtBot locked this on Aug 16, 2022

refactor: Drop boost::thread and boost::chrono #14489

Conflicts