Fix BankingStage packet starvation (1.9)

[buffalu]

Problem

• BankingStage gets starved from new packets if it gets stuck with transactions that exceeds the cost model or repeatedly hit AccountInUse/other errors that mark packets as retry-able that take awhile to get drained from the buffer.
• Issue: Performance degradation (small blocks) after updating from 1.8 to 1.9 #24163
• BankingStage should attempt to read in new packets more frequently.

Summary of Changes

• Remove the looping for process_buffered_packets.
• BankingStage will attempt to go through each threads backlog once before reading new packets.
• If there's packets, set the recv_timeout to 0s so we perform the equivalent of a try_recv

NOTE:

• We are still wasting time trying to pack batches for txs where the cost model is already saturated (serialization, QoS, account locking, etc.) but this seems like a major improvement over the status quo.

[buffalu]

from @carllin:

yeah I think have to make two decisions:

1. How to split the buffer for cost model throttled transactions that are stuck in PacketBatches in 1.9 vs 1.10/master where every packet is independent
2. How often to read from the sigverify pipe into BankingStage. Most straightforward decision is as @buffalu suggested and do one iteration of receive_and_buffer after one round of attempted execution

this should solve 2.

note that this doesn't solve 1, but it should definitely make starvation way better

[buffalu]

something worth discussing

[buffalu]

if you have one packet in buffered_packet_batches that exceeds cost model, it'll probably spin super fast grabbing this lock a ton, so might be good idea to rate limit it

[carllin]

yeah this is a good point, we can wrap it in a check that only checks this once every SLOT_BOUNDARY_CHECK_MS: usize = 10

[ckamm]

Does it even make sense to mark transactions that aren't processed due to cost limits as retryable in bank::load_and_execute_transactions()? Retrying in the same block can work, but only if some other tx's costs are later reverted for some reason. But most likely retrying in the same block would have the same result.

[buffalu]

Does it even make sense to mark transactions that aren't processed due to cost limits as retryable in bank::load_and_execute_transactions()? Retrying in the same block can work, but only if some other tx's costs are later reverted for some reason. But most likely retrying in the same block would have the same result.

ideally they should probably be moved somewhere else (maybe a separate buffer?) and on a new slot inserted back into banking stage at the front. perhaps spread across packet batches to try to get better parallelism as opposed to one batch?

thats probably an issue for a separate PR. if 1.10 coming out soon, i have a feeling this will be "good enough" for 1.9

[carllin]

This also has a poh lock grabbing inside, so i think it's worth wrapping this in a if !process_buffered_packets.is_empty() check

[buffalu]

ah yeah, that's probably something we should keep an eye on too. fixed this comment

[carllin]

yeah this is a good point, we can wrap it in a check that only checks this once every SLOT_BOUNDARY_CHECK_MS: usize = 10

[sakridge]

@carllin can you take a look when you get a chance?