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

compress: introduce CompressHandlerLevel #48

Merged
merged 3 commits into from
Oct 24, 2015
Merged

compress: introduce CompressHandlerLevel #48

merged 3 commits into from
Oct 24, 2015

Conversation

flyingmutant
Copy link
Contributor

CompressHandlerLevel allows to specify a compression level explicitly. This allows clients to use e.g. gzip.BestCompression, which often results in major bandwidth savings.

@@ -48,7 +57,7 @@ func CompressHandler(h http.Handler) http.Handler {
w.Header().Set("Content-Encoding", "gzip")
w.Header().Add("Vary", "Accept-Encoding")

gw := gzip.NewWriter(w)
gw, _ := gzip.NewWriterLevel(w, level)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of ignoring the error, we should check the level is within acceptable bounds, else we will run into undefined behaviour. If not, we should just set gzip.DefaultCompression as the level. Cover this in the function docs.

@elithrar
Copy link
Contributor

Thanks @flyingmutant! Are you able to address my comment and add a test to check that a level outside of the bounds (below/above) is correctly reset to the default level as well?

@flyingmutant
Copy link
Contributor Author

Thanks for the review! Does it look OK now?

@flyingmutant
Copy link
Contributor Author

@elithrar, any updates here?

elithrar added a commit that referenced this pull request Oct 24, 2015
[feature] CompressHandler: introduce CompressHandlerLevel
@elithrar elithrar merged commit 12b0adb into gorilla:master Oct 24, 2015
@elithrar
Copy link
Contributor

Whoops, sorry @flyingmutant - missed this! Looks good to me: thanks for contributing 👍

@elithrar
Copy link
Contributor

One thing I wanted to add (for posterity): make sure to benchmark the CPU impact on the client (i.e. a mobile device!) when increasing compression. Slightly better compression over the wire may not be ideal in those cases.

Using something like gzip.BestCompression in a real-time HTTP context may actually be slower once you take into account CPU time on both ends.

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.

None yet

2 participants