-
Notifications
You must be signed in to change notification settings - Fork 98
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
Native libraries use mimalloc as global allocator #1249
Conversation
Benchmark for 01a59b2Click to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it!
Benchmark for 634f970Click to view benchmark
|
Benchmark for 4acde1fClick to view benchmark
|
Does this mean we have a dependency on a C compiler in our build now also if users want to set up a Q# development environment? (Which I guess we kind of implicitly do anyway as Rust on Windows requires MSVC or C++ Build Tools are installed, Linux generally has a C compiler already... but I'm not sure if that's already a requirement on macOS). |
Yes, you need to have a C compiler handy. I removed the dep on Ninja so that you don't need the generator. You also need cmake on your path. We can get rid of the cmake dep, but we'd need a submodule. |
Benchmark for 52c76e8Click to view benchmark
|
Benchmark for 5c3e644Click to view benchmark
|
Benchmark for 72e6dacClick to view benchmark
|
Benchmark for 1f33ca3Click to view benchmark
|
Benchmark for fc1ff57Click to view benchmark
|
ed83a4a
to
db58fff
Compare
Benchmark for 0579203Click to view benchmark
|
Build warnings on macos when xcode isn't installed will be fixed by rust-lang/cc-rs#1007 |
Benchmark for f40f724Click to view benchmark
|
Benchmark for 7cba09aClick to view benchmark
|
Benchmark for c2294fbClick to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm frankly on the fence about taking this. The perf gains are undeniable, but I have several reservations about the change overall.
- WASM is one our two main use cases and this change wouldn't improve it.
- I'd rather reassess impact after we look into optimizations that reduce allocations across the board, such as Optimize evaluator perf via Control Flow Graph #1261 . I still think we have a lot of low hanging fruit here.
If we do decide it's worth it, these would make the change more palatable for me:
- An easy way to opt out of the CMake/C dependency for local builds. We have external contributors and we love them! One more prereq before you can build at all can be a barrier to engagement.
- Can we use an external implementation via crates.io? This code is a perfect candidate for just using OSS, it's a very general implementation, and exactly the kind of code I wouldn't want to be stuck maintaining.
Benchmark for c29e736Click to view benchmark
|
Benchmark for 8eefe59Click to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the concerns, excited about the perf improvement!
This PR moves the global allocator to MS's mimalloc. The performance gains are significant, and it adds
98KB
to the Python wheel and215KB
to the native dylib used in the python wheel. This dependency adds2.5s
to the build time.We can investigate wasm usage in the future, but this would likely require having a wasi-libc installation available for the build process to give mimalloc a wasi compatible libc to link against.
Benchmarks on Apple M1 Max 64GB