Correct/Simplify the Ack algorithm #177
Conversation
jstnlef
force-pushed
the
correct-ack-algorithm
branch
2 times, most recently
from
1a1d465 to
1424690
Compare
src/infrastructure/acknowledgment.rs
Outdated
| /// Returns the ack_bitfield corresponding to which of the past 32 packets we've | ||
| /// successfully received. | ||
| pub fn ack_bitfield(&self) -> u32 { | ||
| let most_recent_remote_seq_num: u16 = self.remote_sequence_num().wrapping_sub(1); |
There was a problem hiding this comment.
I wanted to make this more specific in that it should be the sequence number that is furthest ahead rather than the last one we've received.
src/infrastructure/acknowledgment.rs
Outdated
| } | ||
|
|
||
| // TODO: Is this what we want? | ||
| // assert_eq!(handler.dropped_packets.len(), 25); |
There was a problem hiding this comment.
I think this is exactly what we want if we have 100 packets and we divide that by 4 we will get 25. So we should have 25 dropped packets over 100 packets. If you don't trust the modulo operation please try the following code out:
let mut counter = 1;
for i in 0..100 {
handler.process_outgoing(vec![1, 2, 3].as_slice(), OrderingGuarantee::None);
handler.sequence_number = i;
// dropping every 4th with modulo's
if counter == 4 {
println!("Dropping packet: {}", drop_count);
drop_count += 1;
counter = 1;
} else {
// We send them a packet
other.process_incoming(i, handler.remote_sequence_num(), handler.ack_bitfield());
// Skipped: other.process_outgoing
// And it makes it back
handler.process_incoming(i, other.remote_sequence_num(), other.ack_bitfield());
counter += 1;
}
}This will drop every fourth packet for sure.
There was a problem hiding this comment.
This goes back to the discussion we were having the other day on discord (plus I know how modulo works :P). The reason why we would only get 17 dropped packets in this case is that the algorithm waits until it sees 32 packets that haven't acked that sequence number. So at received packet 100, which is the last packet received from the remote host, we have an ack field which looks like 01110111011101110111011101110111. <- This is where those extra 8 packets are. We haven't yet determined that they have been dropped because they haven't "fallen off the end"
|
The original acking code has some nice tests by the way which you could use as a reference for this implementation. |
jstnlef
force-pushed
the
correct-ack-algorithm
branch
from
ec04e9c to
a11e475
Compare
jstnlef
force-pushed
the
correct-ack-algorithm
branch
from
8350266 to
421b0d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #177 +/- ##
=========================================
- Coverage 97.68% 97.5% -0.19%
=========================================
Files 27 25 -2
Lines 2071 2000 -71
=========================================
- Hits 2023 1950 -73
- Misses 48 50 +2
Continue to review full report at Codecov.
|
…ffer to handle edge cases
…tor allocation. Also removes the external_ack and local_ack files
…ave it generate on the fly when called
…acket_drop` method
Needed to ensure that we start remote_ack_sequence_num == remote_sequence_num(). Also needed to ensure that the keys of the hash are sorted before the dropped packets are collected
jstnlef
force-pushed
the
correct-ack-algorithm
branch
from
bc4d165 to
b5f1fe6
Compare
jstnlef
changed the title
|
I feel pretty good about this at this point. Would like to see what you guys think. |
There was a problem hiding this comment.
This is awesome work!
I have one comment which you can ignore or try to implement:
Can we somehow optimize the collecting of dropped packets?
We could, for example, return a lazy loader here, instead of collecting, and do the same over here and then continue this lazy operator here. I think that this could improve the code. What do you think, Is it worth it?
|
Not a bad idea. I'll see what I can do tonight 👍 |
|
Hrm. I think it would take a bit to get this set up as a lazy iterator. We can take it as a future improvement I think 👍 |
|
Right, let's get this merged and create an issue for it. |
|
Ticket here: #182 |
Honestly, I have a lot more tests to write before this goes out the door but I wanted to get some initial feedback on what I have currently.
Key highlights are -
u16::maxwas being used as a "null" value which means that we would have incorrect behavior.I also added some descriptive comments around why I'm calculating the fields as I am.