node icon indicating copy to clipboard operation
node copied to clipboard

build node.js using clangcl when under Windows

Open lemire opened this issue 10 months ago • 13 comments

Description

Chrome and Firefox are built with Clang under Windows.

@targos made it possible to build Node.js using clang under Windows, see https://github.com/nodejs/node/pull/35433

At least in some tests, this seems to result in better performance:

                                                          confidence improvement accuracy (*)    (**)    (***)
v8\get-stats.js n=1000000 method='getHeapSpaceStatistics'                 3.36 %      ±10.99% ±14.62%  ±19.03%
v8\get-stats.js n=1000000 method='getHeapStatistics'                     10.35 %      ±10.90% ±14.50%  ±18.87%
v8\serialize.js n=1000000 len=16384                                       1.97 %       ±2.21%  ±3.14%   ±4.53%
v8\serialize.js n=1000000 len=256                                ***      5.59 %       ±1.61%  ±2.30%   ±3.36%
v8\serialize.js n=1000000 len=524288                                     19.28 %      ±48.92% ±76.74% ±130.72%

It is only preliminary, but it is not very difficult to conduct a full examination. There are some maintenance benefits to switching to LLVM under Windows as well. I stress that Node works right now under Windows when compiled with LLVM (credit to @targos).

There is at least one minor issue with ICU: https://github.com/nodejs/node/issues/34201 A tiny patch might be required. I actually expect that it might be the only problem.

I am opening this issue following a request by @mcollina

Further reading: Should Node.js be built with ClangCL under Windows?

Checklist:

  • [x] Add a configuration option in vcbuild.bat to pick from MSVC and CLangCL - Landed in https://github.com/nodejs/node/pull/52870
  • [x] harmonize Clang checks and update V8 warning flags - Landed in https://github.com/nodejs/node/pull/52873
  • [x] Upstream changes in gyp-next to enable setting language standard keys in msvs_settings - Landed upstream in https://github.com/nodejs/gyp-next/pull/252, will eventually get updated in the main project
  • [x] Install ClangCL on Windows hosts in CI - Landed in https://github.com/nodejs/build/pull/3714
  • [ ] Upstream fix for ICU issue for cross-compilation to ARM64 - @StefanStojanovic is looking into this, ideally, we'll fix it upstream and pull it to Node.js as a dependency. PR awaiting one more reviewer https://github.com/unicode-org/icu/pull/3023
  • [x] Upstream fix for V8 ARM64 cross-compilation - Landed upstream in FP16 https://github.com/Maratyszcza/FP16/pull/26 and pulled in V8 in https://chromium-review.googlesource.com/c/v8/v8/+/5553996, will eventually get updated in the main project
  • [x] Implement logic for detecting installed ClangCL version - This is currently hardcoded to the version that can be installed via Visual Studio as a component. @StefanStojanovic has discussed this with @huseyinacacak-janea and they are currently investigating it.
  • [x] Configure Windows CI machines to be able to compile and test ClangCL - This is done, there were 2 build repo PRS: https://github.com/nodejs/build/pull/3714 and https://github.com/nodejs/build/pull/3786
  • [ ] Configure Windows CI jobs to build and test ClangCL - @StefanStojanovic will work on this once most other things are done so we can start gaining confidence for ClangCL builds
  • [ ] Run benchmarks to ensure performances are not worsened by moving to ClangCL from MSVC - This is self-explanatory.
  • [ ] Make sure that the Node.js ecosystem is not facing issues because of the migration - Make sure node-gyp, popular npm packages, etc. are all still working well
  • [ ] Investigate the compilation times between MSVC and ClangCL - This should be taken into serious consideration as we'll be compiling with both MSVC and ClangCL in the CI for at least some time. We should investigate the possibility of using ccache or similar tools with ClangCL.
  • [ ] If there are no reasons not to, switch to using ClangCL as a default compiler for future releases - Potentially Node.js v23 would be a nice time to do this. The same can also be said for dropping x86 support based on https://github.com/nodejs/node/issues/42543#issuecomment-2066905073

lemire avatar May 02 '24 20:05 lemire