Protocol cleanup (9): stream packetization #2956
Conversation
zmerp
force-pushed
the
protocol-cleanup
branch
2 times, most recently
from
d3cde78 to
fe2718b
Compare
| impl<H: Serialize> StreamSender<H> { | ||
| pub fn get_buffer(&mut self, header: &H) -> Result<Buffer<H>> { | ||
| let mut buffer = self.used_buffers.pop().unwrap_or_default(); | ||
| buffer.resize(self.payload_offset, 0); |
There was a problem hiding this comment.
I really don't know what the use of re-using buffers here would be, it feels like a premature optimization. If not using a really awful allocator (which I think isn't default on any target we have, and even then we could just change it), the allocator can do this recycling work just as efficiently for us.
It makes sense when we're trying to cache the allocation size for e.g. control packets cuz there we'd incur frequent resizes as the packet grows but here it's just one allocation either way. Especially because as it currently stands a big video buffer could be given to a small audio packet (which probably doesn't really waste that much either way because the allocator would cache it too, but it does make the memory less reusable and could increase the max memory usage).
The only thing that I could note for that is that it's kinda troublesome with non-constant size headers because those could actually grow which would blow up, but using the byte size of the struct should be a not too overblown upper bound for that size. In that realm it makes sense to have an api to directly get a buffer with the correct size, even tho copying around the prefix probably isn't really costly. (This is also relevant for the control socket btw, because in the sizes we're dealing with allocating 50 percent more really shouldn't incur extra costs and may just be more efficient than doing in house recycling).
There was a problem hiding this comment.
Especially because as it currently stands a big video buffer could be given to a small audio packet
Nope, buffers are recycled with per-stream queues.
As you said, we need to reallocate the vector multiple times while receiving shards, and copying the data multiple times. we may be able to do and optimization where we preallocate a sufficient size, for example the biggest packet that was sent. In the chance of an anomalous huge packet, this is bad, but also it's bad keeping just one of those huge buffers around. we may need to do some statistics, for example using a size of the 99% percentile, or trashing buffers that we consider having an anomalous size
| /// If the range is outside the valid range, new space will be allocated | ||
| /// NB: the offset parameter is applied on top of the internal offset of the buffer | ||
| #[must_use] | ||
| pub fn get_range_mut(&mut self, range: Range<usize>) -> &mut [u8] { |
There was a problem hiding this comment.
Since we're reusing buffers I think this could really use a comment that it starts off with zero non-prefix memory so calling this doesn't run risk to have unused memory at the end. I got confused by that for a while.
| fn wrapping_cmp(lhs: u32, rhs: u32) -> Ordering { | ||
| let diff = lhs.wrapping_sub(rhs); | ||
| if diff == 0 { | ||
| Ordering::Equal | ||
| } else if diff < u32::MAX / 2 { | ||
| Ordering::Greater | ||
| } else { | ||
| // if diff > u32::MAX / 2, it means the sub operation wrapped | ||
| Ordering::Less | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see how this is any better than a normal cmp, I'm pretty sure that normal cmp would have the wished effect. Especially because this isn't really correct, if lhs is u32::max and rhs is 1 then this will be wrong. And I really don't see what the special use of this is except horrible jank.
There was a problem hiding this comment.
Ah, for some reason I was using u32::MAX as an exclusive upper value, while it is inclusive. In any case we need this method. values near u32::MAX are functionally < than small values
There was a problem hiding this comment.
Actually the off by one error here is really inconsequential. we just need a rough midpoint of the u32 range. very unlikely we would get packets from the network with (wrapping) distance near u32::MAX/2, which is the point if this method. Mind that packets are numbered per-stream
alvr/sockets/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn socket_peek(socket: &mut Socket, buffer: &mut [u8]) -> ConResult<usize> { |
There was a problem hiding this comment.
pretty sure socket2 implements peek now, no need to have this.
There was a problem hiding this comment.
socket2 has been supporting peek for 5 years (checked on github blame). But if you notice the implementations are different. The premade implementation was missing some flags which was causing unnecessary errors and connection drops
zmerp
force-pushed
the
protocol-cleanup
branch
from
fe2718b to
1a70790
Compare
zmerp
force-pushed
the
protocol-cleanup
branch
2 times, most recently
from
299bac0 to
fb4b572
Compare
zmerp
force-pushed
the
protocol-cleanup
branch
3 times, most recently
from
c662089 to
9a39de7
Compare
zmerp
force-pushed
the
protocol-cleanup
branch
from
9a39de7 to
6b4b6ba
Compare
zmerp
force-pushed
the
protocol-cleanup
branch
from
6b4b6ba to
9f914f9
Compare
2 participants
Refactor Stream socket splitting UDP and TCP implementation, reducing the size of packets/shards prefixes
Also removes buffer initialization to 0 by using
MaybeUninitslices with socket2 at the receive side.Tested on Quest 3/Windows