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

fix: don't enable jemalloc by default #6913

Closed
wants to merge 1 commit into from
Closed

Conversation

DaniPopes
Copy link
Member

It's not very portable, so we shouldn't enable it by default.

We instruct users to pass the feature when building from source for better performance (https://paradigmxyz.github.io/reth/installation/source.html), and we pass it ourselves in Makefile FEATURES

reth/Makefile

Line 18 in 5cb0258

FEATURES ?= jemalloc asm-keccak

Fixes #6742

@shekhirin
Copy link
Collaborator

Can't we just gate it only for supported targets from https://github.com/gnzlbg/jemallocator instead?

@shekhirin shekhirin added the C-perf A change motivated by improving speed, memory usage or disk footprint label Mar 1, 2024
@gakonst
Copy link
Member

gakonst commented Mar 1, 2024

I'd like to have it on where possible in a smarter way, yeah, can't we use this table as a guide? https://github.com/gnzlbg/jemallocator?tab=readme-ov-file#platform-support

@Rjected
Copy link
Member

Rjected commented Mar 1, 2024

I'd like to have it on where possible in a smarter way, yeah, can't we use this table as a guide? https://github.com/gnzlbg/jemallocator?tab=readme-ov-file#platform-support

note: we might want to switch the names of the deps to tikv-* to make it more clear that we're pulling from
https://github.com/tikv/jemallocator

The crate is linked to the tikv version

@rkrasiuk
Copy link
Member

rkrasiuk commented Mar 2, 2024

supportive of @shekhirin & @gakonst comments, let's enable when possible

@DaniPopes
Copy link
Member Author

DaniPopes commented Mar 2, 2024

How would you even "gate it only for supported targets" in this context? We filter the feature out in make, but you can't do this in Cargo for features.
Also to note that the target may be supported but not all systems will have the same page size set at compile time in jemalloc, which is what prompted this PR, see linked issue.

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

It kind of sucks that jemalloc is not portable, and is causing us to make it non default, it's otherwise very good. But I guess we'll just have to pass --features "jemalloc" everywhere when using cargo cmds

Looking at the original jemalloc issue, it might be possible to have jemallocator build with --with-lg-page=16 or 64

@Rjected
Copy link
Member

Rjected commented Mar 4, 2024

actually, it looks like an environment variable can be set
https://github.com/tikv/jemallocator/blob/af6e6529c087dc1d0ac159372111a859031269f6/jemalloc-sys/README.md?plain=1#L112

@DaniPopes
Copy link
Member Author

Superseded by #7123

@DaniPopes DaniPopes closed this Mar 19, 2024
@DaniPopes DaniPopes deleted the dani/undefault-jemalloc branch March 19, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with jemalloc on system with larger page size
6 participants