nytimes / gziphandler

Go middleware to gzip HTTP responses
https://godoc.org/github.com/NYTimes/gziphandler
Apache License 2.0
868 stars 129 forks source link

Use gzipWriterPools index rather then gzip level in GzipResponseWriter #20

Closed Thomasdezeeuw closed 8 years ago

Thomasdezeeuw commented 8 years ago

This removes the need to calculate the index on every request.

jprobinson commented 8 years ago

This still looks like the index is getting calculated on every request as poolIndex is still being called within the middleware handler. If anything, this is causing the function to be called more often than it would need to be as it no longer is part of the lazy init block.

Have you run benchmarks to check if there are any significant improvements?

Thomasdezeeuw commented 8 years ago

I'll change when the index is calculated to when the function is first called and will add some benchmarks.

jprobinson commented 8 years ago

Could you also please provide some context as to the reasoning behind this change?

Thomasdezeeuw commented 8 years ago

The main reason behind the change is to not calculate the pool index on every request, but only when NewGzipLevelHandler is called. This increase the performance a little bit, see the benchmarks in the commit.

jprobinson commented 8 years ago

It looks like the latest commit you've pushed does what you're looking for as it now calls poolIndex outside of the http.HandlerFunc returned by the construct.

Now you just need to clean up the rename from level => index to get the tests to pass.

Thomasdezeeuw commented 8 years ago

The commits are a bit messed up, but when I try to squash them it messes up the code. This is what happened the first time, that's why I pushed broken code.

jprobinson commented 8 years ago

This looks better, thanks! I'll deal with squashing the commits via the merge button.

jprobinson commented 8 years ago

:tada: 💯

Thomasdezeeuw commented 8 years ago

Thanks