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

[influxd] Use sync.Pool to reuse gzip.Writers across requests #7948

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

CAFxX
Copy link

@CAFxX CAFxX commented Feb 5, 2017

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):

BenchmarkServer_Query_Count_1-4                  	   10000	   1818724 ns/op	  128974 B/op	     513 allocs/op
BenchmarkServer_Query_Count_1K-4                 	    5000	   3693137 ns/op	  152458 B/op	     514 allocs/op
BenchmarkServer_Query_Count_100K-4               	    1000	  12607160 ns/op	 1889711 B/op	     524 allocs/op
BenchmarkServer_Query_Count_1M-4                 	     200	 117704311 ns/op	16635919 B/op	    1554 allocs/op
BenchmarkServer_Query_Count_Where_500-4          	   10000	   1809682 ns/op	   98816 B/op	     580 allocs/op
BenchmarkServer_Query_Count_Where_1K-4           	    5000	   3256780 ns/op	   98983 B/op	     580 allocs/op
BenchmarkServer_Query_Count_Where_10K-4          	    5000	   3886609 ns/op	   98320 B/op	     580 allocs/op
BenchmarkServer_Query_Count_Where_100K-4         	   10000	   2960748 ns/op	   96410 B/op	     580 allocs/op
BenchmarkServer_Query_Count_Where_Regex_500-4    	    5000	   3954818 ns/op	  103231 B/op	     661 allocs/op
BenchmarkServer_Query_Count_Where_Regex_1K-4     	    5000	   3040596 ns/op	  103395 B/op	     661 allocs/op
BenchmarkServer_Query_Count_Where_Regex_10K-4    	    5000	   3777312 ns/op	  103728 B/op	     661 allocs/op
BenchmarkServer_Query_Count_Where_Regex_100K-4   	    5000	   3171716 ns/op	  199497 B/op	    1472 allocs/op
BenchmarkServer_ShowSeries_1-4                   	     200	  87806658 ns/op	18379301 B/op	   70295 allocs/op
BenchmarkServer_ShowSeries_1K-4                  	     200	  85051824 ns/op	18310844 B/op	   69497 allocs/op
BenchmarkServer_ShowSeries_100K-4                	     100	 126484645 ns/op	27288215 B/op	   69693 allocs/op
BenchmarkServer_ShowSeries_1M-4                  	      30	 668360307 ns/op	121411739 B/op	  338035 allocs/op

master (go test ./cmd/influxd/run -bench=Server -run='^$' -benchtime=10s):

BenchmarkServer_Query_Count_1-4                  	   10000	   1904422 ns/op	  909175 B/op	     536 allocs/op
BenchmarkServer_Query_Count_1K-4                 	    5000	   3917357 ns/op	  925456 B/op	     536 allocs/op
BenchmarkServer_Query_Count_100K-4               	    1000	  17747914 ns/op	 2515462 B/op	     539 allocs/op
BenchmarkServer_Query_Count_1M-4                 	     200	 121461770 ns/op	17152968 B/op	    1565 allocs/op
BenchmarkServer_Query_Count_Where_500-4          	   10000	   2150083 ns/op	  909346 B/op	     598 allocs/op
BenchmarkServer_Query_Count_Where_1K-4           	    5000	   4037208 ns/op	  909334 B/op	     598 allocs/op
BenchmarkServer_Query_Count_Where_10K-4          	    5000	   3293030 ns/op	  909253 B/op	     598 allocs/op
BenchmarkServer_Query_Count_Where_100K-4         	    5000	   3381816 ns/op	  908965 B/op	     597 allocs/op
BenchmarkServer_Query_Count_Where_Regex_500-4    	    5000	   3767250 ns/op	  915863 B/op	     678 allocs/op
BenchmarkServer_Query_Count_Where_Regex_1K-4     	    5000	   4134308 ns/op	  915866 B/op	     678 allocs/op
BenchmarkServer_Query_Count_Where_Regex_10K-4    	    5000	   3456730 ns/op	  915869 B/op	     678 allocs/op
BenchmarkServer_Query_Count_Where_Regex_100K-4   	    3000	   3454660 ns/op	 1068348 B/op	    2030 allocs/op
BenchmarkServer_ShowSeries_1-4                   	     100	 101827312 ns/op	18836027 B/op	   69884 allocs/op
BenchmarkServer_ShowSeries_1K-4                  	     200	  83631929 ns/op	18856224 B/op	   70257 allocs/op
BenchmarkServer_ShowSeries_100K-4                	     100	 124226536 ns/op	27889449 B/op	   69344 allocs/op
BenchmarkServer_ShowSeries_1M-4                  	      30	 668649254 ns/op	121761661 B/op	  337101 allocs/op
Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@CAFxX CAFxX changed the title [influxd] Use a sync.Pool to reuse gzip.Writer across requests [influxd] Use sync.Pool to reuse gzip.Writers across requests Feb 5, 2017
@CAFxX CAFxX changed the title [influxd] Use sync.Pool to reuse gzip.Writers across requests [influxd] Use sync.Pool to reuse gzip.Writers across requests Feb 5, 2017
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
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

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

done

@jwilder jwilder assigned joelegasse and unassigned joelegasse Feb 6, 2017
This brings alloc_space down from ~20200M to ~10700M in a run of
go test ./cmd/influxd/run -bench=Server -memprofile=mem.out -run='^$'
@CAFxX
Copy link
Author

CAFxX commented Feb 8, 2017

@jwilder @joelegasse let me know if anything else needs to be changed...

Copy link
Contributor

@joelegasse joelegasse left a 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. :-)

gz.Reset(w)
defer func() {
gz.Close()
gzipWriterPool.Put(gz)
Copy link
Contributor

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.

Copy link
Author

@CAFxX CAFxX Feb 8, 2017

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?

Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Contributor

@joelegasse joelegasse left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@CAFxX
Copy link
Author

CAFxX commented Feb 9, 2017

@joelegasse thanks!
@jwilder I already moved the line in CHANGELOG.md, anything else?

@jwilder jwilder merged commit 0d9fd8a into influxdata:master Feb 9, 2017
@jwilder
Copy link
Contributor

jwilder commented Feb 9, 2017

Thanks @CAFxX!

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

3 participants