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

Use direct cache in index reader for symbol values #3557

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 9, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

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

Changes

Apply the same improvement as the upstream Prometheus pr prometheus/prometheus#8265.

The benchmark result is great comparing to the current master.

name                                                             old time/op    new time/op    delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E        16.7s ± 0%     11.8s ± 0%  -29.31%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E       3.95s ± 0%     3.93s ± 0%   -0.53%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     2.44s ± 0%     1.18s ± 0%  -51.63%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E          14.3s ± 0%      6.3s ± 0%  -56.04%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         2.07s ± 0%     2.11s ± 0%   +2.00%

name                                                             old alloc/op   new alloc/op   delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E       4.05GB ± 0%    3.50GB ± 0%  -13.54%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E      1.09GB ± 0%    1.08GB ± 0%   -0.37%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     663MB ± 0%     545MB ± 0%  -17.78%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E         2.67GB ± 0%    2.22GB ± 0%  -16.99%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         631MB ± 0%     631MB ± 0%   +0.00%

name                                                             old allocs/op  new allocs/op  delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E        38.7M ± 0%     15.5M ± 0%  -59.85%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E        412k ± 0%      229k ± 0%  -44.43%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     5.85M ± 0%     2.10M ± 0%  -64.20%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E          30.4M ± 0%      7.2M ± 0%  -76.31%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         38.4k ± 0%     38.5k ± 0%   +0.28%

Verification

Signed-off-by: Ben Ye <yb532204897@gmail.com>
@bwplotka
Copy link
Member

bwplotka commented Dec 9, 2020

Nice. I will check tomorrow but I still think we could just adjust symbolFactor to be something different than 32. If not 1.

This should also help with allocs, wonder what's the main source. I guess we could together Investigate profiles (:

@bwplotka
Copy link
Member

bwplotka commented Dec 9, 2020

Can you double check you are running this tests for more or equal than minute. Especially if something is taking long time, worth to rule out laptop resources saturation

@yeya24
Copy link
Contributor Author

yeya24 commented Dec 10, 2020

Can you double check you are running this tests for more or equal than minute. Especially if something is taking long time, worth to rule out laptop resources saturation

The result above is under 30s bench time. I did another round of benchmarks with 1m bench time and the result is similar.

name                                                             old time/op    new time/op    delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E        14.7s ± 0%      8.3s ± 0%  -43.40%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E       3.50s ± 0%     3.32s ± 0%   -5.09%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     2.55s ± 0%     1.14s ± 0%  -55.33%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E          15.5s ± 0%      6.6s ± 0%  -57.26%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         2.22s ± 0%     2.02s ± 0%   -9.24%

name                                                             old alloc/op   new alloc/op   delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E       4.05GB ± 0%    3.50GB ± 0%  -13.54%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E      1.09GB ± 0%    1.08GB ± 0%   -0.38%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     663MB ± 0%     545MB ± 0%  -17.78%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E         2.67GB ± 0%    2.22GB ± 0%  -16.99%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         631MB ± 0%     631MB ± 0%   -0.00%

name                                                             old allocs/op  new allocs/op  delta
TelemeterRealData_Series/alerts2w/01EPXBGA0413QZMTF4XQ0M4Q3E        38.7M ± 0%     15.5M ± 0%  -59.85%
TelemeterRealData_Series/alerts15s/01EPXBGA0413QZMTF4XQ0M4Q3E        412k ± 0%      229k ± 0%  -44.49%
TelemeterRealData_Series/subssyncs2w/01EPXBGA0413QZMTF4XQ0M4Q3E     5.85M ± 0%     2.10M ± 0%  -64.20%
TelemeterRealData_Series/subs2w/01EPXBGA0413QZMTF4XQ0M4Q3E          30.4M ± 0%      7.2M ± 0%  -76.31%
TelemeterRealData_Series/subs15s/01EPXBGA0413QZMTF4XQ0M4Q3E         38.5k ± 0%     38.4k ± 0%   -0.10%

Also tested with symbolFactor value 1 and 2, but seems using the default value 32 gives a better performance with the existing benchmark.

@bwplotka
Copy link
Member

Awesome, thanks for checking, I want to try something additional before we move to this 🤗

@bwplotka
Copy link
Member

I was thinking about something like this: #3561

Will compare the results from your PR as well (:

@bwplotka
Copy link
Member

bwplotka commented Dec 10, 2020

Interesting, looks like caching is better:

image

@bwplotka
Copy link
Member

Let's go for this!

Since you are maintainer @yeya24 you should have write access to repo and CI now (: Can you restart flaky test? ;p

@csmarchbanks
Copy link

I just want to chime in and say it is awesome to see this optimization helping out other parts of Thanos too!

@yeya24
Copy link
Contributor Author

yeya24 commented Dec 10, 2020

CI passed. Let's try it!

@yeya24 yeya24 merged commit 4abb51a into thanos-io:master Dec 10, 2020
@yeya24 yeya24 deleted the add-cache-for-symbol-values branch December 10, 2020 18:22
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.

3 participants