Re-use VecDeque in run_socket_consume / run_listen
#5101
Conversation
cpubot
requested review from
behzadnouri,
gregcusack,
jeffwashington and
alessandrod
| @@ -2167,7 +2175,8 @@ impl ClusterInfo { | |||
| } | |||
| }) | |||
| }; | |||
| Ok(sender.send(packets)?) | |||
| packets.clear(); | |||
There was a problem hiding this comment.
would it be worthwhile to toss up a datapoint if packets was resized here, so we can know when it makes sense to bump CHANNEL_RECV_BUFFER_INITIAL_CAPACITY?
| @@ -2220,6 +2229,7 @@ impl ClusterInfo { | |||
| epoch_duration, | |||
| should_check_duplicate_instance, | |||
| )?; | |||
| packets.clear(); | |||
There was a problem hiding this comment.
looks mostly good. just a clarification question/confirmation on packets buffer size. no behavioral change here. all we are doing is preallocating the vecdeque and reusing it. Probably doesn't make sense to reduce the size of the packets buffer periodically since we are capped at about 1.66MB.
| @@ -2243,13 +2253,15 @@ impl ClusterInfo { | |||
| .build() | |||
| .unwrap(); | |||
| let mut epoch_specs = bank_forks.map(EpochSpecs::from); | |||
| let mut packets = VecDeque::with_capacity(CHANNEL_RECV_BUFFER_INITIAL_CAPACITY); | |||
There was a problem hiding this comment.
i see so previously we defined a new vecdeque with capacity 2 that had to grow/allocate memory every time run_socket_consume() was called. now it will grow and stay that size and get reused.
looks like the max size of the packets vecdeque would be 2 * MAX_GOSSIP_TRAFFIC, assuming worst case batch sizes (1 packet per batch). so approx: 2 * MAX_GOSSIP_TRAFFIC * 8 bytes/ptr / 1000000 ≈ 1.66MB. Doesn't seem too bad.
any thoughts here?
| @@ -2085,6 +2093,7 @@ impl ClusterInfo { | |||
| epoch_specs: Option<&mut EpochSpecs>, | |||
| receiver: &PacketBatchReceiver, | |||
| sender: &Sender<Vec<(/*from:*/ SocketAddr, Protocol)>>, | |||
| packets: &mut VecDeque<PacketBatch>, | |||
There was a problem hiding this comment.
nit: maybe rename packets to packet_buffer or something. everytime i read this i think the packets are getting passed into this function via packets instead of receiver.
| @@ -2180,12 +2189,12 @@ impl ClusterInfo { | |||
| thread_pool: &ThreadPool, | |||
| last_print: &mut Instant, | |||
| should_check_duplicate_instance: bool, | |||
| packets: &mut VecDeque<Vec<(/*from:*/ SocketAddr, Protocol)>>, | |||
There was a problem hiding this comment.
nit: same nit on naming convention. won't die on this hill but it is confusing to me.
Problem
A
VecDequeis allocated in each processing loop in bothrun_socket_consumeandrun_listen. Under high load, these dynamically resize to max capacity (num_packets > MAX_GOSSIP_TRAFFIC) and are immediately dropped after processing. This creates significant memory churn.Summary of Changes
A
VecDequeis pre-allocated outside the loops and reused on each iteration. This avoids the allocation / reallocation pressure caused by doing this in each loop.This change is broken out of #5065.