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

chore: Use a forked version of leveldb-sys #7522

Merged
merged 4 commits into from May 20, 2021
Merged

chore: Use a forked version of leveldb-sys #7522

merged 4 commits into from May 20, 2021

Conversation

blt
Copy link
Contributor

@blt blt commented May 20, 2021

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

@blt blt requested a review from jszwedko May 20, 2021 00:26
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

I unarchived https://github.com/timberio/leveldb-sys and made sure everyone has access. I think you should be able to switch it over there if you like.

This changed Cargo.lock much more than I anticipated. Did you mean to upgrade all of those packages? The Tokio 1.6 upgrade #7477 is currently blocked on a test failure (which I also see on this PR).

blt added 2 commits May 20, 2021 10:45
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>
Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@blt blt requested a review from jszwedko May 20, 2021 18:20
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 this is still modifying Cargo.lock much more than I would expect for this change.

@blt
Copy link
Contributor Author

blt commented May 20, 2021

thinking this is still modifying Cargo.lock much more than I would expect for this change.

I ended up needing to run cargo update when introducing the patch. That's where most of the lockfile churn comes from.

Now that you mention it, let me see if I can't avoid running cargo update. I think I see a way.

@blt
Copy link
Contributor Author

blt commented May 20, 2021

@jszwedko ah, hmm. No dice. When adding the patch I am forced to run cargo update. I'd be game to run cargo update in another PR and rebase this one after that.

@jszwedko
Copy link
Member

I think you could just do cargo update -p leveldb-sys?

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@jszwedko
Copy link
Member

@blt I think I did it in fdc7891

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@blt
Copy link
Contributor Author

blt commented May 20, 2021

I think you could just do cargo update -p leveldb-sys?

D'oh

@blt blt merged commit 1e54aca into vectordotdev:master May 20, 2021
@blt blt deleted the leveldb_fork branch May 20, 2021 20:38
@ktff ktff mentioned this pull request May 28, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fork leveldb to apply lower mmap limit
2 participants