Skip to content

Add option to build Chapel with mimalloc as either the host or target allocator #26246

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

Merged
merged 27 commits into from
Apr 9, 2025

Conversation

jabraham17
Copy link
Member

@jabraham17 jabraham17 commented Nov 15, 2024

Adds the ability to set CHPL_HOST_MEM and/or CHPL_TARGET_MEM to mimalloc.

This PR only provides the option to use mimalloc, it does not change the default from jemalloc

Performance comparisons for CHPL_TARGET_MEM on linux64

test/performance/memory/microMemoryAllocation.chpl

Using --trials=2_000_000

  • jemalloc: 1.38
  • mimalloc: 7.79

test/studies/shootout/binary-trees/binarytrees-inner.chpl

Using --n=21

  • jemalloc: 2.274
  • mimalloc: 0.984

Performance comparisons for CHPL_HOST_MEM on linux64

Compiling examples/hello.chpl

  • jemalloc: 6.0s
  • mimalloc: 5.1s

Compiling Arkouda

  • jemalloc: 1879.510
  • mimalloc: 1827.754

Major changes in this PR

  • Add support to use mimalloc from $CHPL_HOME/third-party/mimalloc or from the system
  • Add a bundled mimalloc 2.1.7

Correctness testing

  • paratest with/without gasnet with CHPL_TARGET_MEM=mimalloc on linux64
  • paratest with/without gasnet with CHPL_HOST_MEM=mimalloc on linux64
  • make check with/without gasnet with CHPL_TARGET_MEM=mimalloc on MacOS
  • make check with/without gasnet with CHPL_HOST_MEM=mimalloc on MacOS
  • Test that fixed heap config with SS11 gives proper errors

Future work:

  • enable fixed heap configs
  • enable asan builds

[Reviewed by @jhh67]

@jabraham17 jabraham17 self-assigned this Feb 6, 2025
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
@jabraham17 jabraham17 merged commit 437476b into chapel-lang:main Apr 9, 2025
10 checks passed
@jabraham17 jabraham17 deleted the use-mimalloc branch April 9, 2025 21:41
jabraham17 added a commit that referenced this pull request Apr 9, 2025
Adds two test configs that run the test suite with mimalloc

To be merged after #26246

[Reviewed by @jhh67]
@bradcray
Copy link
Member

bradcray commented Apr 9, 2025

Note that adding a new third-party package requires updates to the $CHPL_HOME/LICENSE and third-party/README files.

Also, I hate to bring this up again today, but historically, we've expected changes as major as this to occur after some sort of heads-up or discussion with the team (which, admittedly, I may have simply missed). The way we've summarized this policy in the past has been "Developers shouldn't hear about a major change to the project architecture or capabilities for the first time by reading a PR merge notification." The goal of this policy is to operate by consensus and avoid surprises.

@jabraham17
Copy link
Member Author

Note that adding a new third-party package requires updates to the $CHPL_HOME/LICENSE and third-party/README files.

I opened #27088 to remedy this

Also, I hate to bring this up again today, but historically, we've expected changes as major as this to occur after some sort of heads-up or discussion with the team (which, admittedly, I may have simply missed). The way we've summarized this policy in the past has been "Developers shouldn't hear about a major change to the project architecture or capabilities for the first time by reading a PR merge notification." The goal of this policy is to operate by consensus and avoid surprises.

This discussion occurred months ago, so there was admittedly a gap between when that occurred and the PR being merged. This gap was caused by factors out of our control.

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.

3 participants