-
Notifications
You must be signed in to change notification settings - Fork 38.8k
refactor: Make scheduler methods type safe #18289
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
Conversation
"Initializing the members in the declaration makes it easy to spot uninitialized ones". https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures
fa7759b to
faa90a0
Compare
ajtowns
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
|
Concept ACK Thanks for making the code easier to read. That is appreciated by future generations of Core developers :) |
kallewoof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK; light code review.
src/scheduler.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: const std::chrono::milliseconds& to avoid copy (here and below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit on nit:
I'm afraid that might be a pessimisation :)
From a performance perspective the typical rule-of-thumb recommendation is to pass "in" parameters by value (instead of reference to const) if the parameter has a size up to 16 bytes ("cheaply-copied" types).
Thus in this case the rule-of-thumb recommendation would be to pass by value instead of reference to const.
(Disclaimer: As always with optimisations -- if it is worth trying to optimise then it is worth trying to quantify the impact of the optimisation by measuring! :))
From the C++ Core Guidelines (Stroustrup & Sutter):
F.16: For “in” parameters, pass cheaply-copied types by value and others by reference to const
Reason: Both let the caller know that a function will not modify the argument, and both allow initialization by rvalues.
What is “cheap to copy” depends on the machine architecture, but two or three words (doubles, pointers, references) are usually best passed by value. When copying is cheap, nothing beats the simplicity and safety of copying, and for small objects (up to two or three words) it is also faster than passing by reference because it does not require an extra indirection to access from the function.
Examples:
void f1(const string& s); // OK: pass by reference to const; always cheap void f2(string s); // bad: potentially expensive void f3(int x); // OK: Unbeatable void f4(const int& x); // bad: overhead on access in f4()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels unstable to me to depend on architecture and/or implementation rather than just go for const x& when you can, but it's a nit; I'll live. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::chrono::milliseconds is just a 64-bit int (well, 45-bit or greater int), so passing it as const & means you're passing a 64-bit pointer instead, and have to deref it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving as is for now, to not invalidate review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other feedback aside, I prefer the copy here over a reference because other arguments (like the function) must be copied anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, copy seems sensible after all.
|
ACK faa90a0 (code review, not tested) |
promag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK faa90a0f933f574aa3304df7616778fc01009f51.
src/net.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| scheduler.scheduleEvery([&] { this->DumpAddresses(); }, DUMP_PEERS_INTERVAL); | |
| scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL); |
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same below.
Where exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consensusParams needs to be passed in, not just this
faa90a0 to
fa5ffe1
Compare
fa5ffe1 to
fadafb8
Compare
|
ACK fa36f3a (code review, not tested) |
|
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. |
|
ACK fa36f3a |
jonatack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa36f3a
A couple of comments if you retouch.
|
|
||
| // NOTE: When adjusting this, update rpcnet:setban's help ("24h") | ||
| static constexpr unsigned int DEFAULT_MISBEHAVING_BANTIME = 60 * 60 * 24; // Default 24-hour ban | ||
| // How often to dump addresses to banlist.dat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as a Doxygen comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but leaving as is for now
| * | ||
| * The timing is not exact: Every time f is finished, it is rescheduled to run again after delta. If you need more | ||
| * accurate scheduling, don't use this method. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
/**
* Repeat f indefinitely until the scheduler is stopped. The first run
* occurs after delta time.
*
* The timing is not exact; every time f finishes, it is scheduled again
* to run delta time later. If you need more accurate scheduling, don't
* use this method.
*/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might take, if I have to force push for other reasons
fa36f3a refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke) fadafb8 scheduler: Make schedule* methods type safe (MarcoFalke) fa70ccc scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke) Pull request description: Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}` ACKs for top commit: vasild: ACK fa36f3a (code review, not tested) ajtowns: ACK fa36f3a jonatack: ACK fa36f3a Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
| // timer. | ||
| static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer"); | ||
| scheduler.scheduleEvery(std::bind(&PeerLogicValidation::CheckForStaleTipAndEvictPeers, this, consensusParams), EXTRA_PEER_CHECK_INTERVAL * 1000); | ||
| scheduler.scheduleEvery([&] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The referenced global consensusParams can change in tests, so it needs to be copied. Fixed in #18376
Summary: Backport of core [[bitcoin/bitcoin#18289 | PR18289]]. This diff extends the original PR a bit due to avalanche. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6764
fa36f3a refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke) fadafb8 scheduler: Make schedule* methods type safe (MarcoFalke) fa70ccc scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke) Pull request description: Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}` ACKs for top commit: vasild: ACK fa36f3a (code review, not tested) ajtowns: ACK fa36f3a jonatack: ACK fa36f3a Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8

Main benefit is that stuff like
15 * 60 * 1000is replaced byminutes{15}