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

Faster scanning of memcache responses #154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bboreham
Copy link

Motivation for this change is at grafana/mimir#285 (comment), where a large program spends 12% of its CPU in this one function.

Re-coded scanGetResponseLine() using IndexByte() instead of SScanf().
Also added a benchmark.

name                   old time/op    new time/op    delta
ScanGetResponseLine-4    4.06µs ± 1%    0.13µs ± 4%  -96.89%  (p=0.008 n=5+5)

name                   old alloc/op   new alloc/op   delta
ScanGetResponseLine-4      128B ± 0%       24B ± 0%  -81.25%  (p=0.008 n=5+5)

name                   old allocs/op  new allocs/op  delta
ScanGetResponseLine-4      7.00 ± 0%      1.00 ± 0%  -85.71%  (p=0.008 n=5+5)

This PR is broadly similar to #74, but about twice as fast on my benchmark.
The benchmark in #74 spends <2% of its time in scanGetResponseLine() so doesn't show the effect so well.

The partial copy of strings.Cut() in this PR allows the code to still build with pre-1.18 Go.

Copy link
Owner

@bradfitz bradfitz left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

memcache/memcache.go Show resolved Hide resolved
memcache/memcache.go Show resolved Hide resolved
@bboreham
Copy link
Author

bboreham commented Nov 2, 2022

I added some tests and checked for int overflow.

I'm not too familiar with what memcached responses look like; I made a brief attempt to find similar tests for other languages and came up blank, so did the edge cases.

@pior
Copy link

pior commented Oct 9, 2024

This looks like a great improvement, I think @bboreham addressed both comments, do you think you can take another look @bradfitz ?

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