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

SCons: Disable misbehaving MSVC incremental linking #80482

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Aug 10, 2023

Fixes #77968.

Testing welcome from MSVC users, to see both whether you could reproduce #77968 and confirm this fixes it, or if you couldn't reproduce it, confirm that it speeds up or doesn't slow down your linking time.

@akien-mga akien-mga added bug platform:windows topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 10, 2023
@akien-mga akien-mga added this to the 4.2 milestone Aug 10, 2023
@akien-mga akien-mga requested a review from a team as a code owner August 10, 2023 12:06
@akien-mga akien-mga added cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.0 labels Aug 10, 2023
@YuriSizov
Copy link
Contributor

I've repeated my experiment from here. I used the same commit and applied this change directly. It resulted in subsequent builds being twice as fast as before (45-50 seconds faster).

Fresh build (reset using git clean -dfxi)
[Time elapsed: 00:02:30.193]

1 new line 🧛
[Time elapsed: 00:00:45.407]

2 new lines 🧛🧛
[Time elapsed: 00:00:44.263]

3 new lines 🧛🧛🧛
[Time elapsed: 00:00:44.248]

4 new lines 🧛🧛🧛🧛
[Time elapsed: 00:00:44.745]

Empty build (no changes compared to above)
[Time elapsed: 00:00:06.146]

Out of curiosity I've removed the patch, and it's the same as before with noticeably longer linking times:

Fresh build (reset using git clean -dfxi)
[Time elapsed: 00:02:01.338]

1 new line 🧛
[Time elapsed: 00:01:33.458]

2 new lines 🧛🧛
[Time elapsed: 00:01:35.264]

3 new lines 🧛🧛🧛
[Time elapsed: 00:01:38.123]

4 new lines 🧛🧛🧛🧛
[Time elapsed: 00:01:34.067]

Empty build (no changes compared to above)
[Time elapsed: 00:00:06.188]

I'm not sure why the initial build is slightly slower now, but that's a fair trade-off if that's what we have for subsequent builds. (Note that in the linked issue my initial time is way too slow because I wasn't plugged in until mid-through the build.)

@akien-mga akien-mga merged commit 478b803 into godotengine:master Aug 11, 2023
15 checks passed
@akien-mga akien-mga deleted the scons-msvc-disable-incremental-linking branch August 11, 2023 08:35
@darksylinc
Copy link
Contributor

darksylinc commented Aug 20, 2023

This PR may have caused issues when compiling with:

scons p=windows vsproj=yes optimize=none debug_symbols=True tests=False dev_mode=True dev_build=True

I keep getting silly errors like this one:

console_wrapper_windows.windows.editor.dev.x86_64.obj : warning LNK4099: PDB 'vc140.pdb' was not found with 'console_wrapper_windows.windows.editor.dev.x86_64.obj' or at 'C:\Projects\Godot\godot\bin\vc140.pdb'; linking object as if no debug info
LINK : error LNK1218: warning treated as error; no output file generated
scons: *** [bin\godot.windows.editor.dev.x86_64.console.exe] Error 4099

A simple workaround is to edit the SConstruct file by removing /WX:

if env["werror"]:
            env.Append(CCFLAGS=["/WX"])
            # env.Append(LINKFLAGS=["/WX"])

@anvilfolk
Copy link
Contributor

anvilfolk commented Aug 21, 2023

I had this error exactly. I also cleaned everything I could, to no avail. As far as I could tell, there were no .obj files anywhere.

It's a few days later and at least one restart and I tried recompiling and just got this (without a newline between errors):

platform\windows\joypad_windows.cpp: fatal error C1041: cannot open program database 'C:\dev\godot\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FSplatform\windows\vulkan_context_win.cpp: fatal error C1041: cannot open program database 'C:\dev\godot\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS

Then recompiled again and it worked.

Now I --cleaned it once more, tried recompiling and I'm back to:

LINK : error LNK1218: warning treated as error; no output file generated
scons: *** [bin\godot.windows.editor.dev.x86_64.console.exe] Error 4099

Recompiling several times does seem to be succeeding now, after a series of the errors above.

So I can see multiple things potentially being a problem:

  • Something doesn't get cleaned up until a restart
  • It's a spurious issue caused by multi-threaded compilation creating an invalid vc140.pdb, as indicated by the two errors above not having a newline between them (two threads writing to console a the same time?). We should test using -j1.
  • Something else entirely that is only explained by dark magic

@lawnjelly
Copy link
Member

Sounds like it does indeed needs the /fs option (force synchronous PDB writes), as the error messages suggests.
https://learn.microsoft.com/en-us/cpp/build/reference/fs-force-synchronous-pdb-writes?view=msvc-170

It could be e.g. a timing error with multiple cl.exe trying to write to the same PDB at the same time, and the timing will have changed without incremental linking.

@bruvzg
Copy link
Member

bruvzg commented Aug 23, 2023

Sounds like it does indeed needs the /fs option (force synchronous PDB writes), as the error messages suggests.

We do already set it, it's useless. The error seems to be from the linker not form cl.exe.

@akien-mga
Copy link
Member Author

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 28, 2023
@akien-mga
Copy link
Member Author

Cherry-picked for 3.5.3.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 28, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subsequent builds are slower on Windows + MSVC due to linking
6 participants