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.

Sign up for GitHub

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

Jump to bottom

[Feature] feat: jemalloc by default on linux #3154

Merged
merged 2 commits into from
Mar 9, 2024
Merged

[Feature] feat: jemalloc by default on linux #3154

merged 2 commits into from
Mar 9, 2024

Conversation

joske
Copy link
Contributor

@joske joske commented Mar 5, 2024

Enable jemalloc by default on linux.

@joske joske requested a review from ljedrz March 5, 2024 08:58
ljedrz
ljedrz approved these changes Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM as long as we're ok with enabling it only for Linux; ideally we'd use a target-specific feature, but it is not doable yet: rust-lang/cargo#1197.

@joske joske marked this pull request as ready for review March 5, 2024 09:31
@joske joske requested a review from howardwu March 5, 2024 09:43
@howardwu
Copy link
Contributor

howardwu commented Mar 5, 2024

Is this also targetable for macOS?

@joske
Copy link
Contributor Author

joske commented Mar 5, 2024 •
edited
Loading

Yes, it works as far as we've tested, but the jemalloc test suite doesn't completely succeed on macOS, so there may be issues. See platform support on https://crates.io/crates/jemallocator.

Maybe we should even restrict to linux on x86 (though unlikely people will be running snarkOS on aarch64 linux at this point).

@howardwu
Copy link
Contributor

howardwu commented Mar 7, 2024

We can't merge this if this PR introduces issues.

@howardwu
Copy link
Contributor

howardwu commented Mar 7, 2024

Moving back to draft until this is resolved, please prioritize as P0

@howardwu howardwu marked this pull request as draft March 7, 2024 17:16
ljedrz
ljedrz approved these changes Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM, this makes jemalloc default on the platforms that are fully supported: https://crates.io/crates/tikv-jemallocator#platform-support.

@howardwu howardwu marked this pull request as ready for review March 9, 2024 00:27
@howardwu howardwu merged commit 7fe320f into AleoNet:mainnet-staging Mar 9, 2024
23 of 24 checks passed
This was referenced Mar 23, 2024
Merged
Draft
@howardwu howardwu changed the title feat: jemalloc by default on linux [Feature] feat: jemalloc by default on linux Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardwu howardwu howardwu approved these changes

@ljedrz ljedrz ljedrz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joske @howardwu @ljedrz