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

Optimize sort keys by server in memcache client #8026

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 29, 2024

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

Changes

When fetching a lot of keys from index cache using memcached, I see in our store gateway 13% CPU spent on MemcachedJumpHashSelector.PickServer. Among them most of the CPU spent on Each which is the function to get all the memcached server addresses.

image

For each key we call PickServer once and each PickServer call requires getting all memcached server addresses by calling Each function. https://github.com/thanos-io/thanos/blob/main/pkg/cacheutil/memcached_server_selector.go#L65

This is unnecessary because memcache server addresses update infrequently. When there are a lot of keys to fetch through memcached, it can be expensive to call Each every time.

This PR I try to cache server addresses in SetServers so that we don't have to call Each to get addresses every time. Instead we call Each once in the sortKeysByServer method to get all addresses and do jump hash using the fetched addresses to help reduce lock contention.

Verification

Signed-off-by: Ben Ye <[email protected]>
Copy link
Contributor

@harry671003 harry671003 left a comment

Choose a reason for hiding this comment

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

Thanks. Change looks good overall.
Lint is failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants