From 529d6914cae183c4cf45b1a90890b556ac4a2262 Mon Sep 17 00:00:00 2001 From: George Barnett Date: Wed, 21 Jul 2021 15:13:55 +0100 Subject: [PATCH] Use sync options to get the http2 stream ID (#1229) Motivation: NIO added API for getting channel options synchronously. NIO HTTP/2 added support for this in 1.17.0. When configuring a server we configure our new stream channels to get the stream ID to attach to the loggers. We can now do this synchronously and save some allocations. Modifications: - Bump minimum H2 version to 1.17.0 - Use sync options to get the stream ID and sync pipeline operations to add the handler. - Modify alloc-limits.sh so format better aligns with new CI yaml Result: Fewer allocations. --- .github/workflows/ci.yaml | 20 ++++++++-------- Package.swift | 2 +- .../GRPC/GRPCServerPipelineConfigurator.swift | 24 +++++++++++-------- scripts/alloc-limits.sh | 2 +- 4 files changed, 26 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 41187b73a..592e5f48d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -51,28 +51,28 @@ jobs: include: - image: swift:5.4-focal env: - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 515000 - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 227000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 503000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 215000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_10_small_requests: 112000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_1_small_request: 67000 MAX_ALLOCS_ALLOWED_embedded_server_unary_1k_rpcs_1_small_request: 63000 - MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 216000 + MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 204000 - image: swift:5.3-focal env: - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 515000 - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 227000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 504000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 216000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_10_small_requests: 112000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_1_small_request: 67000 MAX_ALLOCS_ALLOWED_embedded_server_unary_1k_rpcs_1_small_request: 63000 - MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 216000 + MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 205000 - image: swift:5.2-bionic env: - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 526000 - MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 229000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_10_requests: 515000 + MAX_ALLOCS_ALLOWED_bidi_1k_rpcs_1_request: 218000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_10_small_requests: 112000 MAX_ALLOCS_ALLOWED_embedded_server_bidi_1k_rpcs_1_small_request: 67000 MAX_ALLOCS_ALLOWED_embedded_server_unary_1k_rpcs_1_small_request: 63000 - MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 217000 + MAX_ALLOCS_ALLOWED_unary_1k_ping_pong: 206000 name: Performance Tests on ${{ matrix.image }} runs-on: ubuntu-latest container: @@ -82,4 +82,4 @@ jobs: - name: 🧮 Allocation Counting Tests run: ./Performance/allocations/test-allocation-counts.sh env: ${{ matrix.env }} - timeout-minutes: 20 \ No newline at end of file + timeout-minutes: 20 diff --git a/Package.swift b/Package.swift index 6b2d10111..dabf92d09 100644 --- a/Package.swift +++ b/Package.swift @@ -28,7 +28,7 @@ let package = Package( // Main SwiftNIO package .package(url: "https://github.com/apple/swift-nio.git", from: "2.28.0"), // HTTP2 via SwiftNIO - .package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.16.1"), + .package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.17.0"), // TLS via SwiftNIO .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.14.0"), // Support for Network.framework where possible. diff --git a/Sources/GRPC/GRPCServerPipelineConfigurator.swift b/Sources/GRPC/GRPCServerPipelineConfigurator.swift index 88e129301..156123efc 100644 --- a/Sources/GRPC/GRPCServerPipelineConfigurator.swift +++ b/Sources/GRPC/GRPCServerPipelineConfigurator.swift @@ -103,18 +103,22 @@ final class GRPCServerPipelineConfigurator: ChannelInboundHandler, RemovableChan channel: channel, targetWindowSize: self.configuration.httpTargetWindowSize ) { stream in - // TODO: use sync options when NIO HTTP/2 support for them is released - // https://github.com/apple/swift-nio-http2/pull/283 - stream.getOption(HTTP2StreamChannelOptions.streamID).map { streamID -> Logger in - logger[metadataKey: MetadataKey.h2StreamID] = "\(streamID)" - return logger - }.recover { _ in - logger[metadataKey: MetadataKey.h2StreamID] = "" - return logger - }.flatMap { logger in + // Sync options were added to the HTTP/2 stream channel in 1.17.0 (we require at least this) + // so this shouldn't be `nil`, but it's not a problem if it is. + let http2StreamID = try? stream.syncOptions?.getOption(HTTP2StreamChannelOptions.streamID) + let streamID = http2StreamID.map { streamID in + return String(Int(streamID)) + } ?? "" + + logger[metadataKey: MetadataKey.h2StreamID] = "\(streamID)" + + do { // TODO: provide user configuration for header normalization. let handler = self.makeHTTP2ToRawGRPCHandler(normalizeHeaders: true, logger: logger) - return stream.pipeline.addHandler(handler) + try stream.pipeline.syncOperations.addHandler(handler) + return stream.eventLoop.makeSucceededVoidFuture() + } catch { + return stream.eventLoop.makeFailedFuture(error) } } } diff --git a/scripts/alloc-limits.sh b/scripts/alloc-limits.sh index 5c66b3d39..dbbf2ffdd 100755 --- a/scripts/alloc-limits.sh +++ b/scripts/alloc-limits.sh @@ -30,5 +30,5 @@ grep 'test_.*\.total_allocations: ' \ | sed 's/^test_/MAX_ALLOCS_ALLOWED_/' \ | sed 's/.total_allocations://' \ - | awk '{ print $1 "=" ((int($2 / 1000) + 1) * 1000) }' \ + | awk '{ print " " $1 ": " ((int($2 / 1000) + 1) * 1000) }' \ | sort