From 63764c429c1239b121875f2a5048d01728dd7c19 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 6 Sep 2022 14:38:00 +0300 Subject: [PATCH] use a sync.Pool for ACK frames --- connection.go | 7 ++++- .../ackhandler/received_packet_tracker.go | 18 ++++++------ internal/logutils/frame.go | 17 +++++++++++ internal/wire/ack_frame.go | 4 +-- internal/wire/ack_frame_pool.go | 24 +++++++++++++++ internal/wire/ack_frame_pool_test.go | 29 +++++++++++++++++++ 6 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 internal/wire/ack_frame_pool.go create mode 100644 internal/wire/ack_frame_pool_test.go diff --git a/connection.go b/connection.go index ce19fbb48c2..2acf16fdc97 100644 --- a/connection.go +++ b/connection.go @@ -1334,6 +1334,7 @@ func (s *connection) handleFrame(f wire.Frame, encLevel protocol.EncryptionLevel err = s.handleStreamFrame(frame) case *wire.AckFrame: err = s.handleAckFrame(frame, encLevel) + wire.PutAckFrame(frame) case *wire.ConnectionCloseFrame: s.handleConnectionCloseFrame(frame) case *wire.ResetStreamFrame: @@ -1952,7 +1953,11 @@ func (s *connection) logPacketContents(p *packetContents) { for _, f := range p.frames { frames = append(frames, logutils.ConvertFrame(f.Frame)) } - s.tracer.SentPacket(p.header, p.length, p.ack, frames) + var ack *logging.AckFrame + if p.ack != nil { + ack = logutils.ConvertAckFrame(p.ack) + } + s.tracer.SentPacket(p.header, p.length, ack, frames) } // quic-go logging diff --git a/internal/ackhandler/received_packet_tracker.go b/internal/ackhandler/received_packet_tracker.go index d2cabcbe025..43c7f09a786 100644 --- a/internal/ackhandler/received_packet_tracker.go +++ b/internal/ackhandler/received_packet_tracker.go @@ -171,16 +171,16 @@ func (h *receivedPacketTracker) GetAckFrame(onlyIfQueued bool) *wire.AckFrame { } } - ack := &wire.AckFrame{ - AckRanges: h.packetHistory.AppendAckRanges(nil), - // Make sure that the DelayTime is always positive. - // This is not guaranteed on systems that don't have a monotonic clock. - DelayTime: utils.Max(0, now.Sub(h.largestObservedReceivedTime)), - ECT0: h.ect0, - ECT1: h.ect1, - ECNCE: h.ecnce, - } + ack := wire.GetAckFrame() + ack.DelayTime = utils.Max(0, now.Sub(h.largestObservedReceivedTime)) + ack.ECT0 = h.ect0 + ack.ECT1 = h.ect1 + ack.ECNCE = h.ecnce + ack.AckRanges = h.packetHistory.AppendAckRanges(ack.AckRanges) + if h.lastAck != nil { + wire.PutAckFrame(h.lastAck) + } h.lastAck = ack h.ackAlarm = time.Time{} h.ackQueued = false diff --git a/internal/logutils/frame.go b/internal/logutils/frame.go index 6e0fd311b9f..c894be21d76 100644 --- a/internal/logutils/frame.go +++ b/internal/logutils/frame.go @@ -11,6 +11,10 @@ import ( // Furthermore, it removes the data slices from CRYPTO and STREAM frames. func ConvertFrame(frame wire.Frame) logging.Frame { switch f := frame.(type) { + case *wire.AckFrame: + // We use a pool for ACK frames. + // Implementations of the tracer interface may hold on to frames, so we need to make a copy here. + return ConvertAckFrame(f) case *wire.CryptoFrame: return &logging.CryptoFrame{ Offset: f.Offset, @@ -31,3 +35,16 @@ func ConvertFrame(frame wire.Frame) logging.Frame { return logging.Frame(frame) } } + +func ConvertAckFrame(f *wire.AckFrame) *logging.AckFrame { + ranges := make([]wire.AckRange, 0, len(f.AckRanges)) + ranges = append(ranges, f.AckRanges...) + ack := &logging.AckFrame{ + AckRanges: ranges, + DelayTime: f.DelayTime, + ECNCE: f.ECNCE, + ECT0: f.ECT0, + ECT1: f.ECT1, + } + return ack +} diff --git a/internal/wire/ack_frame.go b/internal/wire/ack_frame.go index dbbc7adc7ef..1ea8a23476d 100644 --- a/internal/wire/ack_frame.go +++ b/internal/wire/ack_frame.go @@ -29,7 +29,7 @@ func parseAckFrame(r *bytes.Reader, ackDelayExponent uint8, _ protocol.VersionNu } ecn := typeByte&0x1 > 0 - frame := &AckFrame{} + frame := GetAckFrame() la, err := quicvarint.Read(r) if err != nil { @@ -106,7 +106,7 @@ func parseAckFrame(r *bytes.Reader, ackDelayExponent uint8, _ protocol.VersionNu return frame, nil } -// Write writes an ACK frame. +// Append appends an ACK frame. func (f *AckFrame) Append(b []byte, _ protocol.VersionNumber) ([]byte, error) { hasECN := f.ECT0 > 0 || f.ECT1 > 0 || f.ECNCE > 0 if hasECN { diff --git a/internal/wire/ack_frame_pool.go b/internal/wire/ack_frame_pool.go new file mode 100644 index 00000000000..a0c6a21d7d3 --- /dev/null +++ b/internal/wire/ack_frame_pool.go @@ -0,0 +1,24 @@ +package wire + +import "sync" + +var ackFramePool = sync.Pool{New: func() any { + return &AckFrame{} +}} + +func GetAckFrame() *AckFrame { + f := ackFramePool.Get().(*AckFrame) + f.AckRanges = f.AckRanges[:0] + f.ECNCE = 0 + f.ECT0 = 0 + f.ECT1 = 0 + f.DelayTime = 0 + return f +} + +func PutAckFrame(f *AckFrame) { + if cap(f.AckRanges) > 4 { + return + } + ackFramePool.Put(f) +} diff --git a/internal/wire/ack_frame_pool_test.go b/internal/wire/ack_frame_pool_test.go new file mode 100644 index 00000000000..d28185953aa --- /dev/null +++ b/internal/wire/ack_frame_pool_test.go @@ -0,0 +1,29 @@ +package wire + +import ( + "math/rand" + "time" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("ACK Frame (for IETF QUIC)", func() { + It("gets an ACK frame from the pool", func() { + for i := 0; i < 100; i++ { + ack := GetAckFrame() + Expect(ack.AckRanges).To(BeEmpty()) + Expect(ack.ECNCE).To(BeZero()) + Expect(ack.ECT0).To(BeZero()) + Expect(ack.ECT1).To(BeZero()) + Expect(ack.DelayTime).To(BeZero()) + + ack.AckRanges = make([]AckRange, rand.Intn(10)) + ack.ECNCE = 1 + ack.ECT0 = 2 + ack.ECT1 = 3 + ack.DelayTime = time.Hour + PutAckFrame(ack) + } + }) +})