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

Use jemalloc in rocksdb #7456

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

akhi3030
Copy link
Collaborator

@akhi3030 akhi3030 commented Aug 22, 2022

We’re using jemalloc in Rust code so it makes sense to do the same in
RocksDB.

Fixes: #6290

@akhi3030 akhi3030 requested a review from a team as a code owner August 22, 2022 14:36
@akhi3030 akhi3030 requested a review from nikurt August 22, 2022 14:36
@mina86
Copy link
Contributor

mina86 commented Aug 22, 2022

This should be enabled only if we enable jemalloc feature on neard

@akhi3030
Copy link
Collaborator Author

This should be enabled only if we enable jemalloc feature on neard

Isn't it already using it? https://github.com/near/nearcore/blob/master/neard/Cargo.toml#L28

@nikurt
Copy link
Contributor

nikurt commented Aug 22, 2022

Do we understand the performance impact of this change?

@akhi3030
Copy link
Collaborator Author

Do we understand the performance impact of this change?

Nope, not yet. Do we have some standard set of tests that we can use?

@nikurt
Copy link
Contributor

nikurt commented Aug 22, 2022

Do we have some standard set of tests that we can use?

That's what I did last time:
https://near.zulipchat.com/#narrow/stream/297873-pagoda.2Fnode/topic/Performance.20impact.20of.20logging/near/283804107

@mina86
Copy link
Contributor

mina86 commented Aug 22, 2022

This should be enabled only if we enable jemalloc feature on neard

Isn't it already using it? https://github.com/near/nearcore/blob/master/neard/Cargo.toml#L28

I mean this feature https://github.com/near/nearcore/blob/master/neard/Cargo.toml#L47 should enable jemalloc for RocksDB.

@matklad
Copy link
Contributor

matklad commented Aug 30, 2022

Do we have some standard set of tests that we can use?

@akhi3030 another approach came up recently here: #7494 (comment)

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM, and I am inclined to merge this without blocking on up-front benchmark:

  • the status quo is that we don't know which allocator rocksdb is useing, 'cause it links the one dynamically from the computer neard is running on
  • any significant perf degradation should be visible in our continous estimation
  • we won't immediately notice change in memory usage, but I think it unlikely to worsen, and we can always revert if we observe something iffy on the betanet.

@akhi3030
Copy link
Collaborator Author

Makes sense. I'll merge.

@near-bulldozer near-bulldozer bot merged commit aaf3797 into near:master Sep 19, 2022
@akhi3030 akhi3030 deleted the akhi3030/use-jemalloc-rocksdb branch September 19, 2022 09:59
@mina86
Copy link
Contributor

mina86 commented Sep 19, 2022

I still think this isn’t correct. We have a cargo feature in neard/Cargo.toml to enable jemalloc. IMO RocksDB jemalloc should be conditionally enabled if that feature is enabled.

@akhi3030
Copy link
Collaborator Author

Can you point me to the relevant bit of code please?

@mina86
Copy link
Contributor

mina86 commented Sep 19, 2022

https://github.com/near/nearcore/blob/master/neard/Cargo.toml#L47

PS. An alternative is to get rid of the feature and always use jemalloc I suppose. Have we actually ever performed any benchmarks to see if jemalloc is faster for us?

@matklad
Copy link
Contributor

matklad commented Sep 19, 2022

Ah, good catch, thanks mina86! Yeah, I think we should just enable jemalloc by default: it's not even the question of "faster", without jemalloc we don't know which allocator we are using.

I don't remeber us performing benchmark for nearcore, but, eg, for rust-analyzer and rust-lang, we did observe jemalloc being measurably faster, and, in ra, we did spend some cycles chasing down issue which boiled down to "the user's allocator is not good".

nikurt pushed a commit that referenced this pull request Nov 9, 2022
We’re using jemalloc in Rust code so it makes sense to do the same in
RocksDB.

Fixes: #6290
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.

Enable jemalloc support in rocksdb
4 participants