From da54baaba80a3175b65422fa5a61525f4b6d6d6c Mon Sep 17 00:00:00 2001
From: Cory Benfield <lukasa@apple.com>
Date: Fri, 7 Aug 2020 14:01:50 +0100
Subject: [PATCH] Don't construct unnecessary buffers. (#925)

Motivation:

Currently the HTTP1ToGRPCServerCodec constructs a CircularBuffer that it
only ever uses when doing grpc-web work. This is not the end of the
world, except that CircularBuffers require memory allocations: there is
no such thing as an empty CircularBuffer.

In general it's not a great idea to force the mainline grpc use-case to
pay for the cost of constructing a circular buffer it will never use, so
instead we replace this with an optional circular buffer. That optional
will be initialized on first use, so it doesn't add any complexity to
our state tracking. It does add some subtle complexity to the rest of
the grpc-web code, though, so we should consider increasing the priority
of dealing with grpc-web properly.

Modifications:

- Make HTTP1ToGRPCServerCodec.responseTextBuffers optional, delay its
  initialization.

Results:

Small performance improvement on HTTP/2 server benchmarks, around 0.7%
per RPC.
---
 Sources/GRPC/HTTP1ToGRPCServerCodec.swift | 31 ++++++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/Sources/GRPC/HTTP1ToGRPCServerCodec.swift b/Sources/GRPC/HTTP1ToGRPCServerCodec.swift
index 0225d21d5..4ad41f6c8 100644
--- a/Sources/GRPC/HTTP1ToGRPCServerCodec.swift
+++ b/Sources/GRPC/HTTP1ToGRPCServerCodec.swift
@@ -82,7 +82,7 @@ public final class HTTP1ToGRPCServerCodec {
   // TODO(kaipi): Extract all gRPC Web processing logic into an independent handler only added on
   // the HTTP1.1 pipeline, as it's starting to get in the way of readability.
   private var requestTextBuffer: NIO.ByteBuffer!
-  private var responseTextBuffers: CircularBuffer<ByteBuffer> = []
+  private var responseTextBuffers: CircularBuffer<ByteBuffer>?
 
   var inboundState = InboundState.expectingHeaders {
     willSet {
@@ -336,7 +336,7 @@ extension HTTP1ToGRPCServerCodec: ChannelOutboundHandler {
             allocator: context.channel.allocator,
             compressed: messageContext.compressed
           )
-          self.responseTextBuffers.append(buffer)
+          self.appendResponseText(buffer)
 
           // Since we stored the written data, mark the write promise as successful so that the
           // ServerStreaming provider continues sending the data.
@@ -381,21 +381,31 @@ extension HTTP1ToGRPCServerCodec: ChannelOutboundHandler {
         trailersBuffer.writeInteger(UInt8(0x80))
         trailersBuffer.writeInteger(UInt32(textTrailers.utf8.count))
         trailersBuffer.writeString(textTrailers)
-        self.responseTextBuffers.append(trailersBuffer)
+        self.appendResponseText(trailersBuffer)
 
-        // The '!' is fine, we know it's not empty since we just added a buffer.
-        var responseTextBuffer = self.responseTextBuffers.popFirst()!
+        // This code can only be called on the grpc-web path, so we know the response text buffers must be non-nil
+        // and must contain at least one element.
+        guard var buffers = self.responseTextBuffers else {
+          preconditionFailure("Building web response text but responseTextBuffers are nil")
+        }
+
+        // Avoid a CoW
+        self.responseTextBuffers = nil
+        var responseTextBuffer = buffers.popFirst()!
 
         // Read the data from the first buffer.
         var accumulatedData = responseTextBuffer.readData(length: responseTextBuffer.readableBytes)!
 
         // Reserve enough capacity and append the remaining buffers.
-        let requiredExtraCapacity = self.responseTextBuffers.lazy.map { $0.readableBytes }.reduce(0, +)
+        let requiredExtraCapacity = buffers.lazy.map { $0.readableBytes }.reduce(0, +)
         accumulatedData.reserveCapacity(accumulatedData.count + requiredExtraCapacity)
-        while let buffer = self.responseTextBuffers.popFirst() {
+        while let buffer = buffers.popFirst() {
           accumulatedData.append(contentsOf: buffer.readableBytesView)
         }
 
+        // Restore the buffers.
+        self.responseTextBuffers = buffers
+
         // TODO: Binary responses that are non multiples of 3 will end = or == when encoded in
         // base64. Investigate whether this might have any effect on the transport mechanism and
         // client decoding. Initial results say that they are innocuous, but we might have to keep
@@ -429,6 +439,13 @@ extension HTTP1ToGRPCServerCodec: ChannelOutboundHandler {
       self.inboundState = .ignore
     }
   }
+
+  private func appendResponseText(_ buffer: ByteBuffer) {
+    if self.responseTextBuffers == nil {
+      self.responseTextBuffers = CircularBuffer()
+    }
+    self.responseTextBuffers!.append(buffer)
+  }
 }
 
 fileprivate extension HTTP1ToGRPCServerCodec {