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

Use gzipWriterPools index rather then gzip level in GzipResponseWriter #20

Merged
merged 4 commits into from
Sep 21, 2016
Merged

Use gzipWriterPools index rather then gzip level in GzipResponseWriter #20

merged 4 commits into from
Sep 21, 2016

Conversation

Thomasdezeeuw
Copy link
Contributor

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

@jprobinson
Copy link
Contributor

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
Copy link
Contributor Author

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

@jprobinson
Copy link
Contributor

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

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

benchmark                        old ns/op     new ns/op     delta
BenchmarkGzipHandler_S2k-8       84140         85446         +1.55%
BenchmarkGzipHandler_S20k-8      429932        433044        +0.72%
BenchmarkGzipHandler_S100k-8     2147390       2115058       -1.51%
BenchmarkGzipHandler_P2k-8       21868         21994         +0.58%
BenchmarkGzipHandler_P20k-8      103495        103410        -0.08%
BenchmarkGzipHandler_P100k-8     518237        503005        -2.94%

benchmark                        old allocs     new allocs     delta
BenchmarkGzipHandler_S2k-8       20             20             +0.00%
BenchmarkGzipHandler_S20k-8      23             23             +0.00%
BenchmarkGzipHandler_S100k-8     25             25             +0.00%
BenchmarkGzipHandler_P2k-8       20             20             +0.00%
BenchmarkGzipHandler_P20k-8      23             23             +0.00%
BenchmarkGzipHandler_P100k-8     25             25             +0.00%

benchmark                        old bytes     new bytes     delta
BenchmarkGzipHandler_S2k-8       6136          5770          -5.96%
BenchmarkGzipHandler_S20k-8      46340         46055         -0.62%
BenchmarkGzipHandler_S100k-8     205909        201839        -1.98%
BenchmarkGzipHandler_P2k-8       6235          6197          -0.61%
BenchmarkGzipHandler_P20k-8      48350         48611         +0.54%
BenchmarkGzipHandler_P100k-8     225116        224954        -0.07%
@Thomasdezeeuw
Copy link
Contributor Author

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
Copy link
Contributor

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.

jameshartig and others added 3 commits September 21, 2016 19:40
This removes the need to calculate the index twice on every request.

benchmark                        old ns/op     new ns/op     delta
BenchmarkGzipHandler_S2k-8       84140         85446         +1.55%
BenchmarkGzipHandler_S20k-8      429932        433044        +0.72%
BenchmarkGzipHandler_S100k-8     2147390       2115058       -1.51%
BenchmarkGzipHandler_P2k-8       21868         21994         +0.58%
BenchmarkGzipHandler_P20k-8      103495        103410        -0.08%
BenchmarkGzipHandler_P100k-8     518237        503005        -2.94%

benchmark                        old allocs     new allocs     delta
BenchmarkGzipHandler_S2k-8       20             20             +0.00%
BenchmarkGzipHandler_S20k-8      23             23             +0.00%
BenchmarkGzipHandler_S100k-8     25             25             +0.00%
BenchmarkGzipHandler_P2k-8       20             20             +0.00%
BenchmarkGzipHandler_P20k-8      23             23             +0.00%
BenchmarkGzipHandler_P100k-8     25             25             +0.00%

benchmark                        old bytes     new bytes     delta
BenchmarkGzipHandler_S2k-8       6136          5770          -5.96%
BenchmarkGzipHandler_S20k-8      46340         46055         -0.62%
BenchmarkGzipHandler_S100k-8     205909        201839        -1.98%
BenchmarkGzipHandler_P2k-8       6235          6197          -0.61%
BenchmarkGzipHandler_P20k-8      48350         48611         +0.54%
BenchmarkGzipHandler_P100k-8     225116        224954        -0.07%
@Thomasdezeeuw
Copy link
Contributor Author

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
Copy link
Contributor

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

@jprobinson jprobinson merged commit f6438db into nytimes:master Sep 21, 2016
@jprobinson
Copy link
Contributor

🎉 💯

@Thomasdezeeuw Thomasdezeeuw deleted the gzipresponsewriter-index branch September 21, 2016 22:45
@Thomasdezeeuw
Copy link
Contributor Author

Thanks

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