perf: Re-introduce fixed size memory pool for bloom querier #13172
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/mempool
branch
2 times, most recently
from
2cc9507
to
9ee6b31
Compare
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
chaudum
commented
Jun 19, 2024
| @@ -18,6 +23,7 @@ type Config struct { | |||
| BlocksCache BlocksCacheConfig `yaml:"blocks_cache"` | |||
| MetasCache cache.Config `yaml:"metas_cache"` | |||
| MetasLRUCache cache.EmbeddedCacheConfig `yaml:"metas_lru_cache"` | |||
| MemoryManagement MemoryManagementConfig `yaml:"memory_management" doc:"hidden"` | |||
There was a problem hiding this comment.
Explicitly hide this config from the docs.
salvacorts
approved these changes
Jun 20, 2024
| // While bloomStoreEntry implements the Store interface, this method must never | ||
| // be used directly and therefore can safely return nil. | ||
| func (b *bloomStoreEntry) Allocator() mempool.Allocator { | ||
| return nil |
There was a problem hiding this comment.
nit: I think would be better to no having to satisfy the interface in this way. Maybe we can have
type StoreWithAllocator interface {
Store
Allocator() mempool.Allocator
}Ans only BloomStore implements that.
Having said that, if we don't want to do this, I think we should rather panic here.
There was a problem hiding this comment.
Good idea. I will add the interface.
Move `Allocatior()` function from base interface to "extended" store interface, like it was done for `BloomMetrics()`. Additionally renamed `Store` to `StoreBase` (which is implemented by all stores, also the individual store entries) and `StoreWithMetrics` to `Store`, because this is the main interface that is used. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
salvacorts
approved these changes
Jun 20, 2024
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 re-introduces the fixed size memory pool that was originally introduced with #13039 but reverted with #13162
Special notes for your reviewer:
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