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 C++ exception handling #80612

Merged

Conversation

akien-mga
Copy link
Member

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

Upon investigating the extremely slow MSVC build times in #80513, I noticed that while Godot policy is to never use exceptions, we weren't enforcing it with compiler flags, and thus still included exception handling code and stack unwinding.

This is wasteful on multiple aspects:

  • Binary size: Around 20% binary size reduction with exceptions disabled for both MSVC and GCC binaries.
  • Compile time:
    • More than 50% build time reduction with MSVC.
    • 10% to 25% build time reduction with GCC + LTO.
  • Performance: Possibly, needs to be benchmarked.

Since users may want to re-enable exceptions in their own thirdparty code or the libraries they compile with Godot, this behavior can be toggled with the disable_exceptions SCons option, which defaults to true.

Alternative to #80516.
Fixes #80513.

Could be considered for a 4.1 cherry-pick, but this will require significant testing to make sure it behaves as expected, and it may be a welcome but surprising change for existing 4.1.x users, so we might opt for not including it.


Test builds

Edit: New test builds for all platforms made with the official buildsystem: https://downloads.tuxfamily.org/godotengine/testing/4.2-dev-pr80612/

To help validate this PR, I've done some builds of the before and after states (from this PR, so on top of 0308422), for Linux (GCC on our official buildsystem) and Windows (VS 2022 on my laptop, unsigned).

You'll find zips here:
https://downloads.tuxfamily.org/godotengine/testing/4.2.dev.no-exceptions/

Linux builds

-rwxr-xr-x. 1 akien akien 116M Aug 14 13:11 godot.linuxbsd.editor.x86_64.exceptions-with-oidn
-rwxr-xr-x. 1 akien akien 110M Aug 14 11:02 godot.linuxbsd.editor.x86_64.exceptions-without-oidn
-rwxr-xr-x. 1 akien akien  97M Aug 14 13:11 godot.linuxbsd.editor.x86_64.no-exceptions-with-oidn
-rwxr-xr-x. 1 akien akien  90M Aug 14 11:02 godot.linuxbsd.editor.x86_64.no-exceptions-without-oidn
-rwxr-xr-x. 1 akien akien  68M Aug 14 13:11 godot.linuxbsd.template_release.x86_64.exceptions
-rwxr-xr-x. 1 akien akien  57M Aug 14 13:11 godot.linuxbsd.template_release.x86_64.no-exceptions

exceptions are the same as we currently use for official builds, and no-exceptions use the new option to disable exception handling.

OIDN is special cased as I couldn't disable exception handling for it yet, so the with-oidn builds actually still have exceptions enabled for OIDN's build environment only. without-oidn builds were made with module_denoise_enabled=no. It's an editor-only dependency so it doesn't affect the template build.

I didn't include the without-oidn builds in the above ZIPs to save your bandwidth, I mostly build them to see the impact on size and build time.

These builds were made on the official buildsystem with the same options as official releases, so with full optimizations and LTO enabled (scons p=linuxbsd target=editor warnings=no production=yes)

Windows builds

-rwxr-xr-x 1 Remi 197121 118M Aug 14 11:17 godot.windows.editor.x86_64.exceptions.exe*
-rwxr-xr-x 1 Remi 197121 101M Aug 14 13:50 godot.windows.editor.x86_64.no-exceptions.exe*
-rwxr-xr-x 1 Remi 197121  54M Aug 14 12:17 godot.windows.template_release.x86_64.exceptions.exe*
-rwxr-xr-x 1 Remi 197121  51M Aug 14 11:52 godot.windows.template_release.x86_64.no-exceptions.exe*

Impact on size

As seen above, for Linux builds with GCC 10, we get a 16% size reduction for the editor build (or said otherwise, enabling exceptions added 20% size), and a similar ratio for the release template.

With LTO disabled, both build types are around 3M bigger, so the ratio doesn't change much.

Dev build: 758M to 736M on Linux, so 3% size reduction. Less than I expected.

On Windows, there's a similar 15% size reduction for the editor build, and only 5% size reduction for the release template (but it was already significantly smaller than the Linux one, now they have a comparable size).

Impact on compile time

Linux with GCC 10

Production builds with LTO enabled.

scons p=linuxbsd target=editor warnings=no production=yes

==> ../logs/godot-editor-exceptions-with-oidn.log <==
[Time elapsed: 00:09:19.461]

==> ../logs/godot-editor-exceptions-without-oidn.log <==
[Time elapsed: 00:09:09.282]

==> ../logs/godot-editor-no-exceptions-with-oidn.log <==
[Time elapsed: 00:08:51.862]

==> ../logs/godot-editor-no-exceptions-without-oidn.log <==
[Time elapsed: 00:07:55.473]

==> ../logs/godot-template_release-exceptions.log <==
[Time elapsed: 00:06:10.944]

==> ../logs/godot-template_release-no-exceptions.log <==
[Time elapsed: 00:04:34.993]

The differences aren't huge, because LTO still takes a while, and it seems like having to compile OIDN with exceptions enabled adds a whole minute to the build time.

If we compare the builds without OIDN, there's a 13% build time reduction for the editor, and a 26% build time reduction for the release template.

Builds without LTO enabled.

no-lto is scons p=linuxbsd target=editor warnings=no, while dev-build is scons p=linuxbsd target=editor dev_build=yes.

==> ../ext/logs/godot-editor-dev-build-exceptions.log <==
[Time elapsed: 00:05:47.942]

==> ../ext/logs/godot-editor-dev-build-exceptions.log <==
[Time elapsed: 00:06:00.398]

==> ../ext/logs/godot-editor-no-lto-exceptions.log <==
[Time elapsed: 00:10:02.207]

==> ../ext/logs/godot-editor-no-lto-no-exceptions.log <==
[Time elapsed: 00:07:35.507]

For an optimized build without LTO (so not paying a disproportionate amount of time for linking), there's a significant 25% build time speedup.

For the dev builds, I actually got a 2% build time increase, though I don't know if it's representative, I ran the test only once.

Sizes for these builds for those interested:

-rwxr-xr-x. 1 root root 758M Aug 14 11:45 godot.linuxbsd.editor.dev.x86_64.exceptions-dev-build
-rwxr-xr-x. 1 root root 736M Aug 14 12:01 godot.linuxbsd.editor.dev.x86_64.no-exceptions-dev-build
-rwxr-xr-x. 1 root root 119M Aug 14 11:34 godot.linuxbsd.editor.x86_64.exceptions-no-lto
-rwxr-xr-x. 1 root root 100M Aug 14 11:19 godot.linuxbsd.editor.x86_64.no-exceptions-no-lto

Windows with MSVC from VS 2022

scons p=windows target=editor scu_build=yes verbose=yes

- exceptions
[Time elapsed: 00:34:24.411]

- no-exceptions
[Time elapsed: 00:12:18.504]


scons p=windows target=template_release scu_build=yes verbose=yes

- exceptions
[Time elapsed: 00:19:28.350]

- no-exceptions
[Time elapsed: 00:09:20.439]

I used scu_build to speed things up, so this exacerbates issues like #80513 since the parallel build goes faster, but we'll still get stuck a long time on gdscript_vm.cpp for the exceptions build.

The results are pretty impressive though, with the build time divided by 3 for the editor (34m to 12m) and divided by 2 for the template (19m to 9m).

Impact on performance

Help welcome to run some benchmarks! You can use the builds linked above.

Impact on tests

I had to disable exceptions in doctest, which downgrades our REQUIRE/FAIL macros to the CHECK level (which marks the test as failed but doesn't rage quit). We need to see if that impacts our test suites negatively, and if we should clarify this behavior by changing our existing REQUIRE/FAIL uses to CHECK.

This is work for a follow-up PR where I'd welcome a volunteer - for this PR we should just ensure that the tests still work properly.

Further work

godot-cpp could likely reuse the same logic to build without extensions by default, with a command line option to override that behavior for users who want to compile with a library that needs exceptions (or their own code).

3.x backport: #80622

@akien-mga akien-mga requested review from a team as code owners August 14, 2023 11:03
@akien-mga akien-mga added topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release bug enhancement performance labels Aug 14, 2023
@akien-mga akien-mga added this to the 4.2 milestone Aug 14, 2023
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from d0893e1 to 7b12c55 Compare August 14, 2023 11:24
@akien-mga akien-mga requested a review from a team as a code owner August 14, 2023 11:24
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 7b12c55 to 3487c54 Compare August 14, 2023 11:49
@akien-mga akien-mga requested a review from a team as a code owner August 14, 2023 11:49
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 3487c54 to bff7893 Compare August 14, 2023 12:36
@akien-mga

This comment was marked as resolved.

@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from bff7893 to be36f1c Compare August 14, 2023 13:34
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from be36f1c to 6d269a8 Compare August 14, 2023 15:12
@akien-mga akien-mga requested review from a team as code owners August 14, 2023 15:12
@akien-mga
Copy link
Member Author

We were already defining -fno-exceptions for Android, iOS and Web, so I removed those since it's now done in SConstruct directly.

Also removed the ios_exceptions option since it's redundant with disable_exceptions (as long as -fexceptions is the default behavior with Apple Clang, to be confirmed @bruvzg).

Comment on lines -16 to -19
# Increase number of addressable sections in object files
# due to doctest's heavy use of templates and macros.
if env_tests.msvc:
env_tests.Append(CCFLAGS=["/bigobj"])
Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant with platform/windows/detect.py, hence the removal.

@akien-mga
Copy link
Member Author

akien-mga commented Aug 16, 2023

Some issues I'm noticing:

  • There are a lot of warnings being emitted for MSVC about missing unwind semantics. It seems like you forgot to carry over /wd4530 from #80516.

I see that too, but I'm surprised, as I thought _HAS_EXCEPTIONS=0 would disable the _TRY_BEGIN/_CATCH* stuff in the MSVC STL.

  • You seem to have forgot to actually re-enable exception-handling (by appending /EHsc) in the denoise module when using MSVC.

I was wondering whether it's needed. I'm not so familiar with MSVC, but it seems like exception handling is enabled by default (and can only be overridden with _HAS_EXCEPTIONS=0, and the /EH flags further specify the behavior of exception handling.

image

/EHc seems possibly useful, but without knowing much about it it seems to me that if we can avoid stack unwinding and it works fine for OIDN, it's maybe best? Advice welcome, I'm really out of my depth here :P

Edit: Well both issues seem to come from compiling OIDN, so it shows OIDN does want /EHsc.

Upon investigating the extremely slow MSVC build times in godotengine#80513, I noticed
that while Godot policy is to never use exceptions, we weren't enforcing it
with compiler flags, and thus still included exception handling code and
stack unwinding.

This is wasteful on multiple aspects:

- Binary size: Around 20% binary size reduction with exceptions disabled
  for both MSVC and GCC binaries.
- Compile time:
  * More than 50% build time reduction with MSVC.
  * 10% to 25% build time reduction with GCC + LTO.
- Performance: Possibly, needs to be benchmarked.

Since users may want to re-enable exceptions in their own thirdparty code
or the libraries they compile with Godot, this behavior can be toggled with
the `disable_exceptions` SCons option, which defaults to true.
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 6d269a8 to 3907e53 Compare August 16, 2023 08:23
@akien-mga
Copy link
Member Author

Re-added /EHsc when compiling OIDN, it should compile fine without warnings now.

akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Aug 16, 2023
akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Aug 16, 2023
akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Aug 16, 2023
@akien-mga
Copy link
Member Author

akien-mga commented Aug 16, 2023

In release builds you can also try ["-fno-unwind-tables", "-fno-asynchronous-unwind-tables"] .

Tested this:

Editor:

# production build with `-fno-exceptions`
[Time elapsed: 00:08:51.862]
-rwxr-xr-x. 1 akien akien  97M Aug 14 13:11 godot.linuxbsd.editor.x86_64.no-exceptions

# production build with `-fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables`
[Time elapsed: 00:10:25.825]
-rwxr-xr-x. 1 akien akien  91M Aug 16 14:09 godot.linuxbsd.editor.x86_64.no-exceptions-no-unwind-tables

Release template:

# production build with `-fno-exceptions`
[Time elapsed: 00:04:34.993]
-rwxr-xr-x. 1 akien akien  57M Aug 14 13:11 godot.linuxbsd.template_release.x86_64.no-exceptions

# production build with `-fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables`
[Time elapsed: 00:06:53.694]
-rwxr-xr-x. 1 akien akien  52M Aug 16 14:20 godot.linuxbsd.template_release.x86_64.no-exceptions-no-unwind-tables

So these options seem to have a significant impact to further decrease the binary size, at the cost of (somewhat surprisingly?) increasing build times.

Might be an interesting tradeoff to explore for official production builds, though I'll leave that for a separate issue/PR.

Edit: Doing some more tests, I'm seeing wild variations in build times on the build server where I did those tests, so the above time comparisons might not be accurate. So it may well be that -fno-unwind-tables does not increase build time in average.

@mihe
Copy link
Contributor

mihe commented Aug 16, 2023

I'm seeing mentions of these "unwind tables" potentially affecting stacktrace generation as well, so might be worth checking to see that any such things in Godot are still working as expected, if applicable.

@akien-mga
Copy link
Member Author

akien-mga commented Aug 16, 2023

I'm seeing mentions of these "unwind tables" potentially affecting stacktrace generation as well, so might be worth checking to see that any such things in Godot are still working as expected, if applicable.

Indeed, I would enable this only when using debug_symbols=no, since the stacktraces are useless anyway without debug symbols.

Currently we build everything with debug_symbols=no for official builds, but we should move towards actually building with debug symbols, extracting them, and providing them for download on-demand to make sense of stacktraces. When we do that, this may indeed remove the possibility to disable unwind tables fully. But we could consider doing that for target=template_release only.

Either way, we're not there yet and this is for future work.

@akien-mga akien-mga merged commit c3fd875 into godotengine:master Aug 16, 2023
15 checks passed
@akien-mga akien-mga deleted the scons-disable-exception-handling branch August 16, 2023 15:04
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 27, 2023
@akien-mga
Copy link
Member Author

Note to self, or Yuri: Worth cherrypicking IMO, but we might want to make disable_exceptions false by default to preserve the existing behavior. Otherwise this could be seen as breaking compat for users with C++ modules using exceptions. Official builds could turn on the option.

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2. Disabled by default, as suggested.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Oct 19, 2023
akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Oct 22, 2023
akien-mga added a commit to akien-mga/godot-cpp that referenced this pull request Oct 22, 2023
dsnopek pushed a commit to dsnopek/godot-cpp that referenced this pull request Oct 22, 2023
dsnopek pushed a commit to dsnopek/godot-cpp that referenced this pull request Oct 23, 2023
dsnopek pushed a commit to dsnopek/godot-cpp that referenced this pull request Oct 23, 2023
akien-mga added a commit to akien-mga/godot that referenced this pull request Nov 1, 2023
We made a mistake when cherry-picking godotengine#80612 with 269b115,
where the global flag was defaulted to false to preserve the 4.1-stable behavior for desktop
platforms, but we forgot that the refactoring removed the force disabling of exceptions for
Android, iOS, and Web.

This reintroduces this behavior so it should be back to the same as in 4.1/4.1.1, and the
export templates should get back to their original size.

Only difference, the old code used to keep exceptions for the Web editor, but I see no reason
for it, so I disable them like with the templates.
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
10 participants