Prevent sending of ACK-only packets #1130
Conversation
Matthias247
force-pushed
the
pending_fix
branch
2 times, most recently
from
c632c86
to
c01f05c
Compare
|
Lints fail due to rustc 1.53 update. Should be fixed with #1147 |
Matthias247
force-pushed
the
pending_fix
branch
3 times, most recently
from
0d0956d
to
a49564a
Compare
| if pending.len() == 1 { | ||
| let mut first = pending.peek_mut().unwrap(); |
There was a problem hiding this comment.
Seems like we could do if let Some(first) = pending.peek_mut() or similar here, avoiding the unwrap?
| @@ -295,7 +295,10 @@ impl StreamsState { | |||
| } | |||
|
|
|||
| pub fn can_send(&self) -> bool { | |||
| !self.pending.is_empty() | |||
| match self.pending.peek() { | |||
There was a problem hiding this comment.
I replaced it with
self.pending
.peek()
.map(|head| !head.queue.borrow().is_empty())
.unwrap_or(false)
but I actually don't find it better, and I feel I gotten the advice to move to a match in the past
There was a problem hiding this comment.
Sorry, I was actually thinking of map_or. Does that help?
There was a problem hiding this comment.
Updated with that. Saves at least I line, but I overall still say those are all the same to me 🤷🏻♂️
| // Enqueue for a different level. If the current level is empty, drop it | ||
| if level.queue.borrow().is_empty() && num_levels != 1 { | ||
| // We keep the last level around even in empty form so that | ||
| // the next insert doesn't have to reallocate the queue | ||
| PeekMut::pop(level); |
There was a problem hiding this comment.
I think we really should find a way to avoid the three duplicated code sections.
There was a problem hiding this comment.
Remove the priorities again or don't care about performance :-)
While I think there might be ways to make this prettier, I'm not super inclined to investigate further in this. This is a rather performance-impacting bug that is open for 30 days, and I would rather like to see it fixed ASAP and spend energy into more impactful things than moving code around.
It's also not exactly duplicated.
The priority-based send state implementation had an issue where if returned `self.can_send() == true` even if no data was available for sending. This condition lead the `poll_transmit` method to initiate a transmission which passed the ACK-only check since it assumed there was stream data to send. However when trying to write stream frames no actual stream data could be written. The reason for this was that the `can_send()` condition only checked for the length of the binary heap. However the length of this one was only reduced in `write_stream_frames()` when the level was inspected the next time, and not after data of one level was actually written. This change fixes that, and drops the unused levels immediately after data was written. There is on exception however: If only one level is left, it is kept around and reused for future transmits to avoid deallocating and reallocating the transmit queue continuously. However the `can_send` check was updated to account for the empty level. I also added some additional debug asserts which cross-checks if an operation which announced to write more than just ACKs still ended up only writing ACKs. That should make it easier to determine similar issues in the future. **Ack only transmissions in benchmark before change:** 1462 **Ack only transmissions in benchmark after change:** 45
The priority-based send state implementation had an issue where if returned
self.can_send() == trueeven if no data was available for sending.This condition lead the
poll_transmitmethod to initiate a transmission whichpassed the ACK-only check since it assumed there was stream data to send.
However when trying to write stream frames no actual stream data could be
written.
The reason for this was that the
can_send()condition only checked for thelength of the binary heap. However the length of this one was only reduced
in
write_stream_frames()when the level was inspected the next time, andnot after data of one level was actually written.
This change fixes that, and drops the unused levels immediately after data
was written. There is on exception however: If only one level is left, it is
kept around and reused for future transmits to avoid deallocating and
reallocating the transmit queue continuously. However the
can_sendcheckwas updated to account for the empty level.
I also added some additional debug asserts which cross-checks
if an operation which announced to write more than just ACKs
still ended up only writing ACKs. That should make it easier to
determine similar issues in the future.
Ack only transmissions in benchmark before change: 1462
Ack only transmissions in benchmark after change: 45