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.

Sign up for GitHub

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

Jump to bottom

[3.x] SCons: Disable C++ exception handling #80622

Merged
merged 1 commit into from
Aug 16, 2023
Merged

[3.x] SCons: Disable C++ exception handling #80622

merged 1 commit into from
Aug 16, 2023
+31 −24

Conversation

akien-mga
Copy link
Member

Backport of #80612, see that PR for discussion.

@akien-mga akien-mga added this to the 3.6 milestone Aug 14, 2023
@akien-mga akien-mga requested review from a team as code owners August 14, 2023 15:22
@akien-mga akien-mga mentioned this pull request Aug 14, 2023
Merged
@akien-mga akien-mga force-pushed the 3.x-scons-disable-exception-handling branch from ccc30da to 2eceb23 Compare August 14, 2023 15:30
@akien-mga akien-mga requested a review from a team as a code owner August 14, 2023 15:30
akien-mga
akien-mga commented Aug 14, 2023
View reviewed changes
modules/fbx/fbx_parser/FBXMaterial.cpp
@@ -328,65 +328,58 @@ Video::Video(uint64_t id, const ElementPtr element, const Document &doc, const s
}

if (Content && !Content->Tokens().empty()) {
//this field is omitted when the embedded texture is already loaded, let's ignore if it's not found
Copy link
Member Author

@akien-mga akien-mga Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@RevoluPowered We shouldn't use exceptions in Godot code, how can I rewrite this to handle the failure case gracefully? (Current diff just removes try { and } catch(...) { ... }

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 seems to be straight from Assimp where it's still using this hack:
https://github.com/assimp/assimp/blob/556c89b5f143738720e17572faa8e272d4eec380/code/AssetLib/FBX/FBXMaterial.cpp#L304-L364

I don't see it intentionally throwing and catching exceptions, so I guess this code will just crash and instead of adding proper failure handling they just wrapped it in exceptions... sigh

So what can crash here? token == nullptr at least I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just changed it to replace the try { } catch (...) { } by if (token), assuming that if it's failing it may return a nullptr. Add added another ERR_FAIL_COND, there was already one, it probably leaves things in a broken state if the condition is met but I'm not sure we even use the FBX Video class at all, so I don't want to do a deep dive.

@akien-mga akien-mga force-pushed the 3.x-scons-disable-exception-handling branch from 2eceb23 to 032b76e Compare August 14, 2023 19:34
@akien-mga akien-mga requested a review from a team as a code owner August 14, 2023 19:34
@akien-mga akien-mga force-pushed the 3.x-scons-disable-exception-handling branch from 032b76e to 9344a36 Compare August 14, 2023 20:24
@akien-mga akien-mga requested a review from a team as a code owner August 14, 2023 20:24
lawnjelly
lawnjelly approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks fine barring the query about the FBXMaterial.

@akien-mga akien-mga force-pushed the 3.x-scons-disable-exception-handling branch from 9344a36 to 9dac444 Compare August 15, 2023 07:39
@Calinou
Copy link
Member

Calinou commented Aug 15, 2023 •
edited
Loading

Linux

OS: Fedora 38
CPU: Intel Core i9-13900K
SSD: Solidigm P44 Pro 2 TB
Compiler: GCC 13.2.1
Linker: mold 1.8.0

Build options

Editor build options: scons -j32 platform=linuxbsd linker=mold optimize=none fast_unsafe=yes progress=no debug_symbols=yes builtin_embree=no builtin_enet=no builtin_freetype=no builtin_graphite=no builtin_harfbuzz=no builtin_libogg=no builtin_libpng=no builtin_libtheora=no builtin_libvorbis=no builtin_libwebp=no builtin_mbedtls=no builtin_miniupnpc=no builtin_pcre2=no builtin_zlib=no builtin_zstd=no scu_build=all tests=no

Release export template build options: scons -j32 platform=linuxbsd tools=no target=release debug_symbols=yes lto=full progress=no scu_build=all

I ran each build twice with git clean -dfX && ccache -C before each build to fully empty caches.

Editor

# Before
vstask "Build (editor)"  524.04s user 47.64s system 1287% cpu 44.397 total
Stripped binary size: 98,324,736 bytes

# After
vstask "Build (editor)"  504.25s user 47.62s system 1374% cpu 40.163 total
Stripped binary size: 89,852,160 bytes

Release export template

# Before
vstask "Build (release export template)"  1240.66s user 66.26s system 2042% cpu 1:03.98 total
Stripped binary size: 40,015,136 bytes

# After
[Time elapsed: 00:00:57.894]
vstask "Build (release export template)"  1085.06s user 62.20s system 1976% cpu 58.045 total
Stripped binary size: 33,625,376 bytes

Windows

OS: Windows 11 22H2
CPU: Intel Core i9-13900K
SSD: Solidigm P44 Pro 2 TB
Compiler/Linker: MSVC 2022

Build options

Editor build options: scons platform=windows progress=no fast_unsafe=yes debug_symbols=no -j32

Release export template build options: scons platform=windows progress=no tools=no target=release debug_symbols=no -j32

# Before
[Time elapsed: 00:00:52.174]
Binary size: 153,532,416 bytes


# After
[Time elapsed: 00:00:52.128]
Binary size: 153,521,152 bytes

Release export template

# Before
[Time elapsed: 00:01:12.613]
Binary size: 33,624,576 bytes

# After
[Time elapsed: 00:01:04.758]
Binary size: 31,526,400 bytes

@akien-mga
Copy link
Member Author

I'm surprised that you're seeing virtually no difference on Windows, I'll have to test.

@akien-mga
SCons: Disable C++ exception handling
55550da 
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 3.x-scons-disable-exception-handling branch from 9dac444 to 55550da Compare August 16, 2023 08:34
@akien-mga
Copy link
Member Author

Tested on Windows with the same command you used, and indeed I can reproduce the results for the editor target=debug build. I guess the difference is only significant for optimized builds.

scons platform=windows progress=no fast_unsafe=yes debug_symbols=no

(implicit: tools=yes target=debug)

# Exceptions enabled
[Time elapsed: 00:03:38.767]
Binary size: 147,213,312 bytes

# Exceptions disabled
[Time elapsed: 00:03:39.114]
Binary size: 147,201,536 bytes

No real difference on a debug build. Now an optimized one:

scons platform=windows tools=yes target=release_debug production=yes

# Exceptions enabled
[Time elapsed: 00:08:15.230]
Binary size: 71,438,848 bytes

# Exceptions disabled
[Time elapsed: 00:07:41.283]
Binary size: 67,204,096 bytes

So 6% size reduction, 5% build time reduction.

@akien-mga akien-mga merged commit d61eba5 into godotengine:3.x Aug 16, 2023
13 checks passed
@akien-mga akien-mga deleted the 3.x-scons-disable-exception-handling branch August 16, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lawnjelly lawnjelly lawnjelly approved these changes

Assignees
No one assigned
Labels
bug enhancement needs testing performance topic:buildsystem
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
3 participants
@akien-mga @Calinou @lawnjelly