Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build node.js using clangcl when under Windows #52809

Open
8 of 13 tasks
lemire opened this issue May 2, 2024 · 13 comments
Open
8 of 13 tasks

build node.js using clangcl when under Windows #52809

lemire opened this issue May 2, 2024 · 13 comments
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Comments

@lemire
Copy link
Member

lemire commented May 2, 2024

Description

Chrome and Firefox are built with Clang under Windows.

@targos made it possible to build Node.js using clang under Windows, see #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: #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:

@mcollina
Copy link
Member

mcollina commented May 2, 2024

I recall that it was recommended by MS folks to build using clang on Windows.

@RedYetiDev RedYetiDev added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels May 2, 2024
@lemire
Copy link
Member Author

lemire commented May 2, 2024

One issue to consider is that Google switched over so they are developing V8 under ClangCL.

@StefanStojanovic
Copy link
Contributor

StefanStojanovic commented May 13, 2024

I was thinking about opening an umbrella issue with a checklist or something like that for all things related to enabling ClangCL compilation on Windows, but since there is already this issue if you agree @lemire we can use it for that (if not, I'll create a separate issue with the content of this message).

Anyway, the following is (not sorted according to priority) a list of things done, being worked on, or that should be worked on before stating ClangCL is officially supported for compiling Node.js on Windows:

  • Add a configuration option in vcbuild.bat to pick from MSVC and CLangCL - This landed in build: add option to enable clang-cl on Windows #52870
  • harmonize Clang checks and update V8 warning flags - This landed in build: harmonize Clang checks and update V8 warning cflags #52873
  • Upstream changes in gyp-next to enable setting language standard keys in msvs_settings - This landed upstream in feat: support language standard keys in msvs_settings gyp-next#252, will eventually get updated in the main project
  • Install ClangCL on Windows hosts in CI - This landed in ansible: add ClangCL to VS2022 build#3714
  • Fix ICU issue for cross-compilation to ARM64 - I'm looking into this, ideally, we'll fix it upstream and pull it to Node.js as a dependency.
    EDIT Opened a draft PR in ICU ICU-21633 Fix ClangCL cross-compilation on Windows unicode-org/icu#3013
  • Fix ARM64 cross-compilation - WIth the latest V8 updates (v12.2 and v12.4) V8 has a new third-party dependency which is causing issues in Node.js. I'm looking into this, ideally, we'll fix it upstream and pull it to Node.js as a dependency.
  • Implement logic for detecting installed ClangCL version - This is currently hardcoded to the version that can be installed via Visual Studio as a component. I've discussed this with @huseyinacacak-janea and he is currently investigating it
  • Configure Windows CI jobs to build and test ClangCL - I 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 Should we drop support for 32 bit Windows? #42543 (comment)

I might have missed something, but most things should be covered here. If anyone notices something missing or wrong, please let me know.

cc: @targos @nodejs/platform-windows

@lemire
Copy link
Member Author

lemire commented May 13, 2024

@StefanStojanovic It definitively makes sense to leverage the current issue.

@StefanStojanovic
Copy link
Contributor

@StefanStojanovic It definitively makes sense to leverage the current issue.

Great! One issue I see with it is that if the comment gets edited often (eg. I edited it just now to add a PR I opened) it will probably not be sent as a notification, plus I'm probably the only one who can edit it. If anyone has a better idea eg. a GitHub Project or something like that, I'm all for it.

@lemire
Copy link
Member Author

lemire commented May 13, 2024

@StefanStojanovic I can edit your comment, but another take on it would be to edit the issue first post (that is, the text I have written). Do you have edit access?

I think you should be able to edit the issue... I think you have access... don't you?

That is, I don't own this issue, it should be a collective one.

@StefanStojanovic
Copy link
Contributor

@StefanStojanovic I can edit your comment, but another take on it would be to edit the issue first post (that is, the text I have written). Do you have edit access?

I think you should be able to edit the issue... I think you have access... don't you?

That is, I don't own this issue, it should be a collective one.

Nope, I do not see edit. This is what I see. Not sure GH supports collective issues, would be great if it did...

image

@lemire
Copy link
Member Author

lemire commented May 13, 2024

@joyeecheung Can you help? I think we would like @StefanStojanovic to have the ability to edit this issue.

@H4ad
Copy link
Member

H4ad commented May 13, 2024

@lemire He will have the ability to edit once he becomes a nodejs collaborator (probably next week)

@lemire
Copy link
Member Author

lemire commented May 13, 2024

@StefanStojanovic Given @H4ad's answer, I recommend just waiting a week or so. ;-)

@hocheung-chromium
Copy link

https://issues.chromium.org/issues/341803745

fp16 in v8 has been updated to the latest version, I think the blocker on the v8 side can be removed.

nodejs-github-bot pushed a commit that referenced this issue May 28, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Jun 11, 2024
PR-URL: #53228
Refs: #52809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Jun 20, 2024
PR-URL: #53228
Refs: #52809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53228
Refs: nodejs#52809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@joyeecheung
Copy link
Member

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.

We should probably update this point since V8 announced that they will drop MSVC support in September: https://groups.google.com/g/v8-dev/c/8mSxb2UriSU

bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53228
Refs: nodejs#52809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to targos/node that referenced this issue Jun 30, 2024
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Jul 17, 2024
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 15, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Oct 16, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 16, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 17, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 18, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 19, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 20, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 21, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Oct 26, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 27, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 28, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Oct 28, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 29, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Oct 29, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 30, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Oct 30, 2024
PR-URL: #53134
Refs: #52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Oct 31, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 1, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 2, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: nodejs#54502
Fixes: nodejs#34201
PR-URL: nodejs#54655
Refs: nodejs#52809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Steven R Loomis <srl295@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#54536
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 3, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 4, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 5, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 6, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 7, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 8, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 9, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 10, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Nov 11, 2024
PR-URL: nodejs/node#53134
Refs: nodejs/node#52809
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

7 participants