p2p: Eliminate atomic for m_last_getheaders_timestamp #25557
Conversation
src/net_processing.cpp
Outdated
| peer->m_last_getheaders_timestamp.store(NodeSeconds{}); | ||
| WITH_LOCK(peer->m_last_getheaders_mutex, peer->m_last_getheaders_timestamp = {}); |
There was a problem hiding this comment.
Why change NodeSeconds{} to {}?
m_last_getheaders_timestamp is of type
NodeClock::time_point aka
std::chrono::time_point<NodeClock> aka
std::chrono::time_point<std::chrono::system_clock>
while NodeSeconds is of type
std::chrono::time_point<NodeClock, std::chrono::seconds> aka
std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>.
So, the type of m_last_getheaders_timestamp omits the second template parameter. It defaults to std::chrono::system_clock::duration. From the documentation, it is std::chrono::duration<rep, period>, but I can't find where it is specified that std::chrono::system_clock::period is std::ratio<1>. It only says:
period -- a std::ratio type representing the tick period of the clock, in seconds
Even if that is ok, there is now inconsistency wrt how m_last_getheaders_timestamp is set - in its declaration NodeSeconds{} is used and here {} is used.
There was a problem hiding this comment.
NodeSeconds{}, as well as std::chrono::time_point<NodeClock>{} will construct a time point representing the clock's epoch, see https://en.cppreference.com/w/cpp/chrono/time_point/time_point. The duration (/time) since epoch is always zero, regardless of the durations period. So this should be fine as is.
There was a problem hiding this comment.
Alright, zero is always zero even if the period is std::ratio<1> for one and something else for the other.
This means that it should be possible to use just {} in the declaration for consistency, right?
-NodeClock::time_point m_last_getheaders_timestamp GUARDED_BY(m_last_getheaders_mutex) {NodeSeconds{}};
+NodeClock::time_point m_last_getheaders_timestamp GUARDED_BY(m_last_getheaders_mutex) {};There was a problem hiding this comment.
This means that it should be possible to use just {} in the declaration for consistency, right?
For atomic that isn't possible due to compiler bugs, but without atomic, it may be possible.
| if (current_time - peer.m_last_getheaders_timestamp.load() > HEADERS_RESPONSE_TIME) { | ||
| if (current_time - peer.m_last_getheaders_timestamp > HEADERS_RESPONSE_TIME) { | ||
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256())); | ||
| peer.m_last_getheaders_timestamp = current_time; |
There was a problem hiding this comment.
A similar result can be achieved without a mutex, in a lock-free manner:
// Only allow a new getheaders message to go out if we don't have a recent
// one already in-flight
- if (current_time - peer.m_last_getheaders_timestamp.load() > HEADERS_RESPONSE_TIME) {
+ auto last = peer.m_last_getheaders_timestamp.load();
+ if (current_time - last > HEADERS_RESPONSE_TIME &&
+ peer.m_last_getheaders_timestamp.compare_exchange_strong(last, current_time)) {
+
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256()));
- peer.m_last_getheaders_timestamp = current_time;
return true;
}Anyway, feel free to ignore. I think in this case using a mutex is also ok.
src/net_processing.cpp
Outdated
| const auto current_time = NodeClock::now(); | ||
|
|
||
| LOCK(peer.m_last_getheaders_mutex); |
There was a problem hiding this comment.
Is it not better to first lock the mutex and then acquire the current time, because locking the mutex can take "long" time and then we would operate on an outdated current_time?
There was a problem hiding this comment.
Concept ACK removing atomic (see my rationale: #25454 (comment))
Though, I don't think a Mutex is needed either? #25454 (comment)
This variable is only used in a single thread, so no atomic or mutex is necessary to guard it.
8b09242 to
613e221
Compare
|
Updated the first commit to remove the mutex as well, since I had it wrong that this variable was being accessed in multiple threads. |
|
review ACK 613e221 This brings it consistently in line with other members that are only used in one thread, such as |
Eliminate the unnecessary atomic guarding
m_last_getheaders_timestamp, which is only accessed in a single thread (thanks to MarcoFalke for pointing this out).Also address a nit that came up in #25454.