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 /EHsc for MSVC, speeds up build significantly #80516

Closed

Conversation

akien-mga
Copy link
Member

As seen in #80513, /EHsc seems responsible for extremely long build times for complex files such as gdscript_vm.cpp and variant_call.cpp, when combined with optimization flags.

On my Windows setup, a clean rebuild of the engine with scons p=windows (thus defaulting to optimize=speed-trace, i.e. /O2 on MSVC) went from 35 min down to 16 min with this patch. Incremental rebuilds that would lead to recompiling either gdscript_vm.cpp or variant_call.cpp will benefit even more.

We were using /EHsc to solve warnings in thirdparty code, so instead we silence that warning specifically.

Fixes #80513.


I could use help to properly assess the impact of removing /EHsc and ignoring the C4530 warning. I'm not super familiar with exception handling and stack unwinding. Relevant docs: https://learn.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model?view=msvc-170

Patching thirdparty libraries so they don't raise this warning is also an option. In theory, we don't want any exception handling in Godot.

@akien-mga akien-mga added bug platform:windows topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release performance cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 11, 2023
@akien-mga akien-mga added this to the 4.2 milestone Aug 11, 2023
@akien-mga akien-mga requested a review from a team as a code owner August 11, 2023 14:28
@akien-mga
Copy link
Member Author

Interesting error on tests after this change:

Error: .\tests/core/io/test_file_access.h(42): error C2338: static_assert failed: 'Exceptions are disabled! Use DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS if you want to compile with exceptions disabled.'

@akien-mga akien-mga requested a review from a team August 11, 2023 15:49
@akien-mga akien-mga force-pushed the scons-msvc-disable-EHsc branch from 7d1dedd to 6bf6211 Compare August 12, 2023 14:08
@akien-mga akien-mga requested a review from a team as a code owner August 12, 2023 14:08
@akien-mga
Copy link
Member Author

Interesting error on tests after this change:

Error: .\tests/core/io/test_file_access.h(42): error C2338: static_assert failed: 'Exceptions are disabled! Use DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS if you want to compile with exceptions disabled.'

So Doctest needs exceptions for the REQUIRE (and I think FAIL) macros, i.e. macros which mark the test as failed and abort. It has an option to disable the use of exceptions but that basically downgrades those macros to CHECK level, which mark the test as failed but don't abort.

We could look into whether we can get by with only CHECK level macros, but if so we can't test unrecoverable conditions. I haven't assessed what our current tests need.

So for now I just re-define /EHsc for the tests/SCsub environment.

As seen in godotengine#80513, `/EHsc` seems responsible for extremely long build times
for complex files such as `gdscript_vm.cpp` and `variant_call.cpp`, when
combined with optimization flags.

On my Windows setup, a clean rebuild of the engine with `scons p=windows`
(thus defaulting to `optimize=speed-trace`, i.e. `/O2` on MSVC) went from
35 min down to 16 min with this patch. Incremental rebuilds that would lead
to recompiling either `gdscript_vm.cpp` or `variant_call.cpp` will benefit
even more.

We were using `/EHsc` to solve warnings in thirdparty code, so instead we
silence that warning specifically.

For tests, doctest needs exceptions for the macros we currently use, so we
keep `/EHsc` in this environment only.

Fixes godotengine#80513.
@akien-mga akien-mga force-pushed the scons-msvc-disable-EHsc branch from 6bf6211 to 15e8cce Compare August 12, 2023 14:13
@ryuukk
Copy link

ryuukk commented Aug 13, 2023

Just randomly passing by

Looked around and seems you can disable EH for doctest:

2 config possible:

DOCTEST_CONFIG_NO_EXCEPTIONS
DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS

EH is evil

@akien-mga
Copy link
Member Author

akien-mga commented Aug 13, 2023

Looked around and seems you can disable EH for doctest:

2 config possible:

DOCTEST_CONFIG_NO_EXCEPTIONS DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS

Indeed, but as I pointed out above, I believe this will break the tests by downgrading REQUIRE/FAIL asserts to the non terminating CHECKED level, so some tests might crash if they rely on aborting when a non recoverable error happens.

Could still be worth doing but this will likely require changing a lot of tests.

@mihe
Copy link
Contributor

mihe commented Aug 13, 2023

There's also the quasi-supported (and undocumented) _HAS_EXCEPTIONS=0 definition to consider, which I typically see defined in conjunction with omitting /EH[sc], and supposedly allows the MSVC STL implementation to make different choices in some cases. I'll admit I don't fully understand the pros/cons to defining this versus not defining it, but I would imagine it's safe to define it as long as you don't plan to actually use any exception classes or exception-handling anywhere. Considering how little the STL is used in Godot anyway, I would imagine it's even less of a risk. Unreal (who also uses the STL very sparingly) seems to get away with defining this, just to give one example.

Has there been any consideration towards disabling exception-handling for GCC/Clang as well, through -fno-exception? Disabling exception-handling seems fairly ubiquitous for gamedev stuff and can have significant impacts on binary size, and presumably instruction cache performance, since it can omit all the generated code for the stack-unwinding, which is just dead weight if you never use exceptions anyway. This PR nets a ~15% decrease in binary size for target=editor. I would be surprised if GCC/Clang didn't net at least a couple of percentage points as well.

@akien-mga
Copy link
Member Author

I made #80612 as an alternative to fully disable exception handling. Reviews and tests welcome :)

@akien-mga
Copy link
Member Author

Superseded by #80612.

@akien-mga akien-mga closed this Aug 15, 2023
@akien-mga akien-mga deleted the scons-msvc-disable-EHsc branch August 15, 2023 07:14
@akien-mga akien-mga added archived and removed 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 15, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Aug 15, 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.

Extremely slow build times for gdscript_vm.cpp with MSVC and optimizations enabled due to /EHsc
4 participants