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

Add feature for using jemalloc allocator. #1269

Closed
wants to merge 1 commit into from

Conversation

eth-p
Copy link
Collaborator

@eth-p eth-p commented Oct 5, 2020

After doing a bit of research and testing, I added the allocator-jemalloc feature. This feature replaces the system allocator with jemalloc. It noticeably improves the startup performance (#951) on my computer (MacOS Mojave), but I'm not sure if it will help too much with other non-Mac platforms.

Benchmark:
Left: Jemalloc
Right: System allocator (bat release build from current master)

image

Note 1:
I would like to make it a default on MacOS, but that's a nightly-only cargo feature.
rust-lang/cargo#1197 (comment)

Note 2:
We should probably find a way to notify the bat Homebrew maintainers once this lands in a release.

@eth-p eth-p requested a review from sharkdp October 5, 2020 22:58
@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2020

Wow - nice. Wouldn't have expected it to make a huge difference for bat, but never cared to measure it. Thanks!

I actually have a bit of experience with this type of change (https://dev.to/sharkdp/an-unexpected-performance-regression-11ai) from the fd project.

but I'm not sure if it will help too much with other non-Mac platforms.

If it's similar as for fd, we will very likely also see a performance boost on Linux. I can run the benchmarks.

MacOS Mojave

Unfortunately, it turned out that there is a problem with jemalloc on MacOS Catalina. You can check out sharkdp/fd#498 for more details. As far as I know, the bug has not been fixed yet. This is why we currently disable jemalloc for macOS in fd.

It also turns out the jemalloc is problematic on some other platforms (sharkdp/fd#636, sharkdp/fd#642).

@eth-p
Copy link
Collaborator Author

eth-p commented Oct 6, 2020

If it's similar as for fd, we will very likely also see a performance boost on Linux. I can run the benchmarks.

👍

Unfortunately, it turned out that there is a problem with jemalloc on MacOS Catalina. You can check out sharkdp/fd#498 for more details. As far as I know, the bug has not been fixed yet. This is why we currently disable jemalloc for macOS in fd.

That is pretty unfortunate... I also don't have a system running Catalina to test it, either. If we can't test it, should we close this, or merge it and let distro maintainers opt-in as needed? They would likely know how jemalloc runs in their own environment better than anyone.

It also turns out the jemalloc is problematic on some other platforms (sharkdp/fd#636, sharkdp/fd#642).

I'm not too surprised about that, since there's a lot of variables at play and all. I'm definitely glad I chose to make it an optional feature, though.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2020

Hm, interesting. I don't see any significant performance difference when using jemalloc except for the jquery benchmark where the jemalloc version is slightly slower (only 3%, but statistically significant).

jemalloc is really the only difference in your benchmarks? Is your bat --cache-dir empty?

We know from our profiling results that the majority of the time in the bat-startup is spent during deserialization. Does it even use allocations? (certainly could be, but I don't see this with strace.. but I'm not sure if I'm doing it correctly)

@sharkdp
Copy link
Owner

sharkdp commented Oct 29, 2020

I'm inclined to close this for now, unless we are sure that jemalloc is the significant factor here.

Having an optional jemalloc dependency might potentially confuse package maintainers, so I'd like to avoid adding it if there are no gains.

If there are definite performance improvements, on the other hand, I'm happy to merge this.

@sharkdp
Copy link
Owner

sharkdp commented Dec 28, 2020

Closing this for now, but happy to reopen if someone wants to make progress on this.

@sharkdp sharkdp closed this Dec 28, 2020
@eth-p eth-p mentioned this pull request May 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants