refactor: Drop boost::thread and boost::chrono #14489
Conversation
|
Apparently I wrote a MSVC-only code. I am going to investigate it. |
ken2812221
changed the title
|
Related #8631 |
ken2812221
changed the title
|
This is ready for review |
src/threadinterrupt.cpp
Outdated
There was a problem hiding this comment.
Missing newline at end of file :-)
src/threadinterrupt.h
Outdated
There was a problem hiding this comment.
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:
[cling]$ #include <iostream>
[cling]$ class PublicInheritanceException : public std::exception {};
[cling]$ try {
throw PublicInheritanceException();
} catch (const std::exception& e) {
std::cout << "exception catched\n";
}
exception catched
[cling]$ class PrivateByDefaultInheritanceException : std::exception {};
[cling]$ try {
throw PrivateByDefaultInheritanceException();
} catch (const std::exception& e) {
std::cout << "exception catched\n";
}
*program terminates*
|
@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 If it can drop InterruptibleSleep in the future, then it's fine to switch to atomic. |
|
@ken2812221 ah yes for the sleep. Thanks for the link to boosts impl. |
|
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. |
|
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. |
|
@donaloconnor I am not sure people would allow this solution. Now the flag contains two variable:
|
|
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. |
|
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? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This commit add a Interrupt function for CCheckQueue that it can handle interrupt by itself instead of relying on boost thread interrupt
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
Manage CCheckQueue workder threads by itself. Do not expose the threads outside the object.
…ad_interrupted in main thread
| Needs rebase |
|
fyi, Boost::Chrono has been removed |
8 participants
Based on #14464 and #14480. Add interruptible thread class to replace boost::thread
The remaining usage of libboost_thread is boost::shared_mutex