Skip to content
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

Fork leveldb to apply lower mmap limit #7514

Closed
jszwedko opened this issue May 19, 2021 · 8 comments · Fixed by #7522
Closed

Fork leveldb to apply lower mmap limit #7514

jszwedko opened this issue May 19, 2021 · 8 comments · Fixed by #7522
Assignees
Labels
domain: buffers Anything related to Vector's memory/disk buffers type: enhancement A value-adding code change that enhances its existing functionality.

Comments

@jszwedko
Copy link
Member

While investigating #7246 and other issues, we've identified that leveldb will mmap up to 1000 files at 4 MB per file. This commonly results in users running into OOM issues using disk buffers.

Unfortunately, this value is not configurable so, to modify it, we have to fork leveldb-sys to change it here:

https://github.com/skade/leveldb-sys/blob/c5383aaf9e264041c951e9a2aa7381613d0d0dba/deps/leveldb-1.22/util/env_posix.cc#L46

@jszwedko jszwedko added type: enhancement A value-adding code change that enhances its existing functionality. domain: buffers Anything related to Vector's memory/disk buffers labels May 19, 2021
@blt blt self-assigned this May 19, 2021
blt added a commit to blt/leveldb-sys that referenced this issue May 19, 2021
Relates to vectordotdev/vector#7514

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@blt
Copy link
Contributor

blt commented May 19, 2021

Hmm, well. I have forked leveldb and can confirm that only the specified number of files are being mapped into the process, which is good. What's bad is vector's resident mem is still roughly equivalent to the disk buffer size. Looking into this further.

@blt
Copy link
Contributor

blt commented May 19, 2021

Interesting, so check this out:

Screenshot from 2021-05-19 16-09-51

It looks like, because my system has no memory pressure, linux is being lazy about reclaiming pages but what happens is the unreclaimed space slowly fills up with retries. Allocation point appears to be src/sinks/util/retries.rs:150. Raw massif:

massif.out.778423.zip

I'm going to run vector for a long period of time now, see if these start growing as I expect. Afterward I'll run vector in a memory constrained environment.

@blt
Copy link
Contributor

blt commented May 19, 2021

Here's a longer run with no memory pressure:

Screenshot from 2021-05-19 16-37-40
massif.out.784090.zip

I had expected the retry memory to continue to grow, so the linear curve here surprises me.

@blt
Copy link
Contributor

blt commented May 20, 2021

Alright, as desired when run like so:

> systemd-run --scope --property MemoryHigh=250M --property MemoryMax=300M ./target/release/vector -qqq --config /tmp/disk-blowup.toml

vector now stays within the 300M limit, which previously was not true. Virtual memory use is still quite high but resident size is within boundary, so this should address #7246. I'll have a PR shortly.

@jszwedko
Copy link
Member Author

Interesting, so check this out:

Screenshot from 2021-05-19 16-09-51

It looks like, because my system has no memory pressure, linux is being lazy about reclaiming pages but what happens is the unreclaimed space slowly fills up with retries. Allocation point appears to be src/sinks/util/retries.rs:150. Raw massif:

massif.out.778423.zip

I'm going to run vector for a long period of time now, see if these start growing as I expect. Afterward I'll run vector in a memory constrained environment.

Hmmm, interesting. Do you think there is an issue there? Should we be piling up retries like that?

@blt
Copy link
Contributor

blt commented May 20, 2021

It’s a good question. I don’t know. It’s something that users will only notice if their vector doesn’t have memory pressure, which most will have. I’d say it’s more of a curiosity until we learn otherwise, a potential place to optimize away clones if someone has a clever notion.

@portante
Copy link

https://github.com/skade/leveldb-sys/blob/c5383aaf9e264041c951e9a2aa7381613d0d0dba/deps/leveldb-1.22/util/env_posix.cc#L46

Perhaps its worth filing an issue against leveldb?

Seems like maintaining a fork can lead to other problems.

@blt
Copy link
Contributor

blt commented May 20, 2021

https://github.com/skade/leveldb-sys/blob/c5383aaf9e264041c951e9a2aa7381613d0d0dba/deps/leveldb-1.22/util/env_posix.cc#L46

Perhaps its worth filing an issue against leveldb?

Seems like maintaining a fork can lead to other problems.

This is a good question. There's been discussion like this in upstream -- see google/leveldb#866 -- and their suggestion is to fork. Realistically leveldb is an awkward fit for what we need in vector. It's a KV store and we just need a disk-backed queue. My expectation is that we'll be reworking the buffer code entirely and removing leveldb within the year.

blt added a commit to vectordotdev/leveldb-sys that referenced this issue May 20, 2021
Relates to vectordotdev/vector#7514

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@blt blt closed this as completed in #7522 May 20, 2021
blt added a commit that referenced this issue May 20, 2021
* Use a forked version of leveldb-sys

The forked version of leveldb-sys patched in here limits leveldb to a maximum of
10 mmap'ed LDB files, reducing the total memory burden of vector. Upstream
leveldb will map 1000 files, so there's quite a difference in memory
usage. Unfortunately this 10 is not configurable.

At the moment the fork is under my personal Github account. I think we ought to
fork to timberio -- we have an archived version -- and pin the patch to a SHA. I
don't have enough juice in the Github org to achieve this.

Resolves #7514

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>

* switch to timberio/leveldb-sys

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>

* Revert cargo updates

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

* Remove accidental Cargo.lock

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Co-authored-by: Jesse Szwedko <jesse@szwedko.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: buffers Anything related to Vector's memory/disk buffers type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants