From c383bfc958502d3a4bbc3a2be9d192a75363f1c2 Mon Sep 17 00:00:00 2001
From: Cory Benfield <lukasa@apple.com>
Date: Fri, 3 May 2019 13:43:04 +0100
Subject: [PATCH] Avoid CoW when appending inbound frame bytes. (#111)

Motivation:

This is basically a similar issue to #104: we have a CoW data structure (ByteBuffer) in some of
these states, and when calling append on them they force an unnecessary CoW. We can reproduce the
fix from that patch, which is to define an empty state and swap into it before we run the append.
We also define a helper that ensures that we don't screw this up.

Modifications:

- Added new appending state to HTTP2FrameDecoder
- Added helpers for avoiding a CoW
- Avoided CoW in HTTP2FrameDecoder.

Result:

Fewer unnecessary copies
---
 Sources/NIOHTTP2/HTTP2FrameParser.swift | 83 +++++++++++++++++++++----
 1 file changed, 70 insertions(+), 13 deletions(-)

diff --git a/Sources/NIOHTTP2/HTTP2FrameParser.swift b/Sources/NIOHTTP2/HTTP2FrameParser.swift
index 7f63abaf..20cfd4c0 100644
--- a/Sources/NIOHTTP2/HTTP2FrameParser.swift
+++ b/Sources/NIOHTTP2/HTTP2FrameParser.swift
@@ -220,8 +220,11 @@ struct HTTP2FrameDecoder {
         /// We are accumulating a CONTINUATION frame.
         case accumulatingContinuationPayload(AccumulatingContinuationPayloadParserState)
 
-        // We are waiting for a new CONTINUATION frame to arrive.
+        /// We are waiting for a new CONTINUATION frame to arrive.
         case accumulatingHeaderBlockFragments(AccumulatingHeaderBlockFragmentsParserState)
+
+        /// A temporary state where we are appending data to a buffer. Must always be exited after the append operation.
+        case appending
     }
 
     internal var headerDecoder: HPACKDecoder
@@ -257,26 +260,41 @@ struct HTTP2FrameDecoder {
     mutating func append(bytes: inout ByteBuffer) {
         switch self.state {
         case .awaitingClientMagic(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .awaitingClientMagic(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .awaitingClientMagic(state)
+            }
         case .initialized:
+            // No need for the CoW helper here, as moveReaderIndex does not CoW.
             self.state = .accumulatingFrameHeader(AccumulatingFrameHeaderParserState(unusedBytes: bytes))
             bytes.moveReaderIndex(to: bytes.writerIndex)        // we ate all the bytes
         case .accumulatingFrameHeader(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .accumulatingFrameHeader(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .accumulatingFrameHeader(state)
+            }
         case .accumulatingData(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .accumulatingData(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .accumulatingData(state)
+            }
         case .simulatingDataFrames(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .simulatingDataFrames(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .simulatingDataFrames(state)
+            }
         case .accumulatingContinuationPayload(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .accumulatingContinuationPayload(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .accumulatingContinuationPayload(state)
+            }
         case .accumulatingHeaderBlockFragments(var state):
-            state.accumulate(bytes: &bytes)
-            self.state = .accumulatingHeaderBlockFragments(state)
+            self.avoidingParserCoW { newState in
+                state.accumulate(bytes: &bytes)
+                newState = .accumulatingHeaderBlockFragments(state)
+            }
+        case .appending:
+            preconditionFailure("Cannot recursively append in appending state")
         }
     }
 
@@ -542,6 +560,9 @@ struct HTTP2FrameDecoder {
             }
 
             self.state = .accumulatingContinuationPayload(AccumulatingContinuationPayloadParserState(fromAccumulatingHeaderBlockFragments: state, continuationHeader: header))
+
+        case .appending:
+            preconditionFailure("Attempting to process in appending state")
         }
 
         return .continue
@@ -1232,3 +1253,39 @@ private struct FrameFlags: OptionSet, CustomStringConvertible {
         return "[\(strings.joined(separator: ", "))]"
     }
 }
+
+
+// MARK: CoW helpers
+extension HTTP2FrameDecoder {
+    /// So, uh...this function needs some explaining.
+    ///
+    /// There is a downside to having all of the parser data in associated data on enumerations: any modification of
+    /// that data will trigger copy on write for heap-allocated data. That means that when we append data to the underlying
+    /// ByteBuffer we will CoW it, which is not good.
+    ///
+    /// The way we can avoid this is by using this helper function. It will temporarily set state to a value with no
+    /// associated data, before attempting the body of the function. It will also verify that the parser never
+    /// remains in this bad state.
+    ///
+    /// A key note here is that all callers must ensure that they return to a good state before they exit.
+    ///
+    /// Sadly, because it's generic and has a closure, we need to force it to be inlined at all call sites, which is
+    /// not ideal.
+    @inline(__always)
+    private mutating func avoidingParserCoW<ReturnType>(_ body: (inout ParserState) -> ReturnType) -> ReturnType {
+        self.state = .appending
+        defer {
+            assert(!self.isAppending)
+        }
+
+        return body(&self.state)
+    }
+
+    private var isAppending: Bool {
+        if case .appending = self.state {
+            return true
+        } else {
+            return false
+        }
+    }
+}