Builds are ~1-5mb larger than they used to be

[dsnopek]

Godot version

4.1-rc2

godot-cpp version

master (80986f8)

System information

Linux

Issue description

@Faless was the one to discover this with the webrtc-native GDExtension. However, you can see it even with the test project: with gcc on Linux, the debug template build is ~5mb larger than when using godot-cpp on 4.0.3-stable tag.

It's PR #1050 that led to this problem.

The reason is that before PR #1050, if you didn't use a class in your GDExtension, it wouldn't be included in the final binary - the linker just leaves it out automatically. However, an unintended consequence of that PR is that now all classes are always included in the final binary.

That PR aimed to solve a bug where the godot-cpp wrapper class that was created for a Godot object was (before the PR) the class of the argument or return value. So, if you called get_node("MyNode") it would always wrap the object in the Node wrapper class, even if the node was actually a Sprite2D. And because Godot caches the wrapper class, it would forever be stuck as a Node wrapper. If you had another method that passed in the same object as a Sprite2D, it would unsafely cast the wrapper to Sprite2D even if it wasn't actually one. Despite being unsafe, this actually works in a surprising number of situations! However, there are case where it causes problems, like with virtual methods. (And it's also plain incorrect C++ :-))

Anyway, that PR solves the problem by making a big HashMap of the binding callbacks for all the Godot engine classes, so when an object is sent over to godot-cpp, it can use the correct wrapper class (ie. the one matching its class on the Godot side). It's this HashMap that's causing all classes to be "used".

I've thought up 4-ish possible solutions (although, there's probably more - please share your ideas!):

1. We make a scons tool that scans the user's GDExtension code for includes, and only puts the classes in the HashMap that were discovered in the scan (and all their parent classes). I don't think this would be the hackiest thing in the world? Header file includes are pretty easy to scan for. And it means that GDExtensions will work as expected out-of-the-box, but you can optionally take this extra step (which we could include in an official godot-cpp template) that optimizes the size of the extension. However, I know that not all people want to use scons, and they'd need to do their own optimization step.
2. We stop caching the wrappers on the Godot side, and just always make a new one when requested. And then Object::cast<T>() would also create a new wrapper when doing a valid downcast. However, this would likely be slower, since we'd be creating and throwing away way more wrappers. And, actually, I'm not sure how those extra wrappers would get cleaned up?
3. We allow Object::cast_to<T>() to "upgrade" the wrapper to one that's further down the inheritance tree, and overwrite the one in the cache, if it's a valid downcast. This would also be somewhat slower (and maybe surprising, because you'll get a different object than you put in) and also leaves the open question of who cleans up the old wrappers.
4. We just accept that GDExtensions with godot-cpp need to be ~1-5mb bigger than they were before :-)

What do you think?

Steps to reproduce

1. Compile the test/demo project with current godot-cpp master and check the resulting binary size
2. Compile the test/demo project with godot-cpp 4.0.3-stable and check the resulting binary size.

Minimal reproduction project

n/a

[mihe]

I made an attempt at trying to reduce the size of register_engine_classes.cpp by having it register the engine classes using a bunch of extern GDExtensionInstanceBindingCallbacks, removing the need to include the classes altogether. This (perhaps unsurprisingly) did make the object file for register_engine_classes.cpp much smaller, but it seemed to have just spread this generated code out across the other object files instead, which squashes my hope that much of this generated code was redundant.

Funnily enough MSVC saw a 72% increase in binary size instead of the ~2% decrease I saw with Clang, which I can only assume has something to do with its link-time optimizer struggling to deduplicate the generated code that's now spread out over hundreds of object files instead.

With that said, I'm curious about that third solution of yours. When you say "upgrade the wrapper", would that entail actually switching out the instance bindings on the engine side of things, or is that something that would be dealt with on the godot-cpp side of things?

If it's the former, then the engine does technically have support for holding multiple instance bindings for one instance, since it needs to support multiple extension libraries anyway, so I guess you might be able to piggyback on that to also hold multiple instance bindings for a single library, and then also (somehow) sorting the ones further down the inheritance tree first, to ensure that they get found first in Object::get_instance_binding. They would be sharing the same token though, which might be a problem.

[dsnopek]

With that said, I'm curious about that third solution of yours. When you say "upgrade the wrapper", would that entail actually switching out the instance bindings on the engine side of things, or is that something that would be dealt with on the godot-cpp side of things?

It would probably require some code on both sides. Basically, it'd change Object::cast_to<T>() in godot-cpp to:

1. Check if T is a valid downcast; otherwise return nullptr
2. Check if the instance binding (ie. the wrapper) cached on the Godot side was already a T or lower class in the hierarchy - if so, cast the cached wrapper to T and return it
3. Create a new T (ie. the wrapper class) to wrap the same object from the engine, and then tell the engine to replace the cached instance binding with the one that was just created, and ultimately return it

If it's the former, then the engine does technically have support for holding multiple instance bindings for one instance, since it needs to support multiple extension libraries anyway, so I guess you might be able to piggyback on that to also hold multiple instance bindings for a single library

I think we'd need to change how this works on the Godot side anyway.

We'd want the wrapper that's the furthest down the class hierarchy to be the cached one (because it can always be upcasted to any of its ancestor classes). But we'd still need to keep track of all the wrappers that were created so we could clean them up once the object on the Godot side was cleaned up. Otherwise, we'd be leaking memory every time a new "upgraded" wrapper was created.

Anyway, I don't know that this approach is even a good idea. :-) But it was one of the only solutions I could think of! Hopefully, ether options nr 1 or 4 will be acceptable, OR someone else will come up with a solution that's better than any of the ones I thought of

[mihe]

Thank you for the breakdown.

I guess it's a notable regression, but the size itself isn't all that outrageous, so I would personally be content with option 4. I am a little bit biased though, since I've dug myself a nice big hole by completely scrapping the default SCons/CMake setup in godot-cpp in favor of a custom CMake one, so option 1 would stir things up a bit, since I only ever build godot-cpp when my Git reference to it changes.

[DmitriySalnikov]

37MB -> 86MB debug_draw_3d

And now I can't use my patch to ignore unnecessary classes during build. But the main problem for now is that all methods that return Object require getting the binding before the cast. So when I check the editor scene tree with get_child(i)->get_class() I get hundreds of errors about Cannot find instance binding callbacks for class because my library doesn't have all node types. And after all the errors, a crash occurs, due to nullptr.

I wonder what will happen if the tree of nodes is processed in the same way, in which nodes from other libraries or custom modules will be used or how it will work after updating the engine without updating the library?..

Because of this, GitHub CI spends 20+ minutes on a complete parallel rebuild, whereas it used to be ~5-7 minutes.

My patch is here: get_used_classes.py and godot_cpp_exclude_unused_classes.patch. It checks the include's and makes a list of only those classes that occur in my code (src/ or override with folder_to_include_classes="src/"). Now it is possible to use plugins in GDExtension and I was planning to try to add support for #if/#ifdef, but at the moment it would be useless..

[dsnopek]

@DmitriySalnikov:

37MB -> 86MB debug_draw_3d

Whoa, that's a huge increase! I wonder why you're seeing much bigger numbers than other folks?

But the main problem for now is that all methods that return Object require getting the binding before the cast. So when I check the editor scene tree with get_child(i)->get_class() I get hundreds of errors about Cannot find instance binding callbacks for class because my library doesn't have all node types. And after all the errors, a crash occurs, due to nullptr.

This sounds like a bug in the code from PR #1050, but since it's not related to the size issue here, it should probably be tracked in a different issue. Can you create one?

And, if you make a new issue, can you share which classes specifically it's having problems with and where they come from?

I wonder what will happen if the tree of nodes is processed in the same way, in which nodes from other libraries or custom modules will be used or how it will work after updating the engine without updating the library?..

There is already code to avoid trying to find bindings for classes from other GDExtensions, and for core classes that aren't exposed. For custom modules you've added to Godot, you should probably update the extension_api.json used to compile godot-cpp. So, anyway, that's why I'm curious about which classes specifically are causing from for you - I need to figure out what the case we missed was :-)

I hadn't really considered the case of updating the engine without updating the GDExtension. This should probably be handled the same as core classes that aren't exposed, but right now, I think it'll lead an error on the console and the Object wrapper being used.

Anyway, it's another issue we need to address!

[mihe]

37MB -> 86MB debug_draw_3d

I'm assuming those numbers are non-stripped Linux/macOS binaries? Your distributed template_debug binaries for 4.0 look to be 2.5 MB and 3.9 MB on Linux and macOS respectively. So that's all just debug symbols, presumably.

I don't have access to Linux/macOS right now, but when compiling your latest changes, on Windows with MSVC, your template_debug binaries seem to only have gone up from 607 KB to 1.15 MB.

Because of this, GitHub CI spends 20+ minutes on a complete parallel rebuild, whereas it used to be ~5-7 minutes.

It would be nice to see godot-cpp adopt something similar to the SCU builds that Godot itself recently introduced. I've been using CMake's "unity builds" to compile godot-cpp in Godot Jolt, and it cuts down on the godot-cpp compile times by some crazy number, like 90%, although don't quote me on that exact number.

[DmitriySalnikov]

I wonder why you're seeing much bigger numbers than other folks?

Android takes up the most space, although in fact on android this library should be a dummy at most 😅

current (+15MB because the editor target has been added to the PC): https://github.com/DmitriySalnikov/godot_debug_draw_3d/actions/runs/5478565765
prev: https://github.com/DmitriySalnikov/godot_debug_draw_3d/actions/runs/5101416193

4.0

android   24.5 MB
linux     4.89 MB
macos     7.44 MB
windows   1.11 MB
total     37.9 MB

4.1

android   56.0 MB
linux     17.1 MB
macos     25.0 MB
windows   3.4  MB
total     101  MB

increase ~x2, without taking into account the addition of libraries for the editor.

I hadn't really considered the case of updating the engine without updating the GDExtension.

But not all libraries are developed by yourself and some may not be updated for a long time. Then why do we need backward compatibility?
In this case, all libraries need to be distributed in the form of source code and, as in Unreal Engine, rebuild them every time the engine or library is updated.

And, if you make a new issue, can you share which classes specifically it's having problems with and where they come from?

This is list of nodes that were not found if my patch was applied to the master branch:

CenterContainer
CheckBox
CheckButton
EditorFileDialog
FontVariation
GridContainer
HBoxContainer
HFlowContainer
HSeparator
HSlider
HSplitContainer
ItemList
LineEdit
MarginContainer
MenuBar
MenuButton
OptionButton
Panel
PopupPanel
ProgressBar
RichTextLabel
SpinBox
SubViewportContainer
TabBar
TabContainer
Timer
Tree
VSeparator
VSplitContainer

Just do:

cd godot-cpp
git apply --ignore-space-change --ignore-whitespace ../patches/godot_cpp_exclude_unused_classes.patch

And build for editor. The library will be updated in addons/debug_draw_3d/libs/. Copy/symlink addon to an empty project and open it in the editor.

---

I'm assuming those numbers are non-stripped Linux/macOS binaries?

Linux and Mac stripped. Android is not stripped, because when publishing to Google Play, it is desirable to send debugging data. Google itself separates them and uses them in error reports.
https://github.com/DmitriySalnikov/godot_debug_draw_3d/blob/47b686fcf8fe2597fbfd9095fd8f2afe4789d4d4/.github/actions/compile_gdextension/action.yml#L76C1-L84

Using the standard strip from Ubuntu, it turned out to process only x86 libraries and the size decreased from 56 MB to 48 MB. I don't know if there is a strip specifically for android.

It would be nice to see godot-cpp adopt something similar to the SCU builds that Godot itself recently introduced.

Jumbo/Unity builds? This patch is also in the patch folder.. It actually speeds up the build a lot and I use it locally on my PC, but for some reason the size of libraries on all platforms has increased quite a lot with it.

[dsnopek]

But not all libraries are developed by yourself and some may not be updated for a long time. Then why do we need backward compatibility?

Yes, I agree that this is a bug. Classes that aren't found should be treated the same way we are treating unexposed classes - it's just something that was missed. Let's fix it!

This is list of nodes that were not found if my patch was applied to the master branch:

CenterContainer
CheckBox
CheckButton
[snip]

Oh, so you're using objects of these classes in your code, but you also want to exclude the wrappers from your GDExtension, such that they are just wrapped in Object?

That's also a case that I hadn't personally considered. Anyway, it would be fixed by the same change I described above (treating missing classes the same as unexposed classes).

But that's still a different issue from the size increase. Even though they are both caused by the same PR, they will likely have different solutions.

[DmitriySalnikov]

Oh, so you're using objects of these classes in your code, but you also want to exclude the wrappers from your GDExtension, such that they are just wrapped in Object?

It would be nice without spam errors. I also created a separate issue where I added a little about the crash.
But it is unlikely that the usual Object will be enough everywhere. After all, get_child returns Node, and get_theme_default_font Ref<Font> and you may have to interact with them.