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.

Sign up for GitHub

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

Jump to bottom

Reduced memory allocations in readIndexRange() #2807

Merged
merged 2 commits into from
Jun 25, 2020
Merged

Reduced memory allocations in readIndexRange() #2807

merged 2 commits into from
Jun 25, 2020

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Jun 25, 2020 •
edited
Loading

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

I'm investigating BucketStore memory allocations and I've noticed that, in our context (Cortex), most of the memory is allocated by readIndexRange() (see profile below). Investigating it further, I've noticed the use of ioutil.ReadAll() which is quite bad when it comes to memory allocations if the length of the reader is known in advance (like this case), so I've tried to optimise it preallocating the buffer with the right size in advance.

Screen Shot 2020-06-25 at 17 24 59

Benchmark result

My old laptop has quite bad consistent CPU performances, so I wouldn't focus too much on CPU +/- (different benchmark runs showed different results). Moreover, from a CPU perspective, nothing should be worse.

benchmark                                                                        old ns/op     new ns/op     delta
BenchmarkBucketIndexReader_ExpandedPostings/n="1"-4                              4070602       3734140       -8.27%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j="foo"-4                      31325200      29048721      -7.27%
BenchmarkBucketIndexReader_ExpandedPostings/j="foo",n="1"-4                      31159607      29626112      -4.92%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j!="foo"-4                     23203504      23041595      -0.70%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".*"-4                            149680474     150642697     +0.64%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".+"-4                            545142975     512976295     -5.90%
BenchmarkBucketIndexReader_ExpandedPostings/i=~""-4                              581137212     563934868     -2.96%
BenchmarkBucketIndexReader_ExpandedPostings/i!=""-4                              427861359     416124371     -2.74%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",j="foo"-4              120270089     143767243     +19.54%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",i!="2",j="foo"-4       127693796     127794129     +0.08%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!=""-4                        210909611     205254609     -2.68%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!="",j="foo"-4                240871660     226334197     -6.04%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",j="foo"-4              347059094     305314452     -12.03%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~"1.+",j="foo"-4             47807330      54461125      +13.92%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!="2",j="foo"-4       335549054     316794654     -5.59%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!~"2.*",j="foo"-4     361758475     355419912     -1.75%
BenchmarkBucketIndexReader_ExpandedPostings/i=~"0|1|2"-4                         83651         86805         +3.77%

benchmark                                                                        old allocs     new allocs     delta
BenchmarkBucketIndexReader_ExpandedPostings/n="1"-4                              80             70             -12.50%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j="foo"-4                      116            93             -19.83%
BenchmarkBucketIndexReader_ExpandedPostings/j="foo",n="1"-4                      116            93             -19.83%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j!="foo"-4                     115            92             -20.00%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".*"-4                            107            91             -14.95%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".+"-4                            300169         300155         -0.00%
BenchmarkBucketIndexReader_ExpandedPostings/i=~""-4                              300158         300140         -0.01%
BenchmarkBucketIndexReader_ExpandedPostings/i!=""-4                              300162         300147         -0.00%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",j="foo"-4              123            100            -18.70%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",i!="2",j="foo"-4       155            121            -21.94%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!=""-4                        300180         300155         -0.01%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!="",j="foo"-4                300211         300175         -0.01%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",j="foo"-4              300221         300180         -0.01%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~"1.+",j="foo"-4             33516          33480          -0.11%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!="2",j="foo"-4       300249         300201         -0.02%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!~"2.*",j="foo"-4     333601         333561         -0.01%
BenchmarkBucketIndexReader_ExpandedPostings/i=~"0|1|2"-4                         123            120            -2.44%

benchmark                                                                        old bytes     new bytes     delta
BenchmarkBucketIndexReader_ExpandedPostings/n="1"-4                              11379940      10087638      -11.36%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j="foo"-4                      23528145      13464286      -42.77%
BenchmarkBucketIndexReader_ExpandedPostings/j="foo",n="1"-4                      23528099      13464264      -42.77%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",j!="foo"-4                     23529126      13465230      -42.77%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".*"-4                            285362226     238260203     -16.51%
BenchmarkBucketIndexReader_ExpandedPostings/i=~".+"-4                            332681812     286776036     -13.80%
BenchmarkBucketIndexReader_ExpandedPostings/i=~""-4                              190785972     97775716      -48.75%
BenchmarkBucketIndexReader_ExpandedPostings/i!=""-4                              332680386     286774434     -13.80%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",j="foo"-4              25134602      15070839      -40.04%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".*",i!="2",j="foo"-4       27232804      15876465      -41.70%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!=""-4                        127412713     80214561      -37.04%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i!="",j="foo"-4                139561032     83591686      -40.10%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",j="foo"-4              139562714     83592298      -40.10%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~"1.+",j="foo"-4             34012422      17921050      -47.31%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!="2",j="foo"-4       141660368     84398512      -40.42%
BenchmarkBucketIndexReader_ExpandedPostings/n="1",i=~".+",i!~"2.*",j="foo"-4     147866818     91897509      -37.85%
BenchmarkBucketIndexReader_ExpandedPostings/i=~"0|1|2"-4                         16104         12264         -23.85%

Verification

Existing tests + benchmark.

pstibrany
pstibrany approved these changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Nice!

@brancz brancz merged commit a9e60f2 into thanos-io:master Jun 25, 2020
GiedriusS
GiedriusS approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@pracucci pracucci deleted the save-mem-allocations-in-read-index-range branch June 26, 2020 07:43
@bwplotka
Copy link
Member

paulfantom added a commit to paulfantom/thanos that referenced this pull request Jul 9, 2020
@paulfantom
Merge remote-tracking branch 'upstream/release-0.14' into 0.14
9c227bc 
* upstream/release-0.14: (46 commits)
  Cut release v0.14.0-rc.1 (thanos-io#2853)
  Query: correctly marshal errors to JSON and ignore if nil (thanos-io#2848)
  ci: Manually download promu in crossbuild stage (thanos-io#2828)
  Cut release v0.14.0-rc.0 (thanos-io#2826)
  Soft cut changelog on master to indicate v0.14.0 being in progress (thanos-io#2824)
  Update ThanosReceiveNoUpload to select sum == 0 (thanos-io#2819)
  receive: Added more observability, fixed leaktest, to actually check leaks ): (thanos-io#2817)
  Query: always return a string in the `lastError` field (thanos-io#2809)
  Added missing CHANGELOG entry for PR 2613 (thanos-io#2820)
  receive: Fixed small options race; Removed unused StartTime feature. (thanos-io#2816)
  go.mod: Bump Prometheus to current latest (thanos-io#2814)
  Implement CLI Flags page in React UI (thanos-io#2796)
  Improve ThanosReceiveNoUpload to only alert on current instances
  store: Preallocate output buffer when encoding postings. (thanos-io#2812)
  compact: introduce flag --block-viewer.global.sync-block-interval (thanos-io#2752)
  docs: compact: add blurb about how retention policy works (thanos-io#2808)
  Reduced memory allocations in readIndexRange() (thanos-io#2807)
  ui: Add Stores page to React UI (thanos-io#2754)
  Added Kemal to Maintainer Role; Kemal is volounteering to be next release shephard (thanos-io#2804)
  proposal: Add scalable rule storage proposal (thanos-io#2661)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@GiedriusS GiedriusS GiedriusS approved these changes

@brancz brancz brancz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pracucci @bwplotka @pstibrany @GiedriusS @brancz