-
Notifications
You must be signed in to change notification settings - Fork 674
Store Gateway: Add experimental configuration to use MAP_POPULATE for indexheader reads. #2019
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
Changes from all commits
1b01f8b
7da1661
7e10511
6ada573
de7e79c
6097f9e
5cb2a7c
1b2a5fc
448fe88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // SPDX-License-Identifier: AGPL-3.0-only | ||
| // Provenance-includes-location: https://github.com/prometheus/prometheus/blob/main/tsdb/fileutil/mmap.go | ||
| // Provenance-includes-license: Apache-2.0 | ||
| // Provenance-includes-copyright: The Prometheus Authors. | ||
|
|
||
| package fileutil | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| type MmapFile struct { | ||
| f *os.File | ||
| b []byte | ||
| } | ||
|
|
||
| func OpenMmapFile(path string, populate bool) (*MmapFile, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as far as I understand this file is basically copied from Prometheus and then modified, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or if we don't want to upstream into upstream prometheus, we could still consider making the change in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return OpenMmapFileWithSize(path, 0, populate) | ||
| } | ||
|
|
||
| func OpenMmapFileWithSize(path string, size int, populate bool) (mf *MmapFile, retErr error) { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "try lock file") | ||
| } | ||
| defer func() { | ||
| if retErr != nil { | ||
| f.Close() | ||
| } | ||
| }() | ||
| if size <= 0 { | ||
| info, err := f.Stat() | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "stat") | ||
| } | ||
| size = int(info.Size()) | ||
| } | ||
|
|
||
| b, err := mmap(f, size, populate) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "mmap, size %d", size) | ||
| } | ||
|
|
||
| return &MmapFile{f: f, b: b}, nil | ||
| } | ||
|
|
||
| func (f *MmapFile) Close() error { | ||
| err0 := munmap(f.b) | ||
| err1 := f.f.Close() | ||
|
|
||
| if err0 != nil { | ||
| return err0 | ||
| } | ||
| return err1 | ||
| } | ||
|
|
||
| func (f *MmapFile) File() *os.File { | ||
| return f.f | ||
| } | ||
|
|
||
| func (f *MmapFile) Bytes() []byte { | ||
| return f.b | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // SPDX-License-Identifier: AGPL-3.0-only | ||
| // Provenance-includes-location: https://github.com/prometheus/prometheus/blob/main/tsdb/fileutil/mmap_unix.go | ||
| // Provenance-includes-license: Apache-2.0 | ||
| // Provenance-includes-copyright: The Prometheus Authors. | ||
|
|
||
| //go:build !windows && !plan9 | ||
| // +build !windows,!plan9 | ||
|
|
||
| package fileutil | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| func mmap(f *os.File, length int, populate bool) ([]byte, error) { | ||
| flags := unix.MAP_SHARED | ||
| if populate { | ||
| flags |= unix.MAP_POPULATE | ||
| } | ||
| return unix.Mmap(int(f.Fd()), 0, length, unix.PROT_READ, flags) | ||
| } | ||
|
|
||
| func munmap(b []byte) (err error) { | ||
| return unix.Munmap(b) | ||
| } |
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.
We should move
index_header_lazy_loading_enabledtoindex_headersection at some point, if this experiment survives.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually nevermind, I'll leave it as is for now and wait to see if our various experiments with index header changes survive.