Add option to use mimalloc as global allocator#16024
Add option to use mimalloc as global allocator#16024mtreinish wants to merge 6 commits intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
|
I did two full asv runs comparing this to main. The first was on my MBP with an m1 max the second on my x86_64 linux benchmarking system. Looking at the just where asv flagged a significant difference, on the mac: |
|
Then on the linux system: I'm not really worried that one circuit extend benchmark appears to have had a 6μs slowdown compared to the overwhelmingly consistent speed ups flagged on most other benchmarks. That being said this is just the benchmarks where asv thought the results with significant, I did this for brevity. I can share the full output if people are interested. |
There was a problem hiding this comment.
In principle I'm ok with this, but the new requirement to have a C compiler available might cause trouble. At a minimum, we should have a build release note (like the label) and document that in the "building from source" parts of the docs.
I think beyond that, it'd either be nice to either fully commit to the mimalloc feature being configurable in pyext by having setup.py interpret some option (can be envvar) about whether to enable it or not, or to say "no, pyext requires mimalloc" (i.e. get rid of the mimalloc feature and just make it a hard Rust dependency) so we don't have to deal with potentially testing two different allocators.
Fwiw I checked manually, and while cc is already in our lockfile, it's to support building spindle with the loom feature enabled, which we don't (spindle gets pulled in by faer and private-gemm-x86, neither of which turn on loom).
|
I didn't write a release note because I wanted to answer that question specifically. I wrote it this way, optional but used by default everywhere, so we could figure out which way we wanted to go. Specifically if we go optional and disabled by default, I'd write the release note as a build system optional feature to opt-in if you have a C compiler and want better performance (likely triggered via an env var). Then update the CI jobs to set that flag where necessary. Likewise if we decide to go the other way and just require it I'd want to do what you propose and just remove the optional flag from the dependency in the |
|
For I've kicked off the wheels-build run to see if there will be problems on Power or Z, though those don't have to be blockers (but might inform whether we make it optional or not). I was wondering if we need to / should do anything about |
|
and btw, the reason I brought it up specifically was that while the commit message says
I wanted to specifically discuss not providing the option for it all. If we end up making it optional because of problems building from source on things like Windows when doing |
Coverage Report for CI Build 24520719224Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.008%) to 87.475%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions66 previously-covered lines in 6 files lost coverage.
Coverage Stats
💛 - Coveralls |
This commit adds a new feature to the cext and pyext crates to switch the global allocator to use mimalloc [1] instead of the system allocator. mimalloc promises better allocation performance and also lower resident set size over glibc, macOS's libmalloc, or window's allocator [2]. In local benchmarking using mimalloc improves runtime performance on transpiler bencmarks in asv about 10% on my x86_64 benchmarking system. The tradeoff for this is the mimalloc crate [3] internally builds and statically links the mimalloc c library from source as part of cargo's build process. This is all encapsulated in the mimalloc-sys's build.rs using the cc [4] crate so it embeds very cleanly into the build system. However, this does however require that a C compiler is available when building from source. For building the Python package and ironically the C standalone lib this was previously not a requirement. When the feature is enabled it switches the global allocator to use mimalloc. Since this is global for a compilation artifact we can only do it once in qiskit crate, and we'll want to do it either in pyext xor cext based on whether we're building a python extension or c standalone lib so that only the outermost crate is opinionated about which allocator to use. By default this feature is disabled on both crates and we opt-in to it in the build system. For building pyext the setup.py file updates the features we pass to setuptools-rust when building the extension to enable the mimalloc crate and set the global allocator in the pyext code. Similarly, the makefile is updated to do the same for build cext in standalone mode. If we don't want to make this enabled by default (primarily because of the dependency on a C compiler) for building from source we can update the logic in the build scripts to expose an option and only enable in CI for testing and for release artifacts. This is something we'll need to discuss before releasing this option. [1] https://github.com/microsoft/mimalloc [2] Leijen, D., Zorn, B., de Moura, L. (2019). Mimalloc: Free List Sharding in Action. In: Lin, A. (eds) Programming Languages and Systems. APLAS 2019. Lecture Notes in Computer Science(), vol 11893. Springer, Cham. https://doi.org/10.1007/978-3-030-34175-6_13 [3] https://crates.io/crates/mimalloc [4] https://docs.rs/cc/latest/cc/
This commit switches the mimalloc feature to be opt-in by default everywhere. Since building mimalloc requires a C compiler and this does significantly change the requirements for building Qiskit from source, making it opt-in for people that want it is the best course of action for now. This commit adds a new environment variable `QISKIT_BUILD_WITH_MIMALLOC=1` which is used to enable building mimalloc when building Qiskit. The CI jobs are updated to mix mimalloc and not in Python test jobs and also to always enable mimalloc in wheel builds for release.
jakelishman
left a comment
There was a problem hiding this comment.
I think this is the right organisation now - we distribute with mimalloc but the build process is opt-in to avoid needing a C compiler by default.
I only had documentation comments.
This commti adds the mimalloc environment variable to the asv config too so that we are enabling the use of mimalloc when running asv numbers. This will mean the performance numbers we measure with asv are showing the performance with mimalloc. This is desired as it will better capture the performance of the packages we ship on release.
This commit adds a new feature to the cext and pyext crates to switch the global allocator to use mimalloc [1] instead of the system allocator. mimalloc promises better allocation performance and also lower resident set size over glibc, macOS's libmalloc, or window's allocator [2]. In local benchmarking using mimalloc improves runtime performance on transpiler bencmarks in asv about 10% on my x86_64 benchmarking system. The tradeoff for this is the mimalloc crate [3] internally builds and statically links the mimalloc c library from source as part of cargo's build process. This is all encapsulated in the mimalloc-sys's build.rs using the cc [4] crate so it embeds very cleanly into the build system. However, this does however require that a C compiler is available when building from source. For building the Python package and ironically the C standalone lib this was previously not a requirement.
When the feature is enabled it switches the global allocator to use mimalloc. Since this is global for a compilation artifact we can only do it once in qiskit crate, and we'll want to do it either in pyext xor cext based on whether we're building a python extension or c standalone lib so that only the outermost crate is opinionated about which allocator to use. By default this feature is disabled on both crates and we opt-in to it in the build system. Opting in is triggered by an environment variable
QISKIT_BUILD_WITH_MIMALLOC=1. For building pyext the setup.py file updates the features we pass to setuptools-rust when building the extension to enable the mimalloc crate and set the global allocator in the pyext code. Similarly, the makefile is updated to do the same for build cext in standalone mode.[1] https://github.com/microsoft/mimalloc
[2] Leijen, D., Zorn, B., de Moura, L. (2019). Mimalloc: Free List Sharding in Action. In: Lin, A. (eds) Programming Languages and Systems. APLAS 2019. Lecture Notes in Computer Science(), vol 11893. Springer, Cham. https://doi.org/10.1007/978-3-030-34175-6_13
[3] https://crates.io/crates/mimalloc
[4] https://docs.rs/cc/latest/cc/
AI/LLM disclosure