perf: Introduce fixed size memory pool for bloom querier #13039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chaudum
force-pushed
the
chaudum/allocator
branch
2 times, most recently
from
1afec2b
to
e49f84b
Compare
github-actions
bot
added
the
type/docs
chaudum
force-pushed
the
chaudum/allocator
branch
from
c19d417
to
cb7d00d
Compare
salvacorts
reviewed
May 28, 2024
docs/sources/shared/configuration.md
Outdated
| # 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"] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think this must be configurable, but it's kind of an "expert setting". We could hide it from the docs
chaudum
force-pushed
the
chaudum/allocator
branch
2 times, most recently
from
a2cbf40
to
96152ca
Compare
github-actions
bot
removed
the
type/docs
chaudum
force-pushed
the
chaudum/allocator
branch
from
3cae039
to
dbb5bed
Compare
This allows for using different implementations of allocators depending on the use case Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
chaudum
force-pushed
the
chaudum/allocator
branch
from
dbb5bed
to
a122b89
Compare
```
loki_mempool_available_buffers_per_slab{slab="..."}
loki_mempool_errors_total{slab="...", reason="..."}
```
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
owen-d
added a commit
that referenced
this pull request
Jun 6, 2024
chaudum
added a commit
that referenced
this pull request
Jun 7, 2024
…ruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
chaudum
added a commit
that referenced
this pull request
Jun 7, 2024
--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
chaudum
added a commit
that referenced
this pull request
Jun 7, 2024
--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
7 tasks
chaudum
added a commit
that referenced
this pull request
Jun 7, 2024
--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
chaudum
added a commit
that referenced
this pull request
Jun 10, 2024
--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
chaudum
added a commit
that referenced
this pull request
Jun 19, 2024
--- Revert "fix(regression): reverts #13039 to prevent use-after-free corruptions (#13162)" This reverts commit 41c5ee2. Signed-off-by: Christian Haudum <[email protected]>
chaudum
added a commit
that referenced
this pull request
Jun 20, 2024
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 <[email protected]>
This was referenced Jul 3, 2024
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[]bytebuffers 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-concurrencyandbloom-gateway.block-query-concurrency) was defined by thebloom.max-query-page-size(and vice versa), because the max page size could be loadedworkers * concurrencytimes 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
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
Allocatorin this context.Checklist
CONTRIBUTING.mdguide (required)featPRs 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.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR