don't allocate a wire.Header when parsing short header packets #2928
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2928 +/- ##
==========================================
+ Coverage 86.22% 86.28% +0.06%
==========================================
Files 132 132
Lines 9141 9183 +42
==========================================
+ Hits 7881 7923 +42
Misses 910 910
Partials 350 350
Continue to review full report at Codecov.
|
marten-seemann
force-pushed
the
no-header-alloc-for-short-headers
branch
3 times, most recently
from
fb27af2
to
e4e8025
Compare
marten-seemann
force-pushed
the
no-header-alloc-for-short-headers
branch
from
e4e8025
to
ff69ca6
Compare
|
It's hard to judge whether the tradeoff between code complexity and performance is worth it here. Do you have some numbers on how much inefficiency the GC currently adds, and whether this improves throughput, latency, etc? |
| @@ -231,6 +231,32 @@ func (t *connectionTracer) SentPacket(hdr *wire.ExtendedHeader, packetSize proto | |||
| t.mutex.Unlock() | |||
| } | |||
|
|
|||
| func (t *connectionTracer) SentShortHeaderPacket(destConnID logging.ConnectionID, pn logging.PacketNumber, kp logging.KeyPhaseBit, size logging.ByteCount, ack *logging.AckFrame, frames []logging.Frame) { | |||
There was a problem hiding this comment.
If you're constructing the Header{} struct anyway, why not do that when calling this function? That would make things more symmetrical with the LongHeader version, and may even allow you to call the Short version from the Long function?
There was a problem hiding this comment.
Well, this is in the qlog package. I'm not worried about the additional allocation here. If you have qlog enabled, you can't expect maximum performance anyway.
| @@ -272,6 +272,25 @@ func (t *connectionTracer) ReceivedPacket(hdr *wire.ExtendedHeader, packetSize p | |||
| t.mutex.Unlock() | |||
| } | |||
|
|
|||
| func (t *connectionTracer) ReceivedShortHeaderPacket(destConnID logging.ConnectionID, pn logging.PacketNumber, kp logging.KeyPhaseBit, size protocol.ByteCount, frames []logging.Frame) { | |||
There was a problem hiding this comment.
As before: If you're constructing the Header{} struct anyway, why not do that when calling this function? That would make things more symmetrical with the LongHeader version, and may even allow you to call the Short version from the Long function?
| @@ -10,6 +10,11 @@ import ( | |||
| "github.com/lucas-clemente/quic-go/internal/utils" | |||
| ) | |||
|
|
|||
| // IsLongHeader says if a packet is a long header packet. | |||
| func IsLongHeader(firstByte byte) bool { | |||
|
We've recently resolved this as part of #3526. |
This PR is part of the ongoing effort to reduce allocations in the fast path (after the handshake, when application data is exchanged). While we already use a
sync.Poolfor STREAM frames, and #2885 adds one for ACK frames, we still allocate awire.ExtendedHeaderfor every incoming packet:https://github.com/lucas-clemente/quic-go/blob/325bc1694dd84af5d96a1d4d9db7e011ec6f3664/internal/wire/extended_header.go#L20-L31
which contains an embedded
wire.Headerhttps://github.com/lucas-clemente/quic-go/blob/325bc1694dd84af5d96a1d4d9db7e011ec6f3664/internal/wire/header.go#L48-L62
That's a pretty large data structure, especially considering that a QUIC short header packet only ever needs:
To avoid allocations, we can just define a function that returns
(protocol.KeyPhaseBit, protocol.ConnectionID, protocol.PacketNumber)instead of a fullwire.ExtendedHeader).Unfortunately, making this change ended up in a major refactoring, as we have many functions that use a
wire.ExtendedHeaderas a function argument and then distinguish between long and short header packets in the function body. These functions had to be split up into one version for long and one for short header packets.It will probably make sense to perform a similar refactoring for the code paths that send out packets.