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

Improve head chunk allocations when reading samples. #3968

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

cyriltovena
Copy link
Contributor

Reading sample is the worst because we are not limited and must return all of them.
Re-using memory will helps ingesters read path.

❯ benchcmp  before.txt after.txt
benchmark                                          old ns/op     new ns/op     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     2935102       1568140       -46.57%
BenchmarkHeadBlockSampleIterator/Size_10000-16     1441980       750856        -47.93%
BenchmarkHeadBlockSampleIterator/Size_8000-16      1146807       602796        -47.44%
BenchmarkHeadBlockSampleIterator/Size_5000-16      698638        370655        -46.95%

benchmark                                          old allocs     new allocs     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     32             16             -50.00%
BenchmarkHeadBlockSampleIterator/Size_10000-16     29             15             -48.28%
BenchmarkHeadBlockSampleIterator/Size_8000-16      28             15             -46.43%
BenchmarkHeadBlockSampleIterator/Size_5000-16      26             15             -42.31%

benchmark                                          old bytes     new bytes     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     2269664       484068        -78.67%
BenchmarkHeadBlockSampleIterator/Size_10000-16     1073582       984           -99.91%
BenchmarkHeadBlockSampleIterator/Size_8000-16      827825        819           -99.90%
BenchmarkHeadBlockSampleIterator/Size_5000-16      475557        786           -99.83%

Signed-off-by: Cyril Tovena [email protected]

Reading sample is the worst because we are not limited and must return all of them.
 Re-using memory will helps ingesters read path.

```
❯ benchcmp  before.txt after.txt
benchmark                                          old ns/op     new ns/op     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     2935102       1568140       -46.57%
BenchmarkHeadBlockSampleIterator/Size_10000-16     1441980       750856        -47.93%
BenchmarkHeadBlockSampleIterator/Size_8000-16      1146807       602796        -47.44%
BenchmarkHeadBlockSampleIterator/Size_5000-16      698638        370655        -46.95%

benchmark                                          old allocs     new allocs     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     32             16             -50.00%
BenchmarkHeadBlockSampleIterator/Size_10000-16     29             15             -48.28%
BenchmarkHeadBlockSampleIterator/Size_8000-16      28             15             -46.43%
BenchmarkHeadBlockSampleIterator/Size_5000-16      26             15             -42.31%

benchmark                                          old bytes     new bytes     delta
BenchmarkHeadBlockSampleIterator/Size_20000-16     2269664       484068        -78.67%
BenchmarkHeadBlockSampleIterator/Size_10000-16     1073582       984           -99.91%
BenchmarkHeadBlockSampleIterator/Size_8000-16      827825        819           -99.90%
BenchmarkHeadBlockSampleIterator/Size_5000-16      475557        786           -99.83%
```

Signed-off-by: Cyril Tovena <[email protected]>
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

LGTM with a few suggestions.

Also, do you mind duplicating this to
https://github.com/grafana/loki/blob/main/pkg/chunkenc/unordered.go#L240-L249

})
}

func unsafeGetBytes(s string) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to put this in it's own pkg somewhere along with a few tests now that we're using it in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't know if two function are worth ? May be an util package ? but then it's a big mess of everything.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 13, 2021
@cyriltovena cyriltovena merged commit 4a8f62b into grafana:main Jul 13, 2021
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