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

Enable jemallocator on 64-bit musl builds #1062

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

tavianator
Copy link
Collaborator

@tavianator tavianator commented Jul 12, 2022

Partial fix for #710 and #980.

@sharkdp
Copy link
Owner

sharkdp commented Jul 12, 2022

I think the reason why I disabled them for musl builds back then (#481) was purely based on the fact that jemalloc didn't work in combination with musl. If there is no other reason we can think of (unfortunately, there is not more information in my commit messages / PR description and the Travis logs are not available anymore), this sounds like a good idea - thank you!

@tavianator
Copy link
Collaborator Author

Whoops I force pushed master instead of this PR branch. I just undid that, but @sharkdp you might want to enable branch protection for master in case I fat finger it again :)

@tavianator
Copy link
Collaborator Author

So actually this used to work, but right now the musl version bundled with Rust is too old, leading to tikv/jemallocator#30 tikv/pprof-rs#142

@sharkdp
Copy link
Owner

sharkdp commented Jul 13, 2022

Whoops I force pushed master instead of this PR branch. I just undid that, but @sharkdp you might want to enable branch protection for master in case I fat finger it again :)

Done. No worries.

@tavianator
Copy link
Collaborator Author

Actually CI works for x86-64, it's just broken on my computers. Can be worked around with je_cv_pthread_getname_np=no cargo build ....

The i686 failure might be related to rust-lang/cc-rs#436.

@sharkdp
Copy link
Owner

sharkdp commented Sep 11, 2022

How do we want to proceed here? I think it would be great to get this into the next release. Maybe we can just add a special case for the two platforms that are failing (i.e.: disable jemalloc for those) and add a comment that links to the tickets you mentioned above?

That would still give musl+jemalloc on the more important platforms.

@tavianator
Copy link
Collaborator Author

@sharkdp That sounds good to me! I probably won't have time for a few days so feel free to take this over if you do.

@tavianator tavianator changed the title Enable jemallocator on musl builds Enable jemallocator on 64-bit musl builds Sep 16, 2022
@tavianator tavianator force-pushed the musl-jemalloc branch 3 times, most recently from 6b29b68 to bc438f4 Compare September 19, 2022 18:36
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the update!

@sharkdp sharkdp merged commit e46d402 into sharkdp:master Sep 19, 2022
@tavianator tavianator deleted the musl-jemalloc branch September 19, 2022 19:38
@tavianator
Copy link
Collaborator Author

The remaining two cases are 32-bit arm and x86. I got x86 working in debug builds but release builds still fail to link. I think x86 may be fixed in a future Rust release by upgrading compiler-builtins. I'm less sure about arm: jemalloc wants 1-byte atomics and I'm not sure arm has those before armv7.

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.

2 participants