-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
static inline is bad. Generates too much code that the linker can't remove. #1621
base: master
Are you sure you want to change the base?
Conversation
…unable to optimize and remove on final link. This causes these symbols from every class in godot-cpp to be included in the final link, even if completely unused by the lib. Removing changes a basic shared library from being ~1.5MB on almost all platforms to now ~200kB.
fafe088
to
ddd03ee
Compare
Thanks! @Faless added a "build profile" feature relatively recently that can help reduce binary size (and build times) - see PR #1167
Unfortunately, we do need these registrations for any type that may be returned by a Godot API, in order to solve an important bug with godot-cpp creating the incorrect wrapper class - see PR #1050 The "build profiles" help but they work at the class level. There will be methods you don't actually call which return types that end up getting registered. It'd be really cool to somehow only generate the methods that are actually called? Anyway, we can't just remove the engine class registration if it means bringing back that bug. If anyone has any better ideas for how to solve this problem, they would be welcome :-) |
inline static ::godot::internal::EngineClassRegistration<m_class> _gde_engine_class_registration_helper; \ | ||
static ::godot::internal::EngineClassRegistration<m_class> _gde_engine_class_registration_helper; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline static
here makes is so that we can declare a static variable in a header without needing to setup its storage in a compilation unit. I think removing the inline
here means here means this variable won't actually be initialized (and hence the registration won't happen) because we don't ever put it in a .cpp file. We could certainly generate an addition to the corresponding .cpp file, but then the registration will always happen for all classes, and not just those that were #include
d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is incorrect according to the disassembly and actually running the code... This is the offending line that causes bloat because the linker isn't able to optimize all these templated unused symbols out. The variable is initialized during static initialization when the library is loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
class variables (without inline
) need to have their storage declared in a compilation unit (.cpp
file) somewhere, otherwise they will never be initialized.
This PR is removing the inline
but not adding the storage bit to any .cpp
file.
inline static RecreateInstance *recreate_instance = nullptr; | ||
static RecreateInstance *recreate_instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching this one away from inline static
is fine, but I have another PR (#1590 - still awaiting review) that removes it as part of fixing a different bug
This PR doesn't remove any of the registrations... And infact |
I understand the build_profiles, but this issue also relates to a lot of code in godot as well. I've been looking at optimizing the web builds for godot main for the past week and a lot of the bloat is coming from |
Keeping these lingering symbols around for unused classes don't even make sense, because everything else from the unused classes are optimized out. Take the |
I'm pretty confident this is removing the registrations. The CI tests are failing on this PR that I would expect to fail if the registrations aren't happening. |
Please take a look at the test failures. They indicate that the bug that the engine class registrations were added to fix has returned. If you can come up with a way to reduce the binary size while keeping these tests passing, I would be very happy to merge that solution. :-) Here are the test failures:
|
Can you elaborate on But think about it logically... I'm trying to 'extend' the engine with my dynamic library, gdextension right? It's a native library and what's nice about that is it knows what symbols need to be resolved when it links with |
PR #1050 which I linked to in my first comment is a good place to start! It links to these two comments which explain the original issue which led to the addition of the engine class registrations in much detail:
I feel like I've already said this several different ways above, but I'll attempt to explain again:
|
I've reviewed a lot of the history in this repo related this particular issue, registering these 'engine' classes statically, and the resulting binary size ramifications. Specifically these comments here #1160 and here #1266
I'll attempt to explain but an externally visible inline function must have the same definition across all translation units; otherwise, an ODR (One Definition Rule) violation occurs, similar to regular functions. c++ also ensures that any local static variable within such a function maintains the same address across all instances. The compiler might choose to inline the function or not, depending on the translation unit. If it doesn't inline, the function's definition is generated for that TU. As a result, the function could end up being included in every TU, but during linking, the linker will select one instance and discard the others. Since all versions are required to be identical, this approach works without issue. I suppose this also depends on the linker supporting a visibility model where multiple TUs can define the symbol as externally visible without causing a 'duplicate symbol' error. But for 'static inline,' the symbol is no longer externally visible. So the compiler inlines the function for each TU. If it doesn't, the compiler generates a separate local definition of the function for that TU. At link time, the linker can't merge all these copies since they're local to each TU, so it merges them via identical code folding. I tested a couple different object files, and the above is indeed the case... Multiple definitions if TUs include the same header, example both What could eventually be a red herring is that this means that any static locals within the inlined static function are now unique to each TU, rather than being shared globally and during the linking process, the linker won't merge these copies since they are local to their respective TUs. So perhaps you have to be careful there. But most importantly, #1266 just isn't the case with a wasm module unfortunately, as seen with what I've posted here. Building a gdextension with a single class and This is leading to a wasm module that is about 10X larger than it should be, and is a limitation for realistically using gdextension for us on the web, and also on web portals that have size limitations like poki for example. I have been looking at a lot of the other language bindings and I'm not sure if there is awareness of this issue either (the binary bloat in godot-cpp as a result of header inclusion), but I've just started looking at a few other language bindings and the resulting wasm modules are quite large for simple extensions as well.
Yes I've seen now why you have added in these 'engine class' registrations, to statically inline their defns into a vector, then initialize all of them when the extension library is loaded via I'll post my results from bloaty for mach-o format in a followup message here, but this is an ongoing effort to try and reduce the binary footprint in godot main as well. |
Looking at the dissasembly of a gdextension library, there are way too many unused symbols and code included. It's bloaty. 1.5MB library for a single gdextension class.
Everyclass seems to include things like this:
Engine class registrations, etc. Even when no class in my extension used the base engine classes. The culprit here is
inline statics
which cause a lot of bloat, and are all ultimately unused. This seems also to be the case in godot main as well.These cause symbols from every class in godot-cpp to be included in the final link, even if completely unused by the impl lib. Reworking changes a basic gdextension shared library from being ~1.5MB to now ~200kB.