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

storegw: Optimized inject label stage of index lookup. #3568

Merged
merged 3 commits into from
Dec 11, 2020
Merged

storegw: Optimized inject label stage of index lookup. #3568

merged 3 commits into from
Dec 11, 2020

Conversation

bwplotka
Copy link
Member 

name                old time/op    new time/op    delta
InjectExtLabels-12    2.16µs ± 4%    0.85µs ± 0%  -60.77%

name                old alloc/op   new alloc/op   delta
InjectExtLabels-12      736B ± 0%      704B ± 0%   -4.35%

name                old allocs/op  new allocs/op  delta
InjectExtLabels-12      2.00 ± 0%      1.00 ± 0%  -50.00%

Overall store benchmarks are not great, something is wrong.

benchstat -delta-test=none _dev/bench/store_symb_dbefore.txt _dev/bench/store_symb_d.txt
name                                                                old time/op    new time/op    delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        841ms ± 0%    1414ms ± 0%  +68.19%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12       284ms ± 0%     352ms ± 0%  +23.89%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12     134ms ± 0%      97ms ± 0%  -27.81%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          3.13s ± 0%     2.11s ± 0%  -32.69%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         1.15s ± 0%     0.97s ± 0%  -15.55%

name                                                                old alloc/op   new alloc/op   delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        335MB ± 0%     410MB ± 0%  +22.51%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12      94.3MB ± 0%    94.9MB ± 0%   +0.71%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12    40.1MB ± 0%    47.8MB ± 0%  +19.27%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          880MB ± 0%    1135MB ± 0%  +29.04%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         228MB ± 0%     228MB ± 0%   +0.00%

name                                                                old allocs/op  new allocs/op  delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        1.52M ± 0%     1.52M ± 0%   +0.00%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12       24.0k ± 0%     24.0k ± 0%   -0.00%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12      190k ± 0%      190k ± 0%   -0.00%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          3.38M ± 0%     3.38M ± 0%   -0.00%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         19.3k ± 0%     19.3k ± 0%   +0.18%

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

@yeya24 yeya24 self-requested a review December 10, 2020 20:52
yeya24
yeya24 reviewed Dec 10, 2020
View reviewed changes
pkg/store/bucket.go Outdated Show resolved Hide resolved
yeya24
yeya24 reviewed Dec 10, 2020
View reviewed changes
pkg/store/bucket.go Outdated Show resolved Hide resolved
@bwplotka
storegw: Optimized inject label stage of index lookup.
ace88ff 
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

Ok now we are talking!

name                                                                old time/op    new time/op    delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        841ms ± 0%     667ms ± 0%  -20.74%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12       284ms ± 0%     330ms ± 0%  +16.34%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12     134ms ± 0%      73ms ± 0%  -45.29%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          3.13s ± 0%     1.69s ± 0%  -45.83%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         1.15s ± 0%     1.05s ± 0%   -8.79%

name                                                                old alloc/op   new alloc/op   delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        335MB ± 0%     313MB ± 0%   -6.48%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12      94.3MB ± 0%    94.1MB ± 0%   -0.21%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12    40.1MB ± 0%    37.4MB ± 0%   -6.71%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          880MB ± 0%     828MB ± 0%   -5.86%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         228MB ± 0%     228MB ± 0%   +0.00%

name                                                                old allocs/op  new allocs/op  delta
TelemeterRealData_Series/alerts2w/01DN3SK96XDAEKRB1AN30AAW6E-12        1.52M ± 0%     1.30M ± 0%  -14.69%
TelemeterRealData_Series/alerts15s/01DN3SK96XDAEKRB1AN30AAW6E-12       24.0k ± 0%     21.9k ± 0%   -8.63%
TelemeterRealData_Series/subssyncs2w/01DN3SK96XDAEKRB1AN30AAW6E-12      190k ± 0%      162k ± 0%  -14.68%
TelemeterRealData_Series/subs2w/01DN3SK96XDAEKRB1AN30AAW6E-12          3.38M ± 0%     2.84M ± 0%  -15.89%
TelemeterRealData_Series/subs15s/01DN3SK96XDAEKRB1AN30AAW6E-12         19.3k ± 0%     19.3k ± 0%   +0.08%

Thanks @yeya24 for spotting issue!

@bwplotka bwplotka marked this pull request as ready for review December 11, 2020 10:20
@bwplotka bwplotka mentioned this pull request Dec 11, 2020
Closed
@bwplotka
Improved helper method instead.
0e372f9 
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

I expanded this to other stores. PTAL 🤗 @brancz @yeya24

@bwplotka
Updated busybox pin.
83f393d 
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
yeya24
yeya24 approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@bwplotka bwplotka merged commit 843482b into master Dec 11, 2020
@bwplotka bwplotka deleted the mergelabels-opt branch December 11, 2020 14:09
khyatisoneji pushed a commit to khyatisoneji/thanos that referenced this pull request Dec 11, 2020
@bwplotka @khyatisoneji
storegw: Optimized inject label stage of index lookup. (thanos-io#3568)
b6db4f8 
* storegw: Optimized inject label stage of index lookup.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

* Improved helper method instead.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

* Updated busybox pin.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
pracucci
pracucci reviewed Jan 7, 2021
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.

Good job! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

@yeya24 yeya24 yeya24 approved these changes

@ahmad-1100 ahmad-1100 ahmad-1100 approved these changes

@kakkoyun kakkoyun Awaiting requested review from kakkoyun

@brancz brancz Awaiting requested review from brancz

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

Successfully merging this pull request may close these issues.
4 participants
@bwplotka @pracucci @yeya24 @ahmad-1100