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

store: Preallocate output buffer when encoding postings. #2812

Merged
merged 3 commits into from
Jun 26, 2020
Merged

store: Preallocate output buffer when encoding postings. #2812

merged 3 commits into from
Jun 26, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jun 26, 2020 •
edited
Loading

This PR modifies diffVarintSnappyEncode method to preallocate output buffer based on number of passed postings. This helps to avoid reallocations.

name                      old time/op    new time/op    delta
EncodePostings/10000-4       114µs ± 1%     106µs ± 1%   -6.55%  (p=0.009 n=3+9)
EncodePostings/100000-4     1.11ms ± 1%    1.05ms ± 0%   -5.39%  (p=0.007 n=3+10)
EncodePostings/1000000-4    11.6ms ± 2%    10.7ms ± 0%   -7.21%  (p=0.009 n=3+9)

name                      old alloc/op   new alloc/op   delta
EncodePostings/10000-4      48.5kB ± 0%    13.6kB ± 0%  -71.98%  (p=0.000 n=3+10)
EncodePostings/100000-4      506kB ± 0%     131kB ± 0%  -74.08%  (p=0.000 n=3+10)
EncodePostings/1000000-4    5.86MB ± 0%    1.25MB ± 0%  -78.62%  (p=0.000 n=3+10)

name                      old allocs/op  new allocs/op  delta
EncodePostings/10000-4        18.0 ± 0%       2.0 ± 0%  -88.89%  (p=0.000 n=3+10)
EncodePostings/100000-4       26.0 ± 0%       2.0 ± 0%  -92.31%  (p=0.000 n=3+10)
EncodePostings/1000000-4      36.0 ± 0%       2.0 ± 0%  -94.44%  (p=0.000 n=3+10)

Notes for reviewers: I chose to pass expected number of postings as another parameter. Alternative I considered was introducing an interface with PostingsCount method, which may look nicer, but would add hidden dependency into diffVarintSnappyEncode -- for postings implementing the interface, it would be faster (use less allocs) than for postings not implementing it. Passing extra parameter is more explicit.

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

Verification

Added benchmark.

@pstibrany
Preallocate output buffer when encoding postings.
9ec183c 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pracucci
pracucci approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Results are amazing! Good job 👏 I left a minor nit, but no blocker to me.

pkg/store/postings_codec.go Outdated Show resolved Hide resolved
bwplotka
bwplotka approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

💪

Motto for the week: PREALLOCATE ;p

pkg/store/postings_codec.go Outdated Show resolved Hide resolved
pstibrany added 2 commits June 26, 2020 13:58
@pstibrany
Rename values to length.
2d03d1d 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Renamed count method to length.
697cc3b 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Copy link
Contributor Author

💪

Motto for the week: PREALLOCATE ;p

Indeed :)

@pstibrany
Copy link
Contributor Author

Test and doc failures seem to be unrelated to my changes :-(

@bwplotka bwplotka merged commit fc27af4 into thanos-io:master Jun 26, 2020
@bwplotka
Copy link
Member

Thanks!

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

@bwplotka bwplotka bwplotka approved these changes

@pracucci pracucci pracucci 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.
3 participants
@pstibrany @bwplotka @pracucci