streaming store-gateway: avoid copying postings when hashing them #3779

dimitarvdimitrov · 2022-12-20T16:27:47Z

This PR solves two problems:

a bug I just discovered - when we previously constructed the byte slice of the postings we were incorrectly setting the byte position of the octet from the posting
- it was
```
hashable[i+octet]   = byte(posting >> (octet * 8))
```
- it should be
```
hashable[i*8+octet] = byte(posting >> (octet * 8))
```
avoids copying the postings into a byte slice altogether. Instead, it creates a new sliceHeader that it points to the data of the postings and adjusts the length and capacity.

It also changes the hashing functions from SHA1 (20B) + Blake2 (32B) to only Blake2 (32B). The memcached cache key is too small to fit both SHA1 and Blake2. In another change that will also remove the matchers hash from the key I will use Blake2 with 42B

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

pracucci · 2022-12-20T16:33:31Z

pkg/storegateway/indexcache/cache.go

+	byteSlice := make([]byte, 0)
+	slicePtr := (*reflect.SliceHeader)(unsafe.Pointer(&byteSlice))
+	slicePtr.Data = (*reflect.SliceHeader)(unsafe.Pointer(&postings)).Data
+	slicePtr.Len = len(postings) * 8


[nit, non blocking] Instead of hardcoding 8, can we read the size of storage.SeriesRef instead?

yeah i thought about that but the tradeoff was using the unsafe package more 😄

i did the change in e57efad, see what you think

pkg/storegateway/indexcache/cache.go

dimitarvdimitrov · 2022-12-20T16:36:29Z

These are the results of running BenchmarkCanonicalPostingsKey

name                                      old time/op    new time/op    delta
CanonicalPostingsKey/10000_postings-10       202µs ± 0%     107µs ± 2%   -47.12%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000000_postings-10    20.0ms ± 0%    10.5ms ± 0%   -47.48%  (p=0.008 n=5+5)
CanonicalPostingsKey/100000_postings-10     2.00ms ± 0%    1.05ms ± 0%   -47.70%  (p=0.008 n=5+5)
CanonicalPostingsKey/100_postings-10        2.46µs ± 3%    1.27µs ± 3%   -48.13%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000_postings-10       20.8µs ± 0%    10.6µs ± 0%   -48.87%  (p=0.016 n=5+4)
CanonicalPostingsKey/10_postings-10          497ns ± 1%     251ns ± 2%   -49.45%  (p=0.008 n=5+5)

name                                      old alloc/op   new alloc/op   delta
CanonicalPostingsKey/10_postings-10           720B ± 0%       96B ± 0%   -86.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/100_postings-10        1.54kB ± 0%    0.10kB ± 0%   -93.75%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000_postings-10       8.83kB ± 0%    0.10kB ± 0%   -98.91%  (p=0.008 n=5+5)
CanonicalPostingsKey/10000_postings-10      82.6kB ± 0%     0.1kB ± 0%   -99.88%  (p=0.008 n=5+5)
CanonicalPostingsKey/100000_postings-10      803kB ± 0%       0kB ± 0%   -99.99%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000000_postings-10    8.00MB ± 0%    0.00MB ± 0%  -100.00%  (p=0.008 n=5+5)

name                                      old allocs/op  new allocs/op  delta
CanonicalPostingsKey/10_postings-10           6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/100_postings-10          6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000_postings-10         6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/10000_postings-10        6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/100000_postings-10       6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)
CanonicalPostingsKey/1000000_postings-10      6.00 ± 0%      2.00 ± 0%   -66.67%  (p=0.008 n=5+5)

pracucci · 2022-12-20T16:36:33Z

Remember to add the PR number to the existing CHANGELOG entry.

pracucci · 2022-12-20T16:39:19Z

I'm wondering if we could also have a simple unit test on CanonicalPostingsKey(). Like, same input, same hash. Different input, different hash.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

pracucci · 2022-12-20T16:54:17Z

pkg/storegateway/indexcache/cache.go


-	return PostingsKey(base64.RawURLEncoding.EncodeToString(checksum))
+const bytesPerPosting = int(unsafe.Sizeof(storage.SeriesRef(0)) / unsafe.Sizeof(byte(0)))


I hope the size of 1 byte won't change :D

yeah fair. we have the go 1.x guarantee 😄

pracucci

LGTM!

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

56quarters

LGTM, just a few small non-blocking things.

pkg/storegateway/indexcache/cache.go

pkg/storegateway/indexcache/cache_test.go

pkg/storegateway/indexcache/cache.go

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

grafanabot · 2022-12-20T17:46:23Z

The backport to r217 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-3779-to-r217 origin/r217
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 4f73f285aaadb5046a9fb8be8217b2d0f934e598
# Push it to GitHub
git push --set-upstream origin backport-3779-to-r217
git switch main
# Remove the local backport branch
git branch -D backport-3779-to-r217

Then, create a pull request where the base branch is r217 and the compare/head branch is backport-3779-to-r217.

) This PR solves two problems: a bug I just discovered - when we previously constructed the byte slice of the postings we were incorrectly setting the byte position of the octet from the posting it was hashable[i+octet] = byte(posting >> (octet * 8)) it should be hashable[i*8+octet] = byte(posting >> (octet * 8)) avoids copying the postings into a byte slice altogether. Instead, it creates a new sliceHeader that it points to the data of the postings and adjusts the length and capacity. It also changes the hashing functions from SHA1 (20B) + Blake2 (32B) to only Blake2 (32B). The memcached cache key is too small to fit both SHA1 and Blake2. In another change that will also remove the matchers hash from the key I will use Blake2 with 42B (cherry picked from commit 4f73f28)

Use unsafeCastPostingsToBytes for hashing postings

2e710af

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov added the component/store-gateway label Dec 20, 2022

dimitarvdimitrov requested a review from a team as a code owner December 20, 2022 16:27

pracucci reviewed Dec 20, 2022

View reviewed changes

dimitarvdimitrov added 5 commits December 20, 2022 17:40

Report allocs in BenchmarkCanonicalPostingsKey

136c0e1

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Add CHANGELOG.md entry

d081972

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Add tests for CanonicalPostingsKey

cd470f2

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Calculate bytes per posting instead of hardcoding them

e57efad

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Make bytesPerPosting const

522028c

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

pracucci reviewed Dec 20, 2022

View reviewed changes

pracucci approved these changes Dec 20, 2022

View reviewed changes

Remove redundant division by 1

5c6a9bf

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

56quarters approved these changes Dec 20, 2022

View reviewed changes

pkg/storegateway/indexcache/cache.go Outdated Show resolved Hide resolved

pkg/storegateway/indexcache/cache_test.go Show resolved Hide resolved

pkg/storegateway/indexcache/cache.go Show resolved Hide resolved

dimitarvdimitrov and others added 2 commits December 20, 2022 18:21

Update pkg/storegateway/indexcache/cache.go

9106277

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>

Add test case for subset of postings

0306cd7

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov added the backport r217 label Dec 20, 2022

dimitarvdimitrov merged commit 4f73f28 into main Dec 20, 2022

dimitarvdimitrov deleted the dimitar/fix-postings-caching branch December 20, 2022 17:45

grafanabot added the backport-failed label Dec 20, 2022

dimitarvdimitrov mentioned this pull request Dec 20, 2022

Backport 3779 to r217 #3782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streaming store-gateway: avoid copying postings when hashing them #3779

streaming store-gateway: avoid copying postings when hashing them #3779

dimitarvdimitrov commented Dec 20, 2022 •

edited

Loading

pracucci Dec 20, 2022

dimitarvdimitrov Dec 20, 2022

dimitarvdimitrov commented Dec 20, 2022

pracucci commented Dec 20, 2022

pracucci commented Dec 20, 2022

pracucci Dec 20, 2022

dimitarvdimitrov Dec 20, 2022

pracucci left a comment

56quarters left a comment

grafanabot commented Dec 20, 2022


		return PostingsKey(base64.RawURLEncoding.EncodeToString(checksum))
		const bytesPerPosting = int(unsafe.Sizeof(storage.SeriesRef(0)) / unsafe.Sizeof(byte(0)))

streaming store-gateway: avoid copying postings when hashing them #3779

streaming store-gateway: avoid copying postings when hashing them #3779

Conversation

dimitarvdimitrov commented Dec 20, 2022 • edited Loading

pracucci Dec 20, 2022

Choose a reason for hiding this comment

dimitarvdimitrov Dec 20, 2022

Choose a reason for hiding this comment

dimitarvdimitrov commented Dec 20, 2022

pracucci commented Dec 20, 2022

pracucci commented Dec 20, 2022

pracucci Dec 20, 2022

Choose a reason for hiding this comment

dimitarvdimitrov Dec 20, 2022

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

56quarters left a comment

Choose a reason for hiding this comment

grafanabot commented Dec 20, 2022

dimitarvdimitrov commented Dec 20, 2022 •

edited

Loading