Browse Source

Avoid allocations when accessing interceptor head/tail (#1039)

Motivation:

In some cases array will allocate when accessing first/last (SR-11262),
this happens to occur whenever we access the head or tail of the
interceptor list. This is rather unfortunate as this happens for every
single request/response part we see. Moreover, we know that if the
RPC is still active we will always have a head and a tail.

Modifications:

- Add an 'InterceptorContextList' smaller wrapper around the first and
  last (non-nil) elements and any others.
- Use the context list in the client and server interceptor pipelines.

Result:

- Approximately 3% improvement in QPS benchmarks
George Barnett 5 years ago
parent
commit
4d28b5f8a3

+ 48 - 39
Sources/GRPC/Interceptor/ClientInterceptorPipeline.swift

@@ -77,7 +77,7 @@ internal final class ClientInterceptorPipeline<Request, Response> {
   /// The contexts associated with the interceptors stored in this pipeline. Context will be removed
   /// once the RPC has completed. Contexts are ordered from outbound to inbound, that is, the tail
   /// is first and the head is last.
-  private var contexts: [ClientInterceptorContext<Request, Response>]?
+  private var contexts: InterceptorContextList<ClientInterceptorContext<Request, Response>>?
 
   /// Returns the next context in the outbound direction for the context at the given index, if one
   /// exists.
@@ -134,46 +134,16 @@ internal final class ClientInterceptorPipeline<Request, Response> {
   ) {
     self.eventLoop = eventLoop
     self.details = details
-
-    // We know we'll have at least a head and a tail as well as any user provided interceptors.
-    var contexts: [ClientInterceptorContext<Request, Response>] = []
-    contexts.reserveCapacity(interceptors.count + 2)
-
-    // Start with the tail.
-    contexts.append(
-      ClientInterceptorContext(
-        for: .tail(
-          for: self,
-          errorDelegate: errorDelegate,
-          onError: onError,
-          onResponsePart: onResponsePart
-        ),
-        atIndex: contexts.count,
-        in: self
-      )
-    )
-
-    // Now the user interceptors.
-    for interceptor in interceptors {
-      contexts.append(
-        ClientInterceptorContext(
-          for: .userProvided(interceptor),
-          atIndex: contexts.count,
-          in: self
-        )
-      )
-    }
-
-    // Finally, the head.
-    contexts.append(
-      ClientInterceptorContext(
-        for: .head(onCancel: onCancel, onRequestPart: onRequestPart),
-        atIndex: contexts.count,
-        in: self
-      )
+    self.contexts = InterceptorContextList(
+      for: self,
+      interceptors: interceptors,
+      errorDelegate: errorDelegate,
+      onError: onError,
+      onCancel: onCancel,
+      onRequestPart: onRequestPart,
+      onResponsePart: onResponsePart
     )
 
-    self.contexts = contexts
     self.setupDeadline()
   }
 
@@ -289,3 +259,42 @@ extension ClientInterceptorPipeline {
     }
   }
 }
+
+private extension InterceptorContextList {
+  init<Request, Response>(
+    for pipeline: ClientInterceptorPipeline<Request, Response>,
+    interceptors: [ClientInterceptor<Request, Response>],
+    errorDelegate: ClientErrorDelegate?,
+    onError: @escaping (Error) -> Void,
+    onCancel: @escaping (EventLoopPromise<Void>?) -> Void,
+    onRequestPart: @escaping (GRPCClientRequestPart<Request>, EventLoopPromise<Void>?) -> Void,
+    onResponsePart: @escaping (GRPCClientResponsePart<Response>) -> Void
+  ) where Element == ClientInterceptorContext<Request, Response> {
+    let middle = interceptors.enumerated().map { index, interceptor in
+      ClientInterceptorContext(
+        for: .userProvided(interceptor),
+        atIndex: index,
+        in: pipeline
+      )
+    }
+
+    let first = ClientInterceptorContext<Request, Response>(
+      for: .tail(
+        for: pipeline,
+        errorDelegate: errorDelegate,
+        onError: onError,
+        onResponsePart: onResponsePart
+      ),
+      atIndex: middle.startIndex - 1,
+      in: pipeline
+    )
+
+    let last = ClientInterceptorContext<Request, Response>(
+      for: .head(onCancel: onCancel, onRequestPart: onRequestPart),
+      atIndex: middle.endIndex,
+      in: pipeline
+    )
+
+    self.init(first: first, middle: middle, last: last)
+  }
+}

+ 37 - 33
Sources/GRPC/Interceptor/ServerInterceptorPipeline.swift

@@ -35,7 +35,7 @@ internal final class ServerInterceptorPipeline<Request, Response> {
   /// The contexts associated with the interceptors stored in this pipeline. Contexts will be
   /// removed once the RPC has completed. Contexts are ordered from inbound to outbound, that is,
   /// the head is first and the tail is last.
-  private var contexts: [ServerInterceptorContext<Request, Response>]?
+  private var contexts: InterceptorContextList<ServerInterceptorContext<Request, Response>>?
 
   /// Returns the next context in the outbound direction for the context at the given index, if one
   /// exists.
@@ -95,39 +95,12 @@ internal final class ServerInterceptorPipeline<Request, Response> {
     self.userInfoRef = userInfoRef
 
     // We need space for the head and tail as well as any user provided interceptors.
-    var contexts: [ServerInterceptorContext<Request, Response>] = []
-    contexts.reserveCapacity(interceptors.count + 2)
-
-    // Start with the head.
-    contexts.append(
-      ServerInterceptorContext(
-        for: .head(for: self, onResponsePart),
-        atIndex: contexts.count,
-        in: self
-      )
+    self.contexts = InterceptorContextList(
+      for: self,
+      interceptors: interceptors,
+      onRequestPart: onRequestPart,
+      onResponsePart: onResponsePart
     )
-
-    // User provided interceptors.
-    for interceptor in interceptors {
-      contexts.append(
-        ServerInterceptorContext(
-          for: .userProvided(interceptor),
-          atIndex: contexts.count,
-          in: self
-        )
-      )
-    }
-
-    // Now the tail.
-    contexts.append(
-      ServerInterceptorContext(
-        for: .tail(onRequestPart),
-        atIndex: contexts.count,
-        in: self
-      )
-    )
-
-    self.contexts = contexts
   }
 
   /// Emit a request part message into the interceptor pipeline.
@@ -160,3 +133,34 @@ internal final class ServerInterceptorPipeline<Request, Response> {
     self.contexts = nil
   }
 }
+
+private extension InterceptorContextList {
+  init<Request, Response>(
+    for pipeline: ServerInterceptorPipeline<Request, Response>,
+    interceptors: [ServerInterceptor<Request, Response>],
+    onRequestPart: @escaping (GRPCServerRequestPart<Request>) -> Void,
+    onResponsePart: @escaping (GRPCServerResponsePart<Response>, EventLoopPromise<Void>?) -> Void
+  ) where Element == ServerInterceptorContext<Request, Response> {
+    let middle = interceptors.enumerated().map { index, interceptor in
+      ServerInterceptorContext(
+        for: .userProvided(interceptor),
+        atIndex: index,
+        in: pipeline
+      )
+    }
+
+    let first = ServerInterceptorContext<Request, Response>(
+      for: .head(for: pipeline, onResponsePart),
+      atIndex: middle.startIndex - 1,
+      in: pipeline
+    )
+
+    let last = ServerInterceptorContext<Request, Response>(
+      for: .tail(onRequestPart),
+      atIndex: middle.endIndex,
+      in: pipeline
+    )
+
+    self.init(first: first, middle: middle, last: last)
+  }
+}

+ 57 - 0
Sources/GRPC/InterceptorContextList.swift

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2020, gRPC Authors All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/// A non-empty list which is guaranteed to have a first and last element.
+///
+/// This is required since we want to directly store the first and last elements: in some cases
+/// `Array.first` and `Array.last` will allocate: unfortunately this currently happens to be the
+/// case for the interceptor pipelines. Storing the `first` and `last` directly allows us to avoid
+/// this. See also: https://bugs.swift.org/browse/SR-11262.
+internal struct InterceptorContextList<Element> {
+  /// The first element, stored at `middle.startIndex - 1`.
+  internal var first: Element
+
+  /// The last element, stored at the `middle.endIndex`.
+  internal var last: Element
+
+  /// The other elements.
+  private var middle: [Element]
+
+  /// The index of `first`
+  private let firstIndex: Int
+
+  /// The index of `last`.
+  private let lastIndex: Int
+
+  internal subscript(checked index: Int) -> Element? {
+    switch index {
+    case self.firstIndex:
+      return self.first
+    case self.lastIndex:
+      return self.last
+    default:
+      return self.middle[checked: index]
+    }
+  }
+
+  internal init(first: Element, middle: [Element], last: Element) {
+    self.first = first
+    self.middle = middle
+    self.last = last
+    self.firstIndex = middle.startIndex - 1
+    self.lastIndex = middle.endIndex
+  }
+}