v2.0: removes early return if prune_messages are empty (backport of #3006) #3011
Conversation
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
behzadnouri
added
the
automerge
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
Even if there are no outgoing prune messages we still need to generate outgoing push messages for packets just received, so the code should not early return here: https://github.com/anza-xyz/agave/blob/d2cc71f0d/gossip/src/cluster_info.rs#L2400-L2402 (cherry picked from commit ce15821)
willhickey
force-pushed
the
mergify/bp/v2.0/pr-3006
branch
from
747d659
to
8289207
Compare
behzadnouri
added
the
automerge
| @@ -2444,7 +2441,9 @@ impl ClusterInfo { | |||
| self.stats | |||
| .packets_sent_push_messages_count | |||
| .add_relaxed((packet_batch.len() - num_prune_packets) as u64); | |||
| let _ = response_sender.send(packet_batch); | |||
| if !packet_batch.is_empty() { | |||
There was a problem hiding this comment.
Should all the stats counter increases be surrounded by this if as well?
There was a problem hiding this comment.
hmmm, not sure it matters much either way:
- the stats counters are just doing an atomic add, and even if the argument is zero it wouldn't really have a measurable impact.
- except for the prune counters, I would expect the other counters to be almost always non-zero anyways. i.e. we always push some messages out when receiving push messages.
Given that, I lean towards keeping the if branch shorter and simpler here. But if we decide to change that, because this is a backport, I would rather to do so on master branch first otherwise v2.0 becomes inconsistent with master.
There was a problem hiding this comment.
assuming the question was asked wrt the metrics remaining accurate with this change?
There was a problem hiding this comment.
The metrics are accurate.
My understanding was that the comment was about avoiding metric.add_relaxed(0) which is a no-op.
There was a problem hiding this comment.
Yep, trying to avoid some wasted time on no-ops and make it match ping handling a little more closely https://github.com/anza-xyz/agave/blob/master/gossip/src/cluster_info.rs#L2561-L2571
But I don't see anything functionally wrong, and agree this isn't turning the needle on performance in any meaningful way.
Problem
Even if there are no outgoing prune messages we still need to generate outgoing push messages for packets just received, so the code should not early return here:
https://github.com/anza-xyz/agave/blob/d2cc71f0d/gossip/src/cluster_info.rs#L2400-L2402
Summary of Changes
removes early return if
prune_messagesare emptyThis is an automatic backport of pull request #3006 done by Mergify.