[PATCH] D71786: [Support] On Windows, add optional support for {rpmalloc|snmalloc|mimalloc}

Thu Aug 27 02:50:27 PDT 2020

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

Thanks for the update! I'm excited to get this in.

In D71786#2240345 <https://reviews.llvm.org/D71786#2240345>, @aganea wrote:

> In D71786#2193070 <https://reviews.llvm.org/D71786#2193070>, @hans wrote:
>
>> From my point of view, I think it would be fine to only support rpmalloc in this patch. One aspect that Russell touched on is that if we support a bunch of options, it's not clear how well that support will be tested. Supporting only rpmalloc would also simplify the patch a little, and small incremental change is always good I think.
>
> Some users reported that snmalloc perfoms better on a cloud VM. The performance heavily depends on the configuration, whether it's an older Windows 10 build (which has the virtual pages kernel bug), or if it has large pages active. I'd also like to see how they (currently supported allocators) compare vs. SCUDO and vs. tcmalloc3, if someone is willing to port those on Windows.
>
> The changes to support the allocators are quite small. @hans I see this as temporary until we gather more feedback from users, would you be willing to keep all three allocators for now?

Sure, I'm not against it.

================
Comment at: llvm/docs/CMake.rst:476
+  the respective allocator, for example:
+    D:\git> git checkout https://github.com/mjansson/rpmalloc
+    D:\llvm-project> cmake ... -DLLVM_INTEGRATED_CRT_ALLOC=D:\git\rpmalloc
----------------
nit: Shouldn't that be git clone?

================
Comment at: llvm/lib/Support/CMakeLists.txt:69
+  
+  string(REGEX REPLACE "/$" "" LLVM_INTEGRATED_CRT_ALLOC "${LLVM_INTEGRATED_CRT_ALLOC}")
+
----------------
Should this also handle trailing backslash?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71786