-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[influxd] Use sync.Pool to reuse gzip.Writers across requests #7948
Conversation
sync.Pool
to reuse gzip.Writer
s across requests
ffb82c1
to
0197233
Compare
sync.Pool
to reuse gzip.Writer
s across requests
CHANGELOG.md
Outdated
@@ -6,6 +6,10 @@ | |||
|
|||
## v1.2.1 [unreleased] | |||
|
|||
### Features | |||
|
|||
- [#7948](https://github.com/influxdata/influxdb/pull/7948): Reduce memory allocations by reusing gzip.Writers across requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to the 1.3.0 section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This brings alloc_space down from ~20200M to ~10700M in a run of go test ./cmd/influxd/run -bench=Server -memprofile=mem.out -run='^$'
0197233
to
a6a7782
Compare
@jwilder @joelegasse let me know if anything else needs to be changed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small change. Thanks for the PR. :-)
services/httpd/handler.go
Outdated
gz.Reset(w) | ||
defer func() { | ||
gz.Close() | ||
gzipWriterPool.Put(gz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly consider resetting to ioutil.Discard
here to remove the lingering reference to the ResponseWriter
? This would also ensure that all writers returned from Get()
are in the same state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want I will add it, but it should be redundant... objects in pools are discarded during GC, so any reference they hold to other objects won't keep the other objects alive. And for being in the same state, it shouldn't really matter because we call gz.Reset(w)
in any case.
Additionally, gz.Reset()
is not exactly free.
Maybe I can add some comments to clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you have the code written right now is fine. I'd prefer to be a little more defensive, though. I think I'd like to see some utility functions instead. The ad-hoc inline usage is what concerns me. getGzipWriter(io.Writer)
and putGzipWriter(*gzip.Writer)
. That way, as long as those functions are used, someone would have to intentionally use the pool directly to re-use the old ResponseWriter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to here, but instead calling Reset
with the new ResponseWriter
on get
, and possibly even calling Close()
in the put
function.
The new code in the handler could then be
gz := getGzipWriter(w)
defer putGzipWriter(gz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@joelegasse thanks! |
Thanks @CAFxX! |
This brings alloc_space down from ~20200M to ~10700M in a run of
go test ./cmd/influxd/run -bench=Server -memprofile=mem.out -run='^$'
It's hard to get stable performance numbers out of the benchmarks on my MBP, so take the following with a grain of salt: this PR makes most influxd benchmarks faster by a few %, mostly by lowering the amount of GC.
this PR (
go test ./cmd/influxd/run -bench=Server -run='^$' -benchtime=10s
):master
(go test ./cmd/influxd/run -bench=Server -run='^$' -benchtime=10s
):Required for all non-trivial PRs