Make gathering of dropped packets more efficient

[jstnlef]

Task Description

Over here we allocate a new vector for dropped packets. In a perfect scenario, this should be avoided.

Task TODO

There is likely a way to set this up to just return a lazy iterator to prevent the vector allocation. It's worth exploring.

• [ ] Investigate into different options to remove this allocation

[TimonPost]

Research if the following is possible: we could return a lazy loader here, instead of collecting, and do the same over here and then continue this lazy operator here.

[kstrafe]

Tried this with an impl Iterator but the problem is that we drain the sent_packets structure while at the same time iterating over its keys.

The workaround is to collect the keys into a Vec<u16>, but that's an unnecessary allocation. I think std should have a drain_if method.

[ncallaway]

@jstnlef It looks like this has been refactored since this issue was created, so that link is no longer good. Any chance you can find and link to the new place where we're now unnecessarily allocating the Vec?

[TimonPost]

@ncallaway the allocated vec can be found in the following places:

https://github.com/amethyst/laminar/blob/master/src/net/virtual_connection.rs#L404 https://github.com/amethyst/laminar/blob/master/src/infrastructure/acknowledgment.rs#L125

[chocolacula]

@BourgondAries:

Tried this with an impl Iterator but the problem is that we drain the sent_packets structure while at the same time iterating over its keys.

The workaround is to collect the keys into a Vec<u16>, but that's an unnecessary allocation. I think std should have a drain_if method.

It has drain_filter in nightly-only. Without it as an option we can use something like:

pub fn dropped_packets(&mut self) -> Vec<SentPacket> {
        let mut sent_sequences: Vec<SentPacket> = Vec::new();

        let remote_ack_sequence = self.remote_ack_sequence_num;
        self.sent_packets.retain(|key, value| {
            if sequence_less_than(*key, remote_ack_sequence)
                && remote_ack_sequence.wrapping_sub(*key) > REDUNDANT_PACKET_ACKS_SIZE
            {
                sent_sequences.push(value.clone());
                false
            } else {
                true
            }
        });

        sent_sequences
    }

But it still leads to necessary using clone() for each retaining value although without allocation of the vector with keys. If it is an appropriate solution while Rust API is stabilizing and merging into new release I can create PR.