Store Gateway: Add experimental configuration to use MAP_POPULATE for indexheader reads. #2019
Conversation
| b []byte | ||
| } | ||
|
|
||
| func OpenMmapFile(path string, populate bool) (*MmapFile, error) { |
There was a problem hiding this comment.
as far as I understand this file is basically copied from Prometheus and then modified, right?
would it be worth it to upstream this change into Prometheus?
There was a problem hiding this comment.
or if we don't want to upstream into upstream prometheus, we could still consider making the change in mimir-prometheus instead of copying the file.
There was a problem hiding this comment.
Given the size of the files, I don't think it's worth the effort doing either right now.
If the testing goes well, then I'll upstream the changes.
There was a problem hiding this comment.
Overall this looks very promising. One small issue that it looks like @replay pointed out as well. I'm excited to see how this helps in a high-traffic cluster
| @@ -3397,6 +3397,13 @@ bucket_store: | |||
| # CLI flag: -blocks-storage.bucket-store.posting-offsets-in-mem-sampling | |||
| [postings_offsets_in_mem_sampling: <int> | default = 32] | |||
|
|
|||
| index_header: | |||
| # (experimental) If enabled, the store-gateway will attempt to pre-populate | |||
There was a problem hiding this comment.
We should move index_header_lazy_loading_enabled to index_header section at some point, if this experiment survives.
There was a problem hiding this comment.
To avoid introducing a breaking change, I would rather do the opposite and use YAML inline for the new index header config struct in the bucket stores config. This would allow us to have a dedicated struct for index header config in the code, but not having to move existing stable flags.
There was a problem hiding this comment.
I'm working on a PR that will touch this area of the code, #2048. I can open a PR to implement this change and update relevant internal (Grafana) configuration.
There was a problem hiding this comment.
Actually nevermind, I'll leave it as is for now and wait to see if our various experiments with index header changes survive.
stevesg
force-pushed
the
indexerheader-reader-experimental-populate
branch
from
efd3b09 to
448fe88
Compare
A previous change (#2019) assumed MAP_POPULATE was available on Darwin. This fixes the build.
Superseded by #2019 Signed-off-by: Nick Pillitteri <[email protected]>
Superseded by #2019 Signed-off-by: Nick Pillitteri <[email protected]>
What this PR does
Introduces a new experimental configuration option (
-blocks-storage.bucket-store.index-header.map-populate-enabled).This enables the use of the
MAP_POPULATEflag whenmmap-ing index-header files in the store-gateway. What this flag does is advise the kernel to (synchronously) pre-fault all pages in the memory region, loading them into the file system cache.Why is this a good idea?
Why is this not perfect?
Rationale about the implementation:
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]