perf: Introduce fixed size memory pool for bloom querier #13039

chaudum · 2024-05-27T07:27:52Z

What this PR does / why we need it:

This PR introduces a fixed size memory pool for bloom pages that are loaded in the block querier.

The aim of a fixed size pool of []byte buffers is to reduce the amount of allocations, as well as to control the maximum heap size to prevent OOMing of the bloom gateways.

Also, with the current usage of a sync.Pool, the query parallelism (bloom-gateway.worker-concurrency and bloom-gateway.block-query-concurrency) was defined by the bloom.max-query-page-size (and vice versa), because the max page size could be loaded workers * concurrency times at the same time. Most of the time, though, smaller pages are loaded, and therefore concurrency is not optimized.

With the new fixed size memory pool, this problem should be solved. The pool is divided into slabs of different sizes holding different amounts of buffers; a larger amount of small sized buffers and a smaller amount of large sized buffers.

TODO

Make slab sizes and their capacity configurable
Expose metrics for mempool usage

Notes for the reviewer

I'm up for suggestions for naming the configuration options, as well as for the interface name. I don't really like the term Allocator in this context.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

pkg/storage/bloom/v1/mempool/pool.go

pkg/storage/bloom/v1/util.go

salvacorts · 2024-05-28T11:01:50Z

docs/sources/shared/configuration.md

+    # and best effort re-cycling of buffers using Go's sync.Pool), fixed (a
+    # fixed size memory pool with configurable slab sizes, see mem-pool-buckets)
+    # CLI flag: -bloom.memory-management.alloc-type
+    [bloom_page_alloc_type: <string> | default = "dynamic"]


Is this something we want to expose to users? Or do we want to have it to run some experiments and will later remove it?

I think this must be configurable, but it's kind of an "expert setting". We could hide it from the docs

pkg/bloomgateway/bloomgateway_test.go

pkg/storage/bloom/v1/block.go

This allows for using different implementations of allocators depending on the use case Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

``` loki_mempool_available_buffers_per_slab{slab="..."} loki_mempool_errors_total{slab="...", reason="..."} ``` Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

salvacorts

LGTM!

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

)" This reverts commit fc26431.

…#13162)

…ruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

This PR re-introduces the fixed size memory pool that was originally introduced with #13039 but reverted with #13162 Additionally, it removes the package-global variable to store the type of allocator that should be used. Instead, the allocator type is passed during the bloom store creation. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

pull-request-size bot added the size/L label May 27, 2024

chaudum force-pushed the chaudum/allocator branch 2 times, most recently from 1afec2b to e49f84b Compare May 27, 2024 13:56

pull-request-size bot added size/XL and removed size/L labels May 27, 2024

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label May 27, 2024

chaudum force-pushed the chaudum/allocator branch from c19d417 to cb7d00d Compare May 27, 2024 20:08

chaudum marked this pull request as ready for review May 27, 2024 20:08

chaudum requested a review from a team as a code owner May 27, 2024 20:08

chaudum requested a review from salvacorts May 27, 2024 20:08

salvacorts reviewed May 28, 2024

View reviewed changes

chaudum force-pushed the chaudum/allocator branch 2 times, most recently from a2cbf40 to 96152ca Compare June 2, 2024 15:53

github-actions bot removed the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jun 3, 2024

chaudum force-pushed the chaudum/allocator branch from 3cae039 to dbb5bed Compare June 5, 2024 08:53

chaudum added 11 commits June 6, 2024 11:26

Pass a buffer allocator to the bloom querier

6088f56

This allows for using different implementations of allocators depending on the use case Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Free last used page when block querier is closed

43acd27

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add fixed size memory pool allocator

388bd82

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Order imports

bb57c0f

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix linter

5d4a804

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Make slab sizes and capacity for fixed size memory pool configurable

38c7ac4

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Move mempool package

83d3ee7

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove global variable of HeapAllocator

4a4d149

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Return error instead of panicking

8146863

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Hide documentation for advanced memory management

9fbb39a

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix tests after rebasing

a122b89

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum force-pushed the chaudum/allocator branch from dbb5bed to a122b89 Compare June 6, 2024 09:30

Add metrics to memory pool

0a5cd51

``` loki_mempool_available_buffers_per_slab{slab="..."} loki_mempool_errors_total{slab="...", reason="..."} ``` Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum requested a review from salvacorts June 6, 2024 10:07

fixup! Add metrics to memory pool

1121d22

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum added 4 commits June 6, 2024 12:34

fixup! fixup! Add metrics to memory pool

f2c925b

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! fixup! fixup! Add metrics to memory pool

696c4a9

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix CSV flag

25f4381

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Validate memory management configuration at startup

d5d867a

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

salvacorts approved these changes Jun 6, 2024

View reviewed changes

Remove skipped test case

d59ca0b

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum merged commit fc26431 into main Jun 6, 2024
59 checks passed

chaudum deleted the chaudum/allocator branch June 6, 2024 14:41

owen-d added a commit that referenced this pull request Jun 6, 2024

Revert "perf: Introduce fixed size memory pool for bloom querier (#13039

c8966c7

)" This reverts commit fc26431.

owen-d mentioned this pull request Jun 6, 2024

fix(regression): reverts grafana/loki#13039 to prevent use-after-free corruptions #13162

Merged

owen-d added a commit that referenced this pull request Jun 6, 2024

fix(regression): reverts #13039 to prevent use-after-free corruptions (…

41c5ee2

…#13162)

chaudum added a commit that referenced this pull request Jun 7, 2024

Revert "fix(regression): reverts #13039 to prevent use-after-free cor…

47b9ff1

…ruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum mentioned this pull request Jun 7, 2024

perf: Re-introduce fixed size memory pool for bloom querier #13172

Merged

7 tasks

loki-gh-app bot mentioned this pull request Jun 10, 2024

chore(k206): release 3.1.0 #13184

Open

loki-gh-app bot mentioned this pull request Jun 17, 2024

chore(k207): release 3.1.0 #13225

Merged

loki-gh-app bot mentioned this pull request Jun 24, 2024

chore(k208): release 3.1.0 #13291

Open

loki-gh-app bot mentioned this pull request Jul 1, 2024

chore(k209): release 3.1.0 #13356

Open

grafanabot mentioned this pull request Jul 2, 2024

chore: [main] chore(k207): release 3.1.0 #13391

Open

loki-gh-app bot mentioned this pull request Jul 3, 2024

chore(release-3.1.x): release 3.0.1 #13402

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Introduce fixed size memory pool for bloom querier #13039

perf: Introduce fixed size memory pool for bloom querier #13039

chaudum commented May 27, 2024 •

edited

Loading

salvacorts May 28, 2024

chaudum May 29, 2024

salvacorts left a comment

perf: Introduce fixed size memory pool for bloom querier #13039

perf: Introduce fixed size memory pool for bloom querier #13039

Conversation

chaudum commented May 27, 2024 • edited Loading

salvacorts May 28, 2024

Choose a reason for hiding this comment

chaudum May 29, 2024

Choose a reason for hiding this comment

salvacorts left a comment

Choose a reason for hiding this comment

chaudum commented May 27, 2024 •

edited

Loading