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

chore: Improve performance of TenantID #541

Merged
merged 4 commits into from
Jul 10, 2024
Merged

chore: Improve performance of TenantID #541

merged 4 commits into from
Jul 10, 2024

Conversation

cyriltovena
Copy link
Contributor

What this PR does:

This improves performance in case there's is only 1 tenant which is apparently 99% of the time. The multi tenant case is kept unchanged.

❯ benchstat before.txt after.txt
name                   old time/op    new time/op    delta
TenantID/single-16       84.9ns ± 1%    19.4ns ± 3%   -77.16%  (p=0.008 n=5+5)
TenantID/single#01-16     180ns ± 5%     197ns ±13%    +9.21%  (p=0.032 n=5+5)

name                   old alloc/op   new alloc/op   delta
TenantID/single-16        40.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
TenantID/single#01-16     72.0B ± 0%     72.0B ± 0%      ~     (all equal)

name                   old allocs/op  new allocs/op  delta
TenantID/single-16         2.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
TenantID/single#01-16      2.00 ± 0%      2.00 ± 0%      ~     (all equal)

Basically saves us 2 allocs per push/query requests.

Which issue(s) this PR fixes:

Fixes #

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX] Don't think I need a changelog entry for that.

Copy link
Collaborator

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see there's a performance regression of about 10% CPU time for the multi-tenant case versus main.

My suggestion is to refactor the string parsing in TenantIDs into a function tenantIDsFromString, that is also called from TenantID so you don't have to call user.ExtractOrgID twice in the multi-tenant case. When I try this, the perf regression goes away.

✗ benchstat perf.txt perf-arve.txt         
goos: linux
goarch: amd64
pkg: github.com/grafana/dskit/tenant
cpu: AMD Ryzen 9 3950X 16-Core Processor            
                   │  perf.txt   │           perf-arve.txt           │
                   │   sec/op    │   sec/op     vs base              │
TenantID/single-32   19.96n ± 9%   20.46n ± 4%       ~ (p=0.589 n=6)
TenantID/multi-32    320.5n ± 3%   290.6n ± 2%  -9.34% (p=0.002 n=6)
geomean              79.98n        77.10n       -3.60%

                   │   perf.txt   │           perf-arve.txt            │
                   │     B/op     │    B/op     vs base                │
TenantID/single-32   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
TenantID/multi-32    48.00 ± 0%     48.00 ± 0%       ~ (p=1.000 n=6) ¹
geomean                         ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                   │   perf.txt   │           perf-arve.txt            │
                   │  allocs/op   │ allocs/op   vs base                │
TenantID/single-32   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
TenantID/multi-32    1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                         ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@cyriltovena
Copy link
Contributor Author

good call

❯ benchstat before.txt after.txt
name                old time/op    new time/op    delta
TenantID/single-16    89.9ns ± 3%    19.0ns ± 1%   -78.85%  (p=0.008 n=5+5)
TenantID/multi-16      184ns ± 4%     183ns ± 2%      ~     (p=0.548 n=5+5)

name                old alloc/op   new alloc/op   delta
TenantID/single-16     40.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
TenantID/multi-16      72.0B ± 0%     72.0B ± 0%      ~     (all equal)

name                old allocs/op  new allocs/op  delta
TenantID/single-16      2.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
TenantID/multi-16       2.00 ± 0%      2.00 ± 0%      ~     (all equal)

Copy link
Collaborator

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@aknuds1
Copy link
Collaborator

aknuds1 commented Jul 4, 2024

CI is failing consistently in TestMultipleClients :/ Could it be because of the change?

@cyriltovena
Copy link
Contributor Author

Seems like a flake

@cyriltovena cyriltovena merged commit 6c2188e into main Jul 10, 2024
6 checks passed
@cyriltovena cyriltovena deleted the chore/perf-tenant branch July 10, 2024 10:47
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.

3 participants