Box all stack modules (#1011)

Building a release-mode proxy currently consumes significant resources:

        Elapsed (wall clock) time (h:mm:ss or m:ss): 4:23.84
        Maximum resident set size (kbytes): 23751132

With this, change we reduce memory consumption by >50%:

        Elapsed (wall clock) time (h:mm:ss or m:ss): 2:18.08
        Maximum resident set size (kbytes): 10144624

This is accomplished by boxing each top-level module. This is made
explicit by modifying the signature of these stack builders to
explicitly reference boxed types (instead of using `impl`).

Furthermore, we add a `BoxNewService` wherever the inner new service is
cloned into a returned `Service`. This most commonly happens with layers
like `http::NewDetectService`` `NewDetectTls`, and `NewRouter`.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Author: olix0r

.github/workflows/advisory.yml

@@ -0,0 +1,54 @@
+# Optional build
+name: Advisory
+
+on:
+  pull_request: {}
+
+jobs:
+  # Prevent sudden announcement of a new advisory from failing Ci.
+  advisories:
+    timeout-minutes: 5
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - uses: EmbarkStudios/cargo-deny-action@202e2b23300c1ef90c9d7389a0bc5b34c05c0fe4
+      with:
+        command: check advisories
+
+  # Ensure that all of the fuzzers continue to build.
+  #
+  # This is optional because it depends on the nightly toolchain.
+  fuzzers:
+    timeout-minutes: 40
+    runs-on: ubuntu-latest
+    container:
+      image: docker://rust:1.52.1-buster
+    strategy:
+      matrix:
+        dir:
+          - addr
+          - app/inbound
+          - dns
+          - proxy/http
+          - tls
+          - transport-header
+    steps:
+      - uses: actions/checkout@v2
+      - run: rustup toolchain add nightly
+      - run: cargo install cargo-fuzz
+      # Iterate through all fuzz crates to ensure each compiles independently.
+      - run: cd linkerd/${{matrix.dir}}/fuzz && cargo +nightly fuzz build
+      # Error if the repo isn't clean (i.e. because lock files were modified).
+      - run: git status && git diff-index --quiet HEAD
+
+  # It's easy to make changes that are innocuous in dev builds that end up ballooning resources
+  # needed for release builds. This job builds the proxy in release-mode to ensure the build isn't
+  # OOM-killed.
+  release-binary:
+    timeout-minutes: 15
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - run: make build
+        env:
+          CARGO_RELEASE: true

.github/workflows/rust.yml

@@ -4,32 +4,20 @@ on:
   pull_request: {}
 
 jobs:
-  fmt:
-    timeout-minutes: 5
-    runs-on: ubuntu-latest
-    container:
-      image: docker://rust:1.52.1-buster
-    steps:
-      - uses: actions/checkout@v2
-      - run: rustup component add rustfmt
-      - run: make check-fmt
 
+  ## Required builds
+
+  # Ensures we don't take unintended dependencies.
   audit:
     timeout-minutes: 5
     runs-on: ubuntu-latest
-    strategy:
-      matrix:
-        checks:
-          - advisories
-          - bans licenses sources
-    # Prevent sudden announcement of a new advisory from failing Ci.
-    continue-on-error: ${{ matrix.checks == 'advisories' }}
     steps:
     - uses: actions/checkout@v2
     - uses: EmbarkStudios/cargo-deny-action@202e2b23300c1ef90c9d7389a0bc5b34c05c0fe4
       with:
-        command: check ${{ matrix.checks }}
+        command: check bans licenses sources
 
+  # Linting
   clippy:
     timeout-minutes: 5
     runs-on: ubuntu-latest
@@ -40,42 +28,31 @@ jobs:
       - run: rustup component add clippy
       - run: make lint
 
+  # Iterate through all (non-fuzzer) sub-crates to ensure each compiles independently.
   check:
     timeout-minutes: 20
     runs-on: ubuntu-latest
     container:
       image: docker://rust:1.52.1-buster
     steps:
       - uses: actions/checkout@v2
-      # Iterate through all (non-fuzzer) sub-crates to ensure each compiles independently.
       - run: for d in $(for toml in $(find . -mindepth 2 -name Cargo.toml -not -path '*/fuzz/*') ; do echo ${toml%/*} ; done | sort -r ) ; do echo "# $d" ; (cd $d ; cargo check --all-targets) ; done
 
-  test:
-    timeout-minutes: 15
+  # Enforce automated formatting.
+  fmt:
+    timeout-minutes: 5
     runs-on: ubuntu-latest
+    container:
+      image: docker://rust:1.52.1-buster
     steps:
       - uses: actions/checkout@v2
-      - run: make test
+      - run: rustup component add rustfmt
+      - run: make check-fmt
 
-  fuzzers:
-    timeout-minutes: 40
+  # Run all tests.
+  test:
+    timeout-minutes: 15
     runs-on: ubuntu-latest
-    container:
-      image: docker://rust:1.52.1-buster
-    strategy:
-      matrix:
-        dir:
-          - addr
-          - app/inbound
-          - dns
-          - proxy/http
-          - tls
-          - transport-header
     steps:
       - uses: actions/checkout@v2
-      - run: rustup toolchain add nightly
-      - run: cargo install cargo-fuzz
-      # Iterate through all fuzz crates to ensure each compiles independently.
-      - run: cd linkerd/${{matrix.dir}}/fuzz && cargo +nightly fuzz build
-      # Error if the repo isn't clean (i.e. because lock files were modified).
-      - run: git status && git diff-index --quiet HEAD
+      - run: make test

linkerd/app/gateway/src/lib.rs

@@ -67,20 +67,14 @@ struct RefusedNoTarget(());
 #[error("the provided address could not be resolved: {}", self.0)]
 struct RefusedNotResolved(NameAddr);
 
-#[allow(clippy::clippy::too_many_arguments)]
+#[allow(clippy::too_many_arguments)]
 pub fn stack<I, O, P, R>(
     Config { allow_discovery }: Config,
     inbound: Inbound<()>,
     outbound: Outbound<O>,
     profiles: P,
     resolve: R,
-) -> impl svc::NewService<
-    GatewayConnection,
-    Service = impl svc::Service<I, Response = (), Error = impl Into<Error>, Future = impl Send>
-                  + Send
-                  + 'static,
-> + Clone
-       + Send
+) -> svc::BoxNewService<GatewayConnection, svc::BoxService<I, (), Error>>
 where
     I: io::AsyncRead + io::AsyncWrite + io::PeerAddr + fmt::Debug + Send + Sync + Unpin + 'static,
     O: Clone + Send + Sync + Unpin + 'static,
@@ -199,7 +193,8 @@ where
             svc::layers()
                 .push(http::Retain::layer())
                 .push(http::BoxResponse::layer()),
-        );
+        )
+        .push(svc::BoxNewService::layer());
 
     // When handling gateway connections from older clients that do not
     // support the transport header, do protocol detection and route requests
@@ -216,6 +211,7 @@ where
         .push_http_server()
         .into_stack()
         .push(svc::Filter::<ClientInfo, _>::layer(HttpLegacy::try_from))
+        .push(svc::BoxNewService::layer())
         .push(detect::NewDetectService::layer(
             detect_protocol_timeout,
             http::DetectHttp::default(),
@@ -259,11 +255,10 @@ where
                 GatewayConnection::TransportHeader(t) => Ok::<_, Never>(svc::Either::A(t)),
                 GatewayConnection::Legacy(c) => Ok(svc::Either::B(c)),
             },
-            legacy_http
-                .push_on_response(svc::BoxService::layer())
-                .push(svc::BoxNewService::layer())
-                .into_inner(),
+            legacy_http.into_inner(),
         )
+        .push_on_response(svc::BoxService::layer())
+        .push(svc::BoxNewService::layer())
         .into_inner()
 }
 

linkerd/app/inbound/src/direct.rs

@@ -67,12 +67,7 @@ impl<N> Inbound<N> {
     pub fn push_direct<T, I, NSvc, G, GSvc>(
         self,
         gateway: G,
-    ) -> Inbound<
-        impl svc::NewService<
-                T,
-                Service = impl svc::Service<I, Response = (), Error = Error, Future = impl Send>,
-            > + Clone,
-    >
+    ) -> Inbound<svc::BoxNewService<T, svc::BoxService<I, (), Error>>>
     where
         T: Param<Remote<ClientAddr>> + Param<OrigDstAddr> + Clone + Send + 'static,
         I: io::AsyncRead + io::AsyncWrite + io::Peek + io::PeerAddr,
@@ -158,11 +153,14 @@ impl<N> Inbound<N> {
             // Build a ClientInfo target for each accepted connection. Refuse the
             // connection if it doesn't include an mTLS identity.
             .push_request_filter(ClientInfo::try_from)
+            .push(svc::BoxNewService::layer())
             .push(tls::NewDetectTls::layer(
                 rt.identity.clone().map(WithTransportHeaderAlpn),
                 detect_timeout,
             ))
-            .check_new_service::<T, I>();
+            .check_new_service::<T, I>()
+            .push_on_response(svc::BoxService::layer())
+            .push(svc::BoxNewService::layer());
 
         Inbound {
             config,

linkerd/app/inbound/src/http/mod.rs

@@ -27,10 +27,10 @@ impl<H> Inbound<H> {
     pub fn push_http_server<T, I, HSvc>(
         self,
     ) -> Inbound<
-        impl svc::NewService<
-                T,
-                Service = impl svc::Service<I, Response = (), Error = Error, Future = impl Send> + Clone,
-            > + Clone,
+        svc::BoxNewService<
+            T,
+            impl svc::Service<I, Response = (), Error = Error, Future = impl Send> + Clone,
+        >,
     >
     where
         T: Param<Version>
@@ -89,7 +89,8 @@ impl<H> Inbound<H> {
             )
             .check_new_service::<T, http::Request<_>>()
             .instrument(|t: &T| debug_span!("http", v=%Param::<Version>::param(t)))
-            .push(http::NewServeHttp::layer(h2_settings, rt.drain.clone()));
+            .push(http::NewServeHttp::layer(h2_settings, rt.drain.clone()))
+            .push(svc::BoxNewService::layer());
 
         Inbound {
             config,
@@ -110,15 +111,15 @@ where
         self,
         profiles: P,
     ) -> Inbound<
-        impl svc::NewService<
-                HttpAccept,
-                Service = impl svc::Service<
+        svc::BoxNewService<
+            HttpAccept,
+            impl svc::Service<
                     http::Request<http::BoxBody>,
                     Response = http::Response<http::BoxBody>,
                     Error = Error,
                     Future = impl Send,
                 > + Clone,
-            > + Clone,
+        >,
     >
     where
         P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + 'static,
@@ -225,19 +226,18 @@ where
                     .push(http::Retain::layer())
                     .push(http::BoxResponse::layer()),
             )
-            // Boxing is necessary purely to limit the link-time overhead of
-            // having enormous types.
-            .push(svc::BoxNewService::layer())
             .check_new_service::<Target, http::Request<http::BoxBody>>()
             // Removes the override header after it has been used to
             // determine a request target.
             .push_on_response(strip_header::request::layer(DST_OVERRIDE_HEADER))
             // Routes each request to a target, obtains a service for that
             // target, and dispatches the request.
             .instrument_from_target()
+            .push(svc::BoxNewService::layer())
             .push(svc::NewRouter::layer(RequestTarget::from))
             // Used by tap.
-            .push_http_insert_target::<HttpAccept>();
+            .push_http_insert_target::<HttpAccept>()
+            .push(svc::BoxNewService::layer());
 
         Inbound {
             config,

linkerd/app/inbound/src/http/tests.rs

@@ -22,16 +22,16 @@ fn build_server<I>(
     rt: ProxyRuntime,
     profiles: resolver::Profiles,
     connect: Connect<Remote<ServerAddr>>,
-) -> impl svc::NewService<
+) -> svc::BoxNewService<
     HttpAccept,
-    Service = impl tower::Service<
-        I,
-        Response = (),
-        Error = impl Into<linkerd_app_core::Error>,
-        Future = impl Send + 'static,
-    > + Send
-                  + Clone,
-> + Clone
+    impl tower::Service<
+            I,
+            Response = (),
+            Error = impl Into<linkerd_app_core::Error>,
+            Future = impl Send + 'static,
+        > + Send
+        + Clone,
+>
 where
     I: io::AsyncRead + io::AsyncWrite + io::PeerAddr + Send + Unpin + 'static,
 {