Refactor out gzipwriter interface #106
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CAFxX
reviewed
Jun 15, 2020
dolmen
reviewed
Apr 22, 2021
writer/stdlib/stdlib.go
Outdated
| } | ||
| } | ||
|
|
||
| type PooledWriter struct { |
dolmen
suggested changes
Apr 22, 2021
gzip.go
Outdated
| @@ -378,6 +343,7 @@ func (pct parsedContentType) equals(mediaType string, params map[string]string) | |||
| type config struct { | |||
| minSize int | |||
| level int | |||
| writer writer.GzipWriterFactory | |||
There was a problem hiding this comment.
Rename to newWriter because this isn't a writer but a function that creates a writer.
| func (pw *PooledWriter) Close() error { | ||
| err := pw.Writer.Close() | ||
| gzipWriterPools[pw.index].Put(pw.Writer) | ||
| return err |
There was a problem hiding this comment.
Missing (to match the original gzip.go line 243):
pw.Writer = nilThis will allow to be future-proof against double-close or use-after-close.
| @@ -407,6 +373,12 @@ func CompressionLevel(level int) option { | |||
| } | |||
| } | |||
|
|
|||
| func Implementation(writer writer.GzipWriterFactory) option { | |||
There was a problem hiding this comment.
Add documentation above.
Add a link to the default implementation.
|
Updated according to reviews |
klauspost
added a commit
to klauspost/compress
that referenced
this pull request
May 31, 2021
Fork and clean up+extend the dead `nytimes/gziphandler` project. Includes nytimes/gziphandler#106 as well as support for stateless encoding. Removes testify from deps.
klauspost
added a commit
to klauspost/compress
that referenced
this pull request
Jun 2, 2021
Fork and clean up+extend the dead `nytimes/gziphandler` project. * Adds `http.Transport` wrapper. * Includes nytimes/gziphandler#106 as well as support for stateless encoding. * Implements a variant of nytimes/gziphandler#81 * Fixes nytimes/gziphandler#103 * Strip "Accept-Ranges" on compressed content. Fixes nytimes/gziphandler#83 * Removes testify from deps. * Constructors boiled down. * Defaults to this gzip package. * Allocations reduced. Default settings comparison: ``` λ benchcmp before.txt after.txt benchmark old ns/op new ns/op delta BenchmarkGzipHandler_S2k-32 51302 25554 -50.19% BenchmarkGzipHandler_S20k-32 301426 174900 -41.98% BenchmarkGzipHandler_S100k-32 1546203 912349 -40.99% BenchmarkGzipHandler_P2k-32 3973 2116 -46.74% BenchmarkGzipHandler_P20k-32 20319 12237 -39.78% BenchmarkGzipHandler_P100k-32 96079 57348 -40.31% benchmark old MB/s new MB/s speedup BenchmarkGzipHandler_S2k-32 39.92 80.14 2.01x BenchmarkGzipHandler_S20k-32 67.94 117.10 1.72x BenchmarkGzipHandler_S100k-32 66.23 112.24 1.69x BenchmarkGzipHandler_P2k-32 515.44 967.76 1.88x BenchmarkGzipHandler_P20k-32 1007.92 1673.55 1.66x BenchmarkGzipHandler_P100k-32 1065.79 1785.58 1.68x benchmark old allocs new allocs delta BenchmarkGzipHandler_S2k-32 22 19 -13.64% BenchmarkGzipHandler_S20k-32 25 22 -12.00% BenchmarkGzipHandler_S100k-32 28 25 -10.71% BenchmarkGzipHandler_P2k-32 22 19 -13.64% BenchmarkGzipHandler_P20k-32 25 22 -12.00% BenchmarkGzipHandler_P100k-32 27 24 -11.11% ``` Client Transport: Speed compared to standard library for an approximate 127KB payload: ``` BenchmarkTransport Single core: BenchmarkTransport/gzhttp-32 1995 609791 ns/op 214.14 MB/s 10129 B/op 73 allocs/op BenchmarkTransport/stdlib-32 1567 772161 ns/op 169.11 MB/s 53950 B/op 99 allocs/op Multi Core: BenchmarkTransport/gzhttp-par-32 29113 36802 ns/op 3548.27 MB/s 11061 B/op 73 allocs/op BenchmarkTransport/stdlib-par-32 16114 66442 ns/op 1965.38 MB/s 54971 B/op 99 allocs/op ``` This includes both serving the http request, parsing requests and decompressing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've implemented a cloudflare-zlib backend for gziphandler. This PR allows for swappable writer implementation to allow people who doesn't want to depend on native module to continue using compress/gzip.
We've been using this in production at @wongnai for 6 months (although we use the build flag to swap implementations, not interface) and as mentioned in #94, it reduces the CPU usage by 43%
Another PR will be open soon to add the zlib implementation once the forked cloudflare-zlib is open source.
closes #94