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

Enable ccache on Windows #56705

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

StefanStojanovic
Copy link
Contributor

This PR continues an effort to enable Clang in the CI. While based on initial tests, it works, we want to make it as fast as possible. For that reason, this PR enables using ccache on Windows. The current cache we have in the CI, the clcache, is not compatible with Clang and is obsolete, so we could move fully to ccache in the CI in the future, but for Clang it is mandatory.

The initial work on this was done in #52126 and #52255 and that is the cornerstone of the changes suggested here. The main change is adding a new vcbuild option ccache which should be the path to the directory where ccache is installed (ccache.exe also needs to be renamed to cl.exe as explained in the docs).

When testing this, I noticed that one of the .h files, generated by V8 during the compilation was causing issues for ccache as its last modification time was later than cached PCH modification times. As a result, I made a simple patch to disable caching that file by using __TIME__.
NOTE TO REVIEWERS - I know this is far from optimal, but was the only way I found to enable ccache on at least a part of the project. I'm looking forward to any suggestions on how to improve it. Thanks in advance.

@StefanStojanovic StefanStojanovic added doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Jan 22, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jan 22, 2025
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2025
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Tips: follow <https://github.com/ccache/ccache/wiki/MS-Visual-Studio>, and you
should notice that obj file will be bigger the normal one.

First, install ccache, assume ccache install to c:\ccache, copy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, thinks means that if your ccache is installed at c:\path\to\ccache, then you can just do

cp c:\path\to\ccache\ccache.exe c:\path\to\ccache\cl.exe
.\vcbuild.bat ccache c:\ccache\

?

(We recommend installing Git and the UNIX tools added to PATH on Windows, so it seems fine to assume cp is available).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that makes sense. I'll add the command about the cp command to the doc.

@@ -41,6 +41,14 @@ void WriteBytecode(std::ofstream& out, Bytecode bytecode,
void WriteHeader(const char* header_filename) {
std::ofstream out(header_filename);

#ifdef CCACHE_USED
// Write a cache invalidator to ensure that ccache does not cache this file.
Copy link
Member

@joyeecheung joyeecheung Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why is it not a problem for non-Windows. Does it have to do with the fact that we only use PCH on Windows? What happens if you just mix and ccache_used != 1 into the condition here?

['OS=="win"', {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is connected to PCH usage. The idea of disabling PCH when wanting to use ccache did cross my mind, but from what I recall, the time for compilation without PCH was much greater than with it. Not sure about the numbers. Let me reiterate it and then I'll share the exact numbers from my machine here together with the PCH error I get from cache. Thanks for the suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that PCH would help a deal when there's no good caching but wouldn't matter as much when there is caching? Especially when dealing with V8 which uses templates very extensively, and headers change quite often...

Prior to these changes compilation with ccache was broken because of
the .h file regeneration in each compilation.
@StefanStojanovic
Copy link
Contributor Author

To follow up on the discussion started here.

The first thing I noticed is that without PCH, MSVC compilation fails with the following error:

..\..\out\Release\lib\v8_base_without_compiler.lib : fatal error LNK1248: image size (10075D049) exceeds maximum allowable size (FFFFFFFF) [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

Based on that, PCH are mandatory for MSVC. However, MSVC works fine without changes in generate-bytecodes-builtins-list.cc and ccache improvements are similar regardless of them.

Now to look at Clang. It compiles with and without PCH, but if PCH are used running with ccache it will fail with the following error:

fatal error: file 'E:\work\node\tools\v8_gypfiles\..\..\out\Release\obj\global_intermediate\generate-bytecode-output-root\builtins-generated\bytecodes-builtins-list.h' has been modified since the precompiled header '..\..\out\Release\obj\v8_base_without_compiler\v8_bas e_without_compiler.pch' was built: mtime changed (was 1737707884, now 1737708834)
note: please rebuild precompiled header '..\..\out\Release\obj\v8_base_without_compiler\v8_base_without_compiler.pch'
C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.Cpp.ClangCl.Common.targets(235,5): error MSB6006: "clang-cl.exe" exited with code 1. [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

Based on that using PCH with Clang and cache would need the modified version version of generate-bytecodes-builtins-list.cc.

The compilation times I got on my machine show that using PCH with modification enabling Clang to work, cuts compilation time to ~80%, so it gets faster but not very noticeable. Disabling PCH on Clang increases first compilation time ~2 times, but future compilations are decreased to ~20% of the original one. To make this easier to understand, here are rounded compilation times I got on my machine:

  • MSVC (with PCH) 1st compilation: 16-17 minutes
  • MSVC (with PCH) other compilations: 13-14 minutes
  • Clang (with PCH) 1st compilation: 15 minutes
  • Clang (with PCH) other compilations: 12 minutes
  • Clang (no PCH) 1st compilation: 30 minutes
  • Clang (no PCH) other compilations: 3 minutes

So these are the facts I gather and based on them the decision should be made. Another thing to keep in mind is that in Windows CI we already have clcache used for MSVC so potentially we should consider moving to ccache for everything on Windows. I guess the outcome depends on how much space we can allocate, but I'd like to hear for other thin based on these numbers. For some time, we'll have MSVC and Clang overlapping, so each build will need 6 compilations (4 release, 2 debug), and that is the exact number of machines we have for compilation, but since they are also used for tests, I expect this to be a bottleneck unless handled properly.

cc @nodejs/build @nodejs/platform-windows @joyeecheung

@joyeecheung
Copy link
Member

Thanks for the detailed investigation! I think when using clang with ccache, having a ~15 minutes slower first compilation but ~9 minutes faster for each incremental compilation and no modifications to the headers (which we need to float and address conflicts for during V8 upgrade) sounds like a good trade-off?

For some time, we'll have MSVC and Clang overlapping, so each build will need 6 compilations (4 release, 2 debug)

What are the extra 2 releases beside MSVC/Clang with ccache? I also wonder if it's fine to move the debug build to clang instead and don't even keep the msvc debug build, because having MSVC + debug may not necessarily give us a lot of extra coverage, especially considering V8 has already abandonded MSVC and the debug checks are highly dependent on V8 internals, often producing false positives from e.g. compiler support of some fancy C++ features.

@StefanStojanovic
Copy link
Contributor Author

Thanks for the detailed investigation! I think when using clang with ccache, having a ~15 minutes slower first compilation but ~9 minutes faster for each incremental compilation and no modifications to the headers (which we need to float and address conflicts for during V8 upgrade) sounds like a good trade-off?

In CI these times are noticeably longer, but percentages should stay the same. It's a good point. Anyway, we can always try and see how it goes and adapt if needed.

What are the extra 2 releases beside MSVC/Clang with ccache? I also wonder if it's fine to move the debug build to clang instead and don't even keep the msvc debug build, because having MSVC + debug may not necessarily give us a lot of extra coverage, especially considering V8 has already abandonded MSVC and the debug checks are highly dependent on V8 internals, often producing false positives from e.g. compiler support of some fancy C++ features.

We have releases for MSVC and Clang for both x64 and ARM64 (we cross-compile on x64 machines). About debug builds I agree. x64 Clang release build is already added and it works fine. I'll go ahead and swap the debug build from MSVC to Clang and if there are no issues it can stay that way.

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. doc Issues and PRs related to the documentations. install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants