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

storegateway: avoid unnecessary allocation in seriesForRefCacheKey #3179

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

ortuman
Copy link
Contributor

@ortuman ortuman commented Oct 10, 2022

Signed-off-by: Miguel Ángel Ortuño ortuman@gmail.com

What this PR does

According to a storegateway memory profile from the ops cluster, the strconv.FormatUint function used in seriesForRefCacheKey is taking roughly 1.37% of total allocated objects. That seems quite a lot for just an id generator function.

mem-prof

This PR adapts seriesForRefCacheKey to parse series ref id using a stack a buffer to avoid unnecessary allocations.

Below are the benchmark results comparing against the old implementation:

Previous:

BenchmarkStringCacheKeys/series_ref-8         	14691523	        81.81 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14615520	        81.87 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14474442	        81.29 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14345236	        81.50 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14785486	        81.70 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14608633	        81.88 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14584176	        81.90 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14644542	        81.68 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14593413	        81.64 ns/op	      88 B/op	       2 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	14616150	        81.88 ns/op	      88 B/op	       2 allocs/op

Current:

BenchmarkStringCacheKeys/series_ref-8         	16587760	        72.40 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16571724	        72.29 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16491350	        72.28 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16540212	        72.34 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16564346	        72.53 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16501989	        72.40 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16519273	        72.47 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16515446	        72.50 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	16552038	        72.55 ns/op	      64 B/op	       1 allocs/op
BenchmarkStringCacheKeys/series_ref-8         	15845139	        72.78 ns/op	      64 B/op	       1 allocs/op

Comparison:

name                          old time/op    new time/op    delta
StringCacheKeys/series_ref-8    81.7ns ± 1%    72.5ns ± 0%  -11.33%  (p=0.000 n=10+10)

name                          old alloc/op   new alloc/op   delta
StringCacheKeys/series_ref-8     88.0B ± 0%     64.0B ± 0%  -27.27%  (p=0.000 n=10+10)

name                          old allocs/op  new allocs/op  delta
StringCacheKeys/series_ref-8      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)

Which issue(s) this PR fixes or relates to

Fixes N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Miguel Ángel Ortuño <ortuman@gmail.com>
Signed-off-by: Miguel Ángel Ortuño <ortuman@gmail.com>
@ortuman ortuman marked this pull request as ready for review October 10, 2022 14:35
@ortuman ortuman requested a review from a team as a code owner October 10, 2022 14:35
@ortuman ortuman changed the title storegateway: avoid allocation when parsing id in seriesForRefCacheKey storegateway: avoid unnecessary allocation in seriesForRefCacheKey Oct 10, 2022
@colega
Copy link
Contributor

colega commented Oct 10, 2022

Very good catch, thank you!

@colega colega merged commit 297ff59 into main Oct 10, 2022
@colega colega deleted the ortuman/improve-series-cache-id-perf branch October 10, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants