banking_stage: track VoteStorage size instead of costly iterator

[AshwinSekar]

LatestUnprocessedVotes uses a costly lock grabbing iterator to compute length, which is used in various parts of the banking stage.
Instead track this separately in an atomic

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (92ebf0f) to head (ac6c23e).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #743    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         851      851            
  Lines      230937   231012    +75     
========================================
+ Hits       189217   189333   +116     
+ Misses      41720    41679    -41     

[AshwinSekar]

we want to backport this to 1.17 / 1.18?

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[steviez]

Probably yes on the BP's. That being said, the last update I pushed was found because we had an integer overflow in a metrics function. I think the code is probably good as is but that does make me a little nervous

[behzadnouri]

This is a very leaky design. For example there is already get_entry function which returns a reference with interior mutability:
https://github.com/anza-xyz/agave/blob/8aa32172d/core/src/banking_stage/latest_unprocessed_votes.rs#L188-L194

So the caller can do:

latest_unprocessed_votes.get_entry(pubkey).unwrap().write().unwrap().take_vote()

which will break this bookkeeping.

The old len function was bad but there is no evidence that it has any significant impact in overall validator performance.

I would pretty much rather the old (slow but correct) code unless there is a better safer design.
The backports for sure are unnecessary because, again, there is no evidence that this will significantly impact any of the current performance issues.

[apfitzge]

1. there is evidence it was affecting performance. there are only so many cores on a machine, and this code has been shown to be at least partially responsible for the near 100% CPU usage of 2 cores.
2. is your concern with get_entry? it is an internal function, you can see the uses of this function in the file. there are only 3 uses.

[behzadnouri]

1. there is evidence it was affecting performance. there are only so many cores on a machine, and this code has been shown to be at least partially responsible for the near 100% CPU usage of 2 cores.

I already read the slack thread and I don't count that as evidence that this will impact performance in any significant form.
If the validator running with and without this code shows significant change in any of the important performance metrics, that would count as evidence.

2. is your concern with get_entry? it is an internal function, you can see the uses of this function in the file. there are only 3 uses.

It doesn't matter. The code changes and that is how the bugs creep in. That is what "leaky" design mean.

[AshwinSekar]

Probably yes on the BP's. That being said, the last update I pushed was found because we had an integer overflow in a metrics function. I think the code is probably good as is but that does make me a little nervous

A bit unfortunate that it was caught in the localnet metrics test first, we do have a lot of tests that exercise this pathway that took longer to run but eventually spotted the issue https://buildkite.com/anza/agave/builds/2374#_.

This is a very leaky design. For example there is already get_entry function which returns a reference with interior mutability:

This is a valid point and part of the hesitation in implementing this approach initially (see solana-labs#26722 (comment)).

I would pretty much rather the old (slow but correct) code unless there is a better safer design.

An alternative we could consider is to modify LatestValidatorVotePacket so that we can avoid the inner lock when computing length / # forwarded

struct VotePacket {
  vote_source: VoteSource,
  vote: Option<Arc<ImmutableDeserializedPacket>>,
  slot: Slot,
  timestamp: Option<UnixTimestamp>
}

struct LatestValidatorVotePacket {
  vote_packet: RwLock<VotePacket>,
  forwarded: AtomicBool,
  is_vote_taken: AtomicBool,
}

pub struct LatestUnprocessedVotes {
  latest_votes_per_pubkey: RwLock<HashMap<Pubkey, Arc<LatestValidatorVotePacket>>>
}

len() can be similar to before (minus having to grab the inner lock), and take_vote() / update_vote() can perform the bookkeeping through is_vote_taken without leaking it to LatestUnprocessedVotes

[alessandrod]

I already read the slack thread and I don't count that as evidence that this will impact performance in any significant form.
If the validator running with and without this code shows significant change in any of the important performance metrics, that would count as evidence.

What? You must have missed this?

This is on a staked devbox running on mnb. What more evidence do you need? We don't have metrics that compute time around these functions, which is the reason why this went unnoticed for so long. This "must be in metrics or doesn't exist" is such a terrible take I can't even.

[alessandrod]

It doesn't matter. The code changes and that is how the bugs creep in. That is what "leaky" design mean.

I agree with this. I haven't reviewed the PR yet but we got here because of a footgun, so we shouldn't be introducing more footguns, as unlikely as they are to be triggered.

[alessandrod]

I've tested this briefly on a staked validator and noticed this:

The overhead of counting votes is now gone, but we're doing a lot of channel recvs with a 0 timeout, which result in a lot of spin looping because the channel is often empty. If we could spin less it would be great.

[apfitzge]

@alessandrod please see #681. This should make the gossip thread spin less, by making it always wait.

Haven't had a chance to pick that up on a node yet, if you wanna pick the commit in mean-time could help us both 😄