Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pool handler maps #2

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Pool handler maps #2

merged 2 commits into from
Sep 18, 2024

Conversation

PapaCharlie
Copy link
Member

The previous behavior was to always keep 2 maps, one being written to by each call to Notify while the other gets drained by send. On paper, this approach seems fine since it stands to reason that at steady state, the client will receive a similar number of resources per response, meaning the two maps will be roughly equally sized and shouldn't require any additional allocations. However, during startup, the client will likely resubmit many subscriptions at once and the initial response will likely be quite large. Because these maps are never reclaimed, they now simply serve as bloat, with a significant memory footprint (see attached heapdump). This is made worse by the fact that these large maps are severely underused when the connection becomes idle (except for the occasional update) after the initial resource dump.

image

This new approach makes all handlers share maps from a pool, limiting the overall allocation rate, but also reducing the footprint of each idle connection by only acquiring a map from the pool on the first notification. This means clients that do not receive frequent updates will not hog any memory at all.

This seems to have neglible performance impact under stress:

% benchstat old new
goos: darwin
goarch: arm64
pkg: github.com/linkedin/diderot/internal/server
cpu: Apple M1 Pro
                      │     old      │                 new                 │
                      │    sec/op    │   sec/op     vs base                │
Handlers/____1_subs-8   125.8n ± 12%   125.2n ± 9%        ~ (p=0.670 n=10)
Handlers/___10_subs-8   1.965µ ±  3%   1.921µ ± 3%   -2.21% (p=0.030 n=10)
Handlers/__100_subs-8   20.72µ ±  5%   20.74µ ± 4%        ~ (p=0.869 n=10)
Handlers/_1000_subs-8   221.4µ ± 15%   243.8µ ± 8%  +10.10% (p=0.029 n=10)
Handlers/10000_subs-8   2.491m ±  5%   2.591m ± 4%        ~ (p=0.165 n=10)
geomean                 19.51µ         19.94µ        +2.22%

                      │     old      │                  new                  │
                      │     B/op     │     B/op      vs base                 │
Handlers/____1_subs-8     128.0 ± 0%     128.0 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/___10_subs-8   1.250Ki ± 0%   1.254Ki ± 0%  +0.31% (p=0.000 n=10)
Handlers/__100_subs-8   12.50Ki ± 0%   12.70Ki ± 0%  +1.57% (p=0.000 n=10)
Handlers/_1000_subs-8   125.4Ki ± 0%   132.5Ki ± 1%  +5.67% (p=0.000 n=10)
Handlers/10000_subs-8   1.258Mi ± 0%   1.334Mi ± 2%  +5.98% (p=0.000 n=10)
geomean                 12.58Ki        12.92Ki       +2.67%
¹ all samples are equal

                      │     old     │                 new                  │
                      │  allocs/op  │  allocs/op   vs base                 │
Handlers/____1_subs-8    1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/___10_subs-8    10.00 ± 0%    10.00 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/__100_subs-8    100.0 ± 0%    100.0 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/_1000_subs-8   1.002k ± 0%   1.003k ± 0%  +0.10% (p=0.005 n=10)
Handlers/10000_subs-8   10.23k ± 0%   10.24k ± 0%       ~ (p=0.402 n=10)
geomean                  100.5         100.5       +0.05%
¹ all samples are equal

The previous behavior was to always keep 2 maps, one being written to by each
call to `Notify` while the other gets drained by `send`. On paper, this approach
seems fine since it stands to reason that at steady state, the client will
receive a similar number of resources per response, meaning the two maps will be
roughly equally sized and shouldn't require any additional allocations. However,
during startup, the client will likely resubmit many subscriptions at once and
the initial response will likely be quite large. Because these maps are never
reclaimed, they now simply serve as bloat, with a significant memory footprint
(see attached heapdump). This is made worse by the fact that these large maps
are severely underused when the connection becomes idle (except for the
occasional update) after the initial resource dump.

This new approach makes all handlers share maps from a pool, limiting the
overall allocation rate, but also reducing the footprint of each idle connection
by only acquiring a map from the pool on the first notification. This means
clients that do not receive frequent updates will not hog any memory at all.

This seems to have neglible performance impact under stress:
```
% benchstat old new
goos: darwin
goarch: arm64
pkg: github.com/linkedin/diderot/internal/server
cpu: Apple M1 Pro
                      │     old      │                 new                 │
                      │    sec/op    │   sec/op     vs base                │
Handlers/____1_subs-8   125.8n ± 12%   125.2n ± 9%        ~ (p=0.670 n=10)
Handlers/___10_subs-8   1.965µ ±  3%   1.921µ ± 3%   -2.21% (p=0.030 n=10)
Handlers/__100_subs-8   20.72µ ±  5%   20.74µ ± 4%        ~ (p=0.869 n=10)
Handlers/_1000_subs-8   221.4µ ± 15%   243.8µ ± 8%  +10.10% (p=0.029 n=10)
Handlers/10000_subs-8   2.491m ±  5%   2.591m ± 4%        ~ (p=0.165 n=10)
geomean                 19.51µ         19.94µ        +2.22%

                      │     old      │                  new                  │
                      │     B/op     │     B/op      vs base                 │
Handlers/____1_subs-8     128.0 ± 0%     128.0 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/___10_subs-8   1.250Ki ± 0%   1.254Ki ± 0%  +0.31% (p=0.000 n=10)
Handlers/__100_subs-8   12.50Ki ± 0%   12.70Ki ± 0%  +1.57% (p=0.000 n=10)
Handlers/_1000_subs-8   125.4Ki ± 0%   132.5Ki ± 1%  +5.67% (p=0.000 n=10)
Handlers/10000_subs-8   1.258Mi ± 0%   1.334Mi ± 2%  +5.98% (p=0.000 n=10)
geomean                 12.58Ki        12.92Ki       +2.67%
¹ all samples are equal

                      │     old     │                 new                  │
                      │  allocs/op  │  allocs/op   vs base                 │
Handlers/____1_subs-8    1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/___10_subs-8    10.00 ± 0%    10.00 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/__100_subs-8    100.0 ± 0%    100.0 ± 0%       ~ (p=1.000 n=10) ¹
Handlers/_1000_subs-8   1.002k ± 0%   1.003k ± 0%  +0.10% (p=0.005 n=10)
Handlers/10000_subs-8   10.23k ± 0%   10.24k ± 0%       ~ (p=0.402 n=10)
geomean                  100.5         100.5       +0.05%
¹ all samples are equal
```
ZoabKapoor
ZoabKapoor previously approved these changes Sep 18, 2024
internal/server/handlers.go Outdated Show resolved Hide resolved
@PapaCharlie PapaCharlie merged commit d1254dc into master Sep 18, 2024
2 checks passed
@PapaCharlie PapaCharlie deleted the pc/handlers branch September 18, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants