Use heap indirection to manage type signatures by olix0r · Pull Request #2510 · linkerd/linkerd2-proxy

olix0r · 2023-11-10T08:53:32Z

The proxy's release debug symbols are currently >6GB. It doesn't appear
to even be possible to build the proxy with debuginfo=2, as we overflow
u32::MAX and violate the restrictions of DWARF32.

To fix this, we more aggressively use dynamic dispatch via the new
Stack::arc_new_box helpers. This helps reduce our largest type
signatures from >700KB to ~50KB. We'll want to follow up with changes
that monitor the size of these signatures to prevent regressions.

This allows us to minimize many type assertions, etc.

There are no functional changes in this PR. Release builds will now
produce full debug information with file:line annotations.

codecov-commenter · 2023-11-10T18:06:06Z

Codecov Report

Attention: 661 lines in your changes are missing coverage. Please review.

Comparison is base (69f9af1) 58.06% compared to head (9da5bf4) 58.94%.
Report is 277 commits behind head on main.

❗ Current head 9da5bf4 differs from pull request most recent head 248df33. Consider uploading reports for the commit 248df33 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2510      +/-   ##
==========================================
+ Coverage   58.06%   58.94%   +0.88%     
==========================================
  Files         293      324      +31     
  Lines       12956    15324    +2368     
==========================================
+ Hits         7523     9033    +1510     
- Misses       5433     6291     +858

Files	Coverage Δ
hyper-balance/src/lib.rs	`43.47% <ø> (ø)`
linkerd/addr/src/addr_match.rs	`63.63% <ø> (ø)`
linkerd/addr/src/lib.rs	`61.95% <100.00%> (+1.51%)`	⬆️
linkerd/app/core/src/config.rs	`100.00% <100.00%> (ø)`
linkerd/app/core/src/control.rs	`90.47% <100.00%> (+2.63%)`	⬆️
linkerd/app/core/src/disco_cache.rs	`100.00% <100.00%> (ø)`
linkerd/app/core/src/errors.rs	`100.00% <100.00%> (ø)`
linkerd/app/core/src/lib.rs	`87.50% <ø> (ø)`
linkerd/app/core/src/metrics.rs	`96.94% <100.00%> (-1.54%)`	⬇️
linkerd/app/core/src/telemetry/build_info.rs	`100.00% <100.00%> (ø)`
... and 177 more

... and 54 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The proxy's release debug symbols are currently >6GB. It doesn't appear to even be possible to build the proxy with debuginfo=2, as we overflow u32::MAX and violate the restrictions of DWARF32. To fix this, we more aggressively use dynamic dispatch via the new `Stack::arc_new_box` helpers. This helps reduce our largest type signatures from >700KB to ~50KB. We'll want to follow up with changes that monitor the size of these signatures to prevent regressions. This allows us to minimize many type assertions, etc. There are no functional changes in this PR. Release builds will now produce full debug information with file:line annotations.

hawkw

this looks good to me!

i was a bit curious about the change from using one push_on_service calls with multiple layers to using a separate push_on_service for each layer --- does that impact symbol size, or was it just a refactor?

i suspect this won't have too big of a latency impact, since the boxing is mostly at the NewService level, and not at the level of individual request/response futures. i'm still curious about potential perf testing, though!

hawkw · 2023-11-10T23:05:26Z

linkerd/app/admin/src/stack.rs

+        let http = svc::stack(move |_| admin.clone())
+            .push(
+                metrics
+                    .proxy
+                    .http_endpoint
+                    .to_layer::<classify::Response, _, Permitted>(),
+            )
            .push(classify::NewClassify::layer_default())
            .push_map_target(|(permit, http)| Permitted { permit, http })
-            .push(inbound::policy::NewHttpPolicy::layer(metrics.http_authz.clone()))
+            .push(inbound::policy::NewHttpPolicy::layer(
+                metrics.http_authz.clone(),
+            ))


this looks like just Rustfmt, right?

hawkw · 2023-11-10T23:07:27Z

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

-                        // This box is needed to reduce compile times on recent
-                        // (2021-10-17) nightlies, though this may be fixed by
-                        // https://github.com/rust-lang/rust/pull/89831. It should
-                        // be removed when possible.
-                        .push(svc::BoxService::layer())


i'm assuming this BoxService was obviated by the boxing added in this PR?

We're just doing this everywhere now, so this place isn't all that special. The boxing is now included in the arc_ helper

hawkw · 2023-11-10T23:08:15Z

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

+                // Downgrades the protocol if upgraded by an outbound proxy.
+                .push_on_service(http::orig_proto::Downgrade::layer())
+                // Limit the number of in-flight inbound requests.
+                //
+                // TODO(ver) This concurrency limit applies only to
+                // requests that do not yet have responses, but ignores
+                // streaming bodies. We should change this to an
+                // HTTP-specific imlementation that tracks request and
+                // response bodies.
+                .push_on_service(svc::ConcurrencyLimitLayer::new(max_in_flight_requests))
+                // Shed load by failing requests when the concurrency
+                // limit is reached.
+                .push_on_service(svc::LoadShed::layer())


out of curiosity: does separating these into multiple push_on_service calls also have an impact on symbol size? or, was it just a drive-by refactor?

The additional layer types don't help anything when we're looking at the type signatures. This keeps them a little simpler. Also the added benefit of outdenting things...

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) Signed-off-by: Oliver Gould <[email protected]>

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) * proxy: Use debian12 distroless base image Signed-off-by: Oliver Gould <[email protected]>

olix0r added 18 commits November 10, 2023 00:21

boxit

ca50da6

little boxes made out of ticky tacky

a01c2a7

mas box

b86b266

boxup

212bec5

boxy

a81cded

combine filters

f45b16b

box gateway

7f588d3

required boxing

59ebd87

wipbox

afb3976

enable full debuginfo

c8ba954

docker

340a616

debuginfo=1 comparison

fe654e8

apples to apples

cd08b3c

fixup! apples to apples

49553a7

coverage/

18c0b96

no sudo

7f23e17

hack

4a934f9

continue-on-error

ba69337

olix0r added 10 commits November 10, 2023 19:19

ci: Fix check-each

b4c1825

Merge branch 'main' into ver/box

18abf10

Use helpers to reduce boilerplate

9da5bf4

naming

54eca6c

Merge branch 'main' into ver/box

f3b29c6

fix inbound fuzzer build

8d1f510

dir

5c7c23c

naming

8c31a01

cleanup

9d71c64

olix0r changed the title ~~Ver/box~~ Use heap indirection to manage type signatures Nov 10, 2023

olix0r marked this pull request as ready for review November 10, 2023 22:46

olix0r requested a review from a team as a code owner November 10, 2023 22:46

fixup! Use heap indirection to manage type signatures

19b97cf

hawkw approved these changes Nov 10, 2023

View reviewed changes

only enable debuginfo=1 in actual releases

248df33

olix0r merged commit e62cc28 into main Nov 11, 2023

olix0r deleted the ver/box branch November 11, 2023 02:51

olix0r mentioned this pull request Nov 16, 2023

stack: Add BoxCloneSyncService #2523

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use heap indirection to manage type signatures#2510

Use heap indirection to manage type signatures#2510
olix0r merged 30 commits intomainfrom
ver/box

olix0r commented Nov 10, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 10, 2023 •

edited by codecov bot

Loading

Uh oh!

hawkw left a comment

Uh oh!

hawkw Nov 10, 2023

Uh oh!

olix0r Nov 10, 2023

Uh oh!

hawkw Nov 10, 2023

Uh oh!

olix0r Nov 10, 2023

Uh oh!

hawkw Nov 10, 2023

Uh oh!

olix0r Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

olix0r commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 10, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

hawkw Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

hawkw Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olix0r commented Nov 10, 2023 •

edited

Loading

codecov-commenter commented Nov 10, 2023 •

edited by codecov bot

Loading