p2p: Eliminate atomic for m_last_getheaders

sdaftuar commented at 6:37 pm on July 6, 2022: member

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.

DrahtBot added the label P2P on Jul 6, 2022

in src/net_processing.cpp:3981 in 8b09242cfb outdated

3977@@ -3974,7 +3978,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3978 
3979         // Assume that this is in response to any outstanding getheaders
3980         // request we may have sent, and clear out the time of our last request
3981-        peer->m_last_getheaders_timestamp.store(NodeSeconds{});
3982+        WITH_LOCK(peer->m_last_getheaders_mutex, peer->m_last_getheaders_timestamp = {});

vasild commented at 10:53 am on July 11, 2022:

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.

MarcoFalke commented at 12:55 pm on July 11, 2022:

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.

vasild commented at 1:30 pm on July 11, 2022:

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?

0-NodeClock::time_point m_last_getheaders_timestamp GUARDED_BY(m_last_getheaders_mutex) {NodeSeconds{}};
1+NodeClock::time_point m_last_getheaders_timestamp GUARDED_BY(m_last_getheaders_mutex) {};

MarcoFalke commented at 3:04 pm on July 11, 2022:

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.

in src/net_processing.cpp:2281 in 8b09242cfb outdated

2281     // Only allow a new getheaders message to go out if we don't have a recent
2282     // one already in-flight
2283-    if (current_time - peer.m_last_getheaders_timestamp.load() > HEADERS_RESPONSE_TIME) {
2284+    if (current_time - peer.m_last_getheaders_timestamp > HEADERS_RESPONSE_TIME) {
2285         m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256()));
2286         peer.m_last_getheaders_timestamp = current_time;

vasild commented at 11:59 am on July 11, 2022:

A similar result can be achieved without a mutex, in a lock-free manner:

 0     // Only allow a new getheaders message to go out if we don't have a recent
 1     // one already in-flight
 2-    if (current_time - peer.m_last_getheaders_timestamp.load() > HEADERS_RESPONSE_TIME) {
 3+    auto last = peer.m_last_getheaders_timestamp.load();
 4+    if (current_time - last > HEADERS_RESPONSE_TIME &&
 5+        peer.m_last_getheaders_timestamp.compare_exchange_strong(last, current_time)) {
 6+
 7         m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256()));
 8-        peer.m_last_getheaders_timestamp = current_time;
 9         return true;
10     }

Anyway, feel free to ignore. I think in this case using a mutex is also ok.

in src/net_processing.cpp:2279 in 8b09242cfb outdated

2275@@ -2274,9 +2276,11 @@ bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& loc
2276 
2277     const auto current_time = NodeClock::now();
2278 
2279+    LOCK(peer.m_last_getheaders_mutex);

vasild commented at 12:07 pm on July 11, 2022:

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?

MarcoFalke approved

MarcoFalke commented at 3:08 pm on July 11, 2022: member

Concept ACK removing atomic (see my rationale: #25454 (review))

Though, I don’t think a Mutex is needed either? #25454 (review)

Remove atomic for m_last_getheaders_timestamp

This variable is only used in a single thread, so no atomic or mutex is
necessary to guard it.

e939cf2b76

test: remove unnecessary parens 613e221149

sdaftuar force-pushed on Jul 12, 2022

sdaftuar commented at 5:40 pm on July 12, 2022: member

Updated the first commit to remove the mutex as well, since I had it wrong that this variable was being accessed in multiple threads.

sdaftuar renamed this:
~~p2p: Replace atomic with a mutex for m_last_getheaders_timestamp~~
p2p: Eliminate atomic for m_last_getheaders_timestamp
on Jul 12, 2022

vasild approved

vasild commented at 6:11 am on July 13, 2022: contributor

ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73

MarcoFalke commented at 6:24 am on July 13, 2022: member

review ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73

This brings it consistently in line with other members that are only used in one thread, such as m_addr_token_bucket for example.

MarcoFalke merged this on Jul 14, 2022

MarcoFalke closed this on Jul 14, 2022

sidhujag referenced this in commit 4a969ffa9b on Jul 14, 2022

bitcoin locked this on Jul 14, 2023

p2p: Eliminate atomic for m_last_getheaders_timestamp #25557