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

➕ Optional usage of jemalloc #640

Merged
merged 44 commits into from
Jan 31, 2025
Merged

➕ Optional usage of jemalloc #640

merged 44 commits into from
Jan 31, 2025

Conversation

wlambooy
Copy link
Collaborator

@wlambooy wlambooy commented Jan 20, 2025 •
edited
Loading

Description

This PR introduces the CMake option -DFICTION_ENABLE_JEMALLOC with which the standard implementation of malloc is replaced with replaced with the jemalloc implementation by Jason Evans.

While the usage is not beneficial to every application (and may add runtime due to overhead), it can bring significant runtime improvements to some applications. In particular, it is recommended to use jemalloc for a parallelised application in which allocations are predominantly non-ephemeral.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have added a changelog entry.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@wlambooy
Copy link
Collaborator Author

I'm wondering if you would prefer -DFICTION_JEMALLOC or -DFICTION_ENABLE_JEMALLOC, @marcelwa

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@codecov Codecov
Copy link

codecov bot commented Jan 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.40%. Comparing base (08c6914) to head (d3683f9).
Report is 45 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
- Coverage   98.41%   98.40%   -0.01%     
==========================================
  Files         252      252              
  Lines       40856    40856              
  Branches     1863     1863              
==========================================
- Hits        40207    40205       -2     
- Misses        649      651       +2

see 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08c6914...d3683f9. Read the comment docs.

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa
Copy link
Collaborator

I'm wondering if you would prefer -DFICTION_JEMALLOC or -DFICTION_ENABLE_JEMALLOC, @marcelwa

That's a good question. I don't think we have established consistency rules for that. And it might even be that the current usage is inconsistent even. My gut feeling says -DFICTION_ENABLE_JEMALLOC. What do you prefer?

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@wlambooy
Copy link
Collaborator Author

I'm wondering if you would prefer -DFICTION_JEMALLOC or -DFICTION_ENABLE_JEMALLOC, @marcelwa

That's a good question. I don't think we have established consistency rules for that. And it might even be that the current usage is inconsistent even. My gut feeling says -DFICTION_ENABLE_JEMALLOC. What do you prefer?

I think that makes sense. It's rather like -DFICTION_ENABLE_UNITY_BUILDS and -DFICTION_ENABLE_PCH with it's use case, although I believe it also behaves like a library by making jemalloc.h available, though I don't think we would use that so quickly.

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@wlambooy
Copy link
Collaborator Author

wlambooy commented Jan 28, 2025 via email

@marcelwa
Copy link
Collaborator

marcelwa commented Jan 28, 2025 •
edited
Loading

I'm not figuring out why it keeps failing on macOS + g++. I'm considering excluding it from the CI

If you include this exception in the documentation, I'd be okay with disabling jemalloc under macOS 13.

@wlambooy
Copy link
Collaborator Author

Reminder to self: add documentation

@wlambooy wlambooy marked this pull request as ready for review January 30, 2025 16:42
@wlambooy wlambooy requested a review from marcelwa January 30, 2025 16:42
marcelwa
marcelwa requested changes Jan 30, 2025
View reviewed changes
@wlambooy wlambooy requested a review from marcelwa January 30, 2025 18:10
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@wlambooy wlambooy merged commit 8b2e0f9 into cda-tum:main Jan 31, 2025
49 checks passed
@wlambooy wlambooy deleted the optional-jemalloc-cmake branch January 31, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelwa marcelwa marcelwa approved these changes

Assignees

@wlambooy wlambooy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wlambooy @marcelwa