search

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

Lazily create sync.Pools in gzipWriterPools #21

Closed Thomasdezeeuw closed 7 years ago

jprobinson commented 8 years ago

It looks like we're just transferring the logic in the init block to the constructor function, which should happen at server startup anyways.

Are you expecting some performance boost from this addition? Have you run the benchmarks?

Thomasdezeeuw commented 8 years ago

I don't expect a performance boost. However most applications will likely only use a single compression level.

With this change pools will only be created for compression levels that are actually used. This should reduce the memory usage, albeit not by much.

jprobinson commented 8 years ago

I'm a bit concerned we're adding complexity to the library with very little benefit with this PR. Were you having memory or performance issues with this middleware that led to this change?

In the future, please provide some additional context when making a PR. For some ideas, take a look at our contributing guidelines: https://github.com/NYTimes/gziphandler/blob/master/CONTRIBUTING.md

Thomasdezeeuw commented 8 years ago

I didn't have any problems with the package, however creating a pool for each compression level seemed unnecessary to me, hence the change.