-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
perf(blooms): Avoid tiny string allocations for insert cache #13487
Conversation
Given that we see really high insert cache hit rate, we can void the string allocations for the cache ownership check. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@owen-d Can you take a look as well? Feel free to merge it thereafter. |
_, found := bt.cache[str] // A cache is used ahead of the SBF, as it cuts out the costly operations of scaling bloom filters | ||
if found { | ||
// To avoid allocations, an unsafe string can be used to check ownership in cache. | ||
str := unsafe.String(unsafe.SliceData(tok), len(tok)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: The []byte
is reused across Next
calls. I wonder if this impacts us here.
// The Token iterator uses shared buffers for performance. The []byte returned by At()
// is not safe for use after subsequent calls to Next()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unsafe string is only used for the membership check, not the assignment in the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point. LGTM
_, found := bt.cache[str] // A cache is used ahead of the SBF, as it cuts out the costly operations of scaling bloom filters | ||
if found { | ||
// To avoid allocations, an unsafe string can be used to check ownership in cache. | ||
str := unsafe.String(unsafe.SliceData(tok), len(tok)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point. LGTM
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
What this PR does / why we need it:
Given that we see really high insert cache hit rate, we can void the string allocations for the cache ownership check.
Special notes for your reviewer:
Running benchmark with command:
$ gotest -v -count=10 ./pkg/storage/bloom/v1/... -test.run=^$ -test.bench=BenchmarkPopulateSeriesWithBloom
Results:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR