(Double posting from Discord’s #windows channel)
Our downstream LLVM build is recently trying to switch to third party crt allocators too, which brought me to D71786 which raises more questions than it solved.
The patch attempted to support three implementations:
rpmalloc
Probably works, the hard-coded path still works on today’s ToT of rpmalloc, not sure about future.
snmalloc
The hard-coded path no longer works since a long time ago.
mimalloc
Requires manually applying some third-party patch in the issues section from 3 years that remains unmerged by upstream.
On top of that, LLVM_USE_CRT_{target}added by this patch seems to conflict with CMake 3.15’s MSVC_RUNTIME_LIBRARY and maybe could be deprecated / discarded.
I think we should revisit the notion that we should re-implement external allocators build systems in llvm. I support vendoring one implementation in that case.
I think we can first enable it only on Windows, like the original diff did.
IIRC us at hoyoverse saw quite some improvement in our use case with ThinLTO. Though Alexandre probably has more data.
If we stick to the original diff’s design, we don’t ship snmalloc with LLVM itself, the user must explicitly opt-in and build by themselves. Also it has MIT license
I think I was not as clear as I wanted to be. I blame writing comments on my phone
We also use rpmalloc for our toolchain, so I am not keen on losing that as well.
I think patches to fix what’s broken is fine, but I think we should revisit if this logic, where we need to add custom code for each allocator is the right way forward.
I wonder if there is a way to handle this where the user have to build the allocator code outside LLVM and have a flag to point to the static library. I haven’t investigated deeply enough.
I started to work on vendoring rpmalloc and enable it for more platforms than windows at one point - but it requires some amount of effort that I didn’t have time to do that.
We can say with certainty that rpmalloc will be supported in LLVM. It’s the path of least resistance, since there are only a few .c files to build, and the alloc APIs won’t change in the foreseeable future. There are several clients using it, so even if the rpmalloc repo was to change, someone will fix the LLVM integration.
snmalloc and mimalloc were experimental integrations. I am trying to maintain them occasionally to compare the performance and memory usage. Although like @Naville pointed out, they require more maintenance. If someone is willing to contribute with a better integration into LLVM for these allocators, they would be very welcome!
You don’t need to use LLVM_USE_CRT_RELEASE=MT anymore – one can use CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded instead. We should most likely remove LLVM_USE_CRT_* from LLVM.
As for vendoring rpmalloc, we could do it such as [Support] Vendor rpmalloc · aganea/llvm-project@ebc5764 · GitHub
The only benefit I see is that it would allow compiling LLVM everywhere on any platform with a more optimal allocator, without having to dig in the docs, and understand how to achieve that (using rpmalloc would still remain an option). Several years ago, I had this discussion with the LLVM foundation, regarding vendoring & the rpmalloc licence, and even though it doesn’t fit the LLVM licence, it seems that we could vendor it since the author explicitly said that they were willing to donate the code to LLVM. The only thing is that we should go through the RFC process. If anyone is in favor I could do that.
My only reluctance was that LLVM already contains the SCUDO allocator. However folks from SN-Systems have tried in the past to use it, in place of rpmalloc, and we never really got it to work/match the performance of rpmalloc, on Windows at least. I don’t know if the situation changed.
As for performance, I’ve extensively tested all the popular allocators out there, and rpmalloc was the best above all. Even on recent Ubuntu, it was still able to outperform glibc-alloc by a few percent (last time I tried, a few years ago). On Windows, ThinLTO link is definitely not usable unless you go through one of the lock-free allocators supported in LLVM.
ty. I’ll try to submit a patch this week to remove LLVM_USE_CRT_RELEASE first and see how it goes from there.
On a side note, on my recent attempt to build with snmalloc that contains my lastest patch, the build was greeted with a duplicate symbol error. So that’s one more thing worth investigating
A patch from 2019 did add a mention and use of something (which already was in place), which later was deprecated with more recent CMake. When we deprecated LLVM_USE_CRT_{target} we also took care of most of our own internal uses, in e.g. [CMake] Switch the CMP0091 policy (MSVC_RUNTIME_LIBRARY) to the new b… · llvm/llvm-project@c6bd873 · GitHub. After that, there’s nothing relating to the custom allocators that use the deprecated LLVM_USE_CRT_{target} options after that.
That said, as LLVM_USE_CRT_{target} is deprecated and was marked that way in the 17.x branch (which is released now as well), so I guess we could consider removing it at some point. Perhaps that point is now?
But I fail to see the connection to what’s discussed here - the deprecation and switch to MSVC_RUNTIME_LIBRARY (and the associated work to make sure things didn’t break) is already done and complete?
Right, before actually push that pr I was reading the LLVM 16 version of llvm/lib/Support/CMakeLists.txt, which still uses LLVM_USE_CRT for detection. So the initial plan was to remove it and simplify related code before continuing.
After began to actually work on that patch I realized the offending cmake code has been fixed in main and 17+ release. Thought it would still be a good idea to continue the deprecation
