-
Notifications
You must be signed in to change notification settings - Fork 803
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
Replace inmemory index cache to fastcache based implementation #5619
Conversation
} | ||
|
||
c.added = promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | ||
Name: "thanos_store_index_cache_items_added_total", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still use thanos_
prefix to make those metrics compatible with the previous implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with this. Can you add some benchmarks tests ? The juicy numbers like https://github.com/VictoriaMetrics/fastcache#benchmarks are nice
@@ -455,6 +457,8 @@ github.com/alicebob/miniredis/v2 v2.30.4 h1:8S4/o1/KoUArAGbGwPxcwf0krlzceva2XVOS | |||
github.com/alicebob/miniredis/v2 v2.30.4/go.mod h1:b25qWj4fCEsBeAAR2mlb0ufImGC6uH3VlUfb/HS5zKg= | |||
github.com/aliyun/aliyun-oss-go-sdk v2.2.2+incompatible h1:9gWa46nstkJ9miBReJcN8Gq34cBFbzSpQZVVT9N09TM= | |||
github.com/aliyun/aliyun-oss-go-sdk v2.2.2+incompatible/go.mod h1:T/Aws4fEfogEE9v+HPhhw+CntffsBHJ8nXQCwKr0/g8= | |||
github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156 h1:eMwmnE/GDgah4HI848JfFxHt+iPb26b4zyfspmqY0/8= | |||
github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156/go.mod h1:Cb/ax3seSYIx7SuZdm2G2xzfwmv3TPSk2ucNfQESPXM= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using a very old version of bigcache from 2019. Not saying it's wrong per se though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in fastcache library's benchmark https://github.com/VictoriaMetrics/fastcache/blob/master/fastcache_timing_test.go so should not be related.
Sure, I can add some benchmark to compare it with the previous version of inmem cache. |
First benchmark test I added is to test
Updated: it seems that large items are not that good for fast cache because in the test it needs to evict. Causing higher latency in this case. But look at the test results below with higher concurrency it is better. |
Tested store inmem cache with 500 concurrency. This is common scenario in store gateway. Fastcache seems better.
|
Read benchmark with single thread fetch. Fastcache is worse than Thanos cache. Fastcache has much higher mem allocations because it has a sync pool internally when fetching data, due to its implementation details.
Read benchmark with 500 concurrent fetch. This is more close to real scenario where multiple requests sent to Store Gateway and accessing inmem cache at the same time. Even though single fetch request fastcache is slower than Thanos cache, under high concurrency scenario it is 2X faster due to its stripped lock. So overall I think its query performance is not as bad as it looks like.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
68632ce
to
eac871c
Compare
What this PR does:
This PR adds a new inmemory index cache implementation, based on the https://github.com/VictoriaMetrics/fastcache library.
The previous inmemory index cache will be replaced by this one. As the previous inmemory index cache has pretty bad performance under high concurrency environment thanos-io/thanos#6762.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]