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

Fri Jul 31 09:02:51 PDT 2020

russell.gallop added a comment.

Thanks for your work on this. I've just tried out this version with rpmalloc (1.4.0) checked out out at the top of llvm-project.

  > cmake -G Ninja -DLLVM_ENABLE_PROJECTS=clang;lld -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_INTEGRATED_CRT_ALLOC=<path>/llvm-project/rpmalloc -DLLVM_USE_CRT_RELEASE=MT ..\llvm
  -- The C compiler identification is MSVC 19.26.28806.0
  -- The CXX compiler identification is MSVC 19.26.28806.0
  ...
  > ninja all

And hit a compile issue in rpmalloc:

  C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1426~1.288\bin\Hostx64\x64\cl.exe  /nologo -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Support -I<path>\llvm-project\llvm\lib\Support -Iinclude -I<path>\llvm-project\llvm\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:strictStrings /Oi /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /MDd /Zi /Ob0 /Od /RTC1 /showIncludes /Folib\Support\CMakeFiles\LLVMSupport.dir\F_\git\llvm-project\rpmalloc\rpmalloc\malloc.c.obj /Fdlib\Support\CMakeFiles\LLVMSupport.dir\LLVMSupport.pdb /FS -c <path>\llvm-project\rpmalloc\rpmalloc\malloc.c
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(20): error C2143: syntax error: missing ')' before 'sizeof'
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(20): error C2143: syntax error: missing '{' before 'sizeof'
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(20): error C2059: syntax error: 'sizeof'
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(21): error C2143: syntax error: missing ')' before 'sizeof'
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(21): error C2143: syntax error: missing '{' before 'sizeof'
  <path>\llvm-project\rpmalloc\rpmalloc\malloc.c(21): error C2059: syntax error: 'sizeof'

On the whole I preferred the version of this patch with rpmalloc committed as:

- It lets us test it on a buildbot without adding other repositories. That makes it easier to keep it working across all the support LLVM configurations (as mentioned by @Meinersbur above)
- It could be used in the official LLVM binary releases (https://releases.llvm.org/download.html) (if @hans is happy with that), helping users of that
- All downstream users will use the same alternative memory allocator, so there is less chance of strange downstream issues.
- I can't see enough of an argument for each downstream user having a choice. Any of {rp,sn,mi}malloc will fix the multi-threaded locking issue and boost single threaded perf. Any difference between them is relatively small (particularly if the memory usage of rpmalloc is improved as https://github.com/mjansson/rpmalloc/issues/150)
- If rpmalloc is accepted in upstream LLVM then it actually simplifies licensing for downstream toolchain providers
- The current version (supporting {rpmalloc|snmalloc|mimalloc}) is coupled to those external projects (in llvm/lib/Support/CMakeLists.txt) and I'm not sure how we'd test this integration.

IANAL but I believe that rpmalloc probably *would* be acceptable to the LLVM project under the MIT license. I think that it is up to the LLVM board to decide on licensing issues. If you're happy to go with the rpmalloc route then you could contact them directly (contact details here: http://llvm.org/foundation/), or I would be happy to in support of this if that would help.

Thanks
Russ

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71786