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

Improving Performance on the API Gzip Handler #12363

Merged
merged 4 commits into from May 30, 2023

Conversation

alanprot
Copy link
Contributor

@alanprot alanprot commented May 13, 2023

Using github.com/klauspost/compress package to replace the current Gzip Handler on the API.

We can see significant improvements using this handler over the current one as shown in the benchmark:

For instance: for a 4mb compressed response we can see that the compression time decreased from 1.1s to 237ms (-78.64%) and the memory allocated decreased from 26.6M to 5.5M (-78.95%) while the response size only increased from 4.863M to 4.904M (0.85%)

PS: This api is already being used on Thanos: thanos-io/thanos#6332

[ec2-user@ip-172-31-30-12 ~]$ ~/go/bin/benchstat /tmp/old /tmp/new
goos: linux
goarch: amd64
pkg: github.com/prometheus/prometheus/util/httputil
cpu: Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
                                 │   /tmp/old    │              /tmp/new               │
                                 │    sec/op     │    sec/op     vs base               │
_compression/gzip-100K-labels-32   107.90m ± ∞ ¹   22.63m ± ∞ ¹  -79.03% (p=0.008 n=5)
_compression/gzip-1M-labels-32     1111.4m ± ∞ ¹   237.3m ± ∞ ¹  -78.64% (p=0.008 n=5)
_compression/gzip-10-labels-32     332.61µ ± ∞ ¹   59.47µ ± ∞ ¹  -82.12% (p=0.008 n=5)
_compression/gzip-100-labels-32     393.5µ ± ∞ ¹   102.8µ ± ∞ ¹  -73.87% (p=0.008 n=5)
_compression/gzip-1K-labels-32     1462.6µ ± ∞ ¹   357.9µ ± ∞ ¹  -75.53% (p=0.008 n=5)
_compression/gzip-10K-labels-32     9.914m ± ∞ ¹   2.359m ± ∞ ¹  -76.20% (p=0.008 n=5)
geomean                             7.814m         1.740m        -77.73%
¹ need >= 6 samples for confidence interval at level 0.95

                                 │   /tmp/old    │               /tmp/new               │
                                 │ ContentLength │ ContentLength  vs base               │
_compression/gzip-100K-labels-32    472.4k ± ∞ ¹    474.2k ± ∞ ¹   +0.37% (p=0.008 n=5)
_compression/gzip-1M-labels-32      4.863M ± ∞ ¹    4.904M ± ∞ ¹   +0.85% (p=0.008 n=5)
_compression/gzip-10-labels-32       96.00 ± ∞ ¹    171.00 ± ∞ ¹  +78.12% (p=0.008 n=5)
_compression/gzip-100-labels-32      405.0 ± ∞ ¹     397.0 ± ∞ ¹   -1.98% (p=0.008 n=5)
_compression/gzip-1K-labels-32      4.140k ± ∞ ¹    4.248k ± ∞ ¹   +2.61% (p=0.008 n=5)
_compression/gzip-10K-labels-32     47.31k ± ∞ ¹    48.24k ± ∞ ¹   +1.96% (p=0.008 n=5)
geomean                             16.11k          17.85k        +10.79%
¹ need >= 6 samples for confidence interval at level 0.95

                                 │    /tmp/old     │               /tmp/new               │
                                 │      B/op       │     B/op       vs base               │
_compression/gzip-100K-labels-32    1051.7Ki ± ∞ ¹   140.7Ki ± ∞ ¹  -86.62% (p=0.008 n=5)
_compression/gzip-1M-labels-32      26.387Mi ± ∞ ¹   5.554Mi ± ∞ ¹  -78.95% (p=0.008 n=5)
_compression/gzip-10-labels-32     804.040Ki ± ∞ ¹   4.815Ki ± ∞ ¹  -99.40% (p=0.008 n=5)
_compression/gzip-100-labels-32     804.01Ki ± ∞ ¹   10.59Ki ± ∞ ¹  -98.68% (p=0.008 n=5)
_compression/gzip-1K-labels-32      804.25Ki ± ∞ ¹   10.43Ki ± ∞ ¹  -98.70% (p=0.008 n=5)
_compression/gzip-10K-labels-32     806.23Ki ± ∞ ¹   24.60Ki ± ∞ ¹  -96.95% (p=0.008 n=5)
geomean                              1.476Mi         46.78Ki        -96.90%
¹ need >= 6 samples for confidence interval at level 0.95

                                 │   /tmp/old   │              /tmp/new               │
                                 │  allocs/op   │  allocs/op    vs base               │
_compression/gzip-100K-labels-32    325.0 ± ∞ ¹    307.0 ± ∞ ¹   -5.54% (p=0.008 n=5)
_compression/gzip-1M-labels-32     2.571k ± ∞ ¹   2.499k ± ∞ ¹   -2.80% (p=0.008 n=5)
_compression/gzip-10-labels-32      85.00 ± ∞ ¹    64.00 ± ∞ ¹  -24.71% (p=0.008 n=5)
_compression/gzip-100-labels-32     85.00 ± ∞ ¹    69.00 ± ∞ ¹  -18.82% (p=0.008 n=5)
_compression/gzip-1K-labels-32      87.00 ± ∞ ¹    73.00 ± ∞ ¹  -16.09% (p=0.008 n=5)
_compression/gzip-10K-labels-32    108.00 ± ∞ ¹    95.00 ± ∞ ¹  -12.04% (p=0.008 n=5)
geomean                             196.0          169.2        -13.66%
¹ need >= 6 samples for confidence interval at level 0.95

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@roidelapluie
Copy link
Member

See also #10782

@alanprot
Copy link
Contributor Author

Nice! A similar PR, but here it's using the GzipHandler directly which has other optimizations:

See: https://github.com/klauspost/compress/blob/9a951bee0e5e9df261e6ca6a68bfa1e819f333d0/gzhttp/compress.go#L420-L432

Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

I think it's worth it. We should also go on with the scraper side.

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

I was a bit confused initially what is going on in this PR. There are two commits, but neither one gives any explanation and one only describes half of what was done.
I think I see:

  • refactoring to move selection of compression from newCompressedResponseWriter to CompressionHandler.ServeHTTP
  • renaming compressedResponseWriter since it now only does one kind of compression.
  • change from Go standard gzip to klauspost/compress (this is in the PR description)
  • changing a test to use a bigger payload (unclear why).
  • adding a benchmark

(PS you can clean up the "need >= 6 samples" in benchmark results by re-running with -count=6)

util/httputil/compression.go Outdated Show resolved Hide resolved
util/httputil/compression.go Outdated Show resolved Hide resolved
util/httputil/compression.go Show resolved Hide resolved
util/httputil/compression_test.go Show resolved Hide resolved
@@ -14,11 +14,12 @@
package httputil

import (
"compress/gzip"
"compress/zlib"
Copy link
Member

Choose a reason for hiding this comment

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

Why not change zlib to klauspost too?

Copy link
Contributor Author

@alanprot alanprot May 24, 2023

Choose a reason for hiding this comment

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

Yeah.. we could.. lets do it! :D

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks for updates. Couple more thoughts.

util/httputil/compression.go Show resolved Hide resolved
@@ -30,51 +31,31 @@ const (

// Wrapper around http.Handler which adds suitable response compression based
Copy link
Member

Choose a reason for hiding this comment

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

It's not around http.Handler, and doesn't look at headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! thanks a lot for the comments btw!

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Copy link
Member

@bboreham bboreham 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!

@bboreham bboreham merged commit dfae954 into prometheus:main May 30, 2023
23 checks passed
@alanprot
Copy link
Contributor Author

Thanks!! :D

roidelapluie added a commit to roidelapluie/prometheus that referenced this pull request Jun 20, 2023
…)"

This reverts commit dfae954.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
roidelapluie added a commit that referenced this pull request Jun 20, 2023
Revert "Improving Performance on the API Gzip Handler (#12363)"
bboreham pushed a commit to bboreham/prometheus that referenced this pull request Jul 24, 2023
Using github.com/klauspost/compress package to replace the current Gzip Handler on the API.
We see significant improvements using this handler over the current one as shown in the benchmark added.

Also:
* move selection of compression from `newCompressedResponseWriter` to `*CompressionHandler.ServeHTTP`.
* renaming `compressedResponseWriter` since it now only does one kind of compression.

Signed-off-by: Alan Protasio <alanprot@gmail.com>
bboreham pushed a commit to bboreham/prometheus that referenced this pull request Jul 24, 2023
…)"

This reverts commit dfae954.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants