Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 7, 2020

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

@maflcko maflcko force-pushed the 2003-schedTypeChrono branch from fa7759b to faa90a0 Compare March 7, 2020 14:15
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@practicalswift
Copy link
Contributor

Concept ACK

Thanks for making the code easier to read. That is appreciated by future generations of Core developers :)

Copy link
Contributor

@kallewoof kallewoof left a 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.

Copy link
Contributor

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).

Copy link
Contributor

@practicalswift practicalswift Mar 9, 2020

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()

Copy link
Contributor

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. :)

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Contributor

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.

@vasild
Copy link
Contributor

vasild commented Mar 8, 2020

ACK faa90a0 (code review, not tested)

Copy link
Contributor

@promag promag left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
scheduler.scheduleEvery([&] { this->DumpAddresses(); }, DUMP_PEERS_INTERVAL);
scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL);

Same below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same below.

Where exactly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@maflcko maflcko Mar 10, 2020

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

@maflcko maflcko force-pushed the 2003-schedTypeChrono branch from faa90a0 to fa5ffe1 Compare March 10, 2020 13:46
@maflcko maflcko force-pushed the 2003-schedTypeChrono branch from fa5ffe1 to fadafb8 Compare March 10, 2020 13:47
@maflcko
Copy link
Member Author

maflcko commented Mar 10, 2020

Addressed feedback by @vasild about the doxygen comment and feedback by @promag about the lambda stylistic syntax rewording. Also, added a commit to move the banman contstant to banman.

@vasild
Copy link
Contributor

vasild commented Mar 10, 2020

ACK fa36f3a (code review, not tested)

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 12, 2020

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

Conflicts

Reviewers, 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.

@ajtowns
Copy link
Contributor

ajtowns commented Mar 13, 2020

ACK fa36f3a

Copy link
Member

@jonatack jonatack left a 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
Copy link
Member

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?

Copy link
Member Author

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.
*/
Copy link
Member

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.
     */

Copy link
Member Author

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

@maflcko maflcko merged commit ce87d56 into bitcoin:master Mar 17, 2020
@maflcko maflcko deleted the 2003-schedTypeChrono branch March 17, 2020 20:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 18, 2020
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});
Copy link
Member Author

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

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 29, 2020
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
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants