[PATCH] D71786: RFC: [Support] On Windows, add optional support for rpmalloc

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop added a comment.

In D71786#1812388 <https://reviews.llvm.org/D71786#1812388>, @riccibruno wrote:

> Am I the only one who is a little bit uncomfortable with including a replacement to the system allocator in LLVM?


No, you're not. I don't think it's ideal but I'm not sure what the alternative is to remove this bottleneck for ThinLTO on Windows.

It might be possible to use Win32 API (https://docs.microsoft.com/en-gb/windows/win32/api/heapapi/) HeapCreate() to create a heap for each thread and HeapAlloc() with HEAP_NO_SERIALIZE but HeapCreate() heaps are finite so you'd have to handle exhausting that and by then you're practically writing a new memory allocator...

It does feel strange that we have to replace the default allocator to get efficient memory allocation on multiple threads, which doesn't sound like an uncommon requirement to me.

Including the entire rpmalloc source does add quite a bit of source code in one go. Would it be better to keep the rpmalloc source outside LLVM and just include hooks?

With all respect to @maniccoder, there are other replacement memory allocators that could be considered as alternatives (e.g.):
tcmalloc from https://github.com/gperftools/gperftools (as @rnk mentioned above)
https://github.com/microsoft/mimalloc

> It seems to me that this is something that is clearly under the responsibility of the C library. Now this might still be worth doing given the large performance improvement, but yuck...

I wouldn't be happy with unconditionally replacing the C library malloc (without a cmake option). LLVM should certainly continue to be tested, and work, with the default allocator as we wouldn't want to unintentionally develop a dependence on an alternative.

I guess we can hope that the standard implementation improves with increased use of multi-threaded code. glibc for instance added a thread cache to malloc in 2017: https://sourceware.org/ml/libc-alpha/2017-08/msg00010.html


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71786/new/

https://reviews.llvm.org/D71786