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

Add support for build profiles. #1167

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Jul 7, 2023

Allow enabling or disabling specific classes (which will not be built).

This is similar to upstream godot build_profiles.

Try building the test with:

scons build_profile='build_profile.json'

Partly address #1160.

@Faless
Copy link
Contributor Author

Faless commented Jul 7, 2023

For reference, with the following build profile:

{
        "enabled_classes": [
                "RefCounted",
                "PacketPeer",
                "Object",
                "WebRTCDataChannel",
                "WebRTCDataChannelExtension",
                "WebRTCPeerConnection",
                "WebRTCPeerConnectionExtension"
        ]
}

Those are the resulting size for the webrtc plugin:

upstream: 16M
upstream-stripped: 11M
build_profile: 11M
build_profile-stripped: 9.1M

Note that I had to manually delete:

src/classes/editor_plugin.cpp # Or it would force me to include lots of editor/node/gui/resource stuff.
src/classes/low_level.cpp # Or it would force me to include WorkerThreadPool

And I had to edit godot.cpp to prevent it from including editor_plugin.hpp.

This also hugely decreases the compilation time:

upstream: 2m41,525s
build_profile: 0m7,622s

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 7, 2023

Thanks, Fabio! This is looking awesome so far :-)

It'd be great if ancestors of the enabled classes were automatically included, so you can just specify the WebRTC* classes, and the basic ones like RefCounted and Object wouldn't need to be specificied. That'd probably require binding_generator.py to loop over the data and make an index of parent classes when it started up.

Also, it'd be super awesome to have a script that could scan a GDExtension's source code for #include statements and automatically produce the profile!

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 7, 2023

And I had to edit godot.cpp to prevent it from including editor_plugin.hpp

Ah, we probably need a special case to set a #define to remove the editor plugin stuff when EditorPlugin isn't among the enabled classes.

@dsnopek dsnopek added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Oct 18, 2023
@Faless Faless force-pushed the build/build_profile branch 2 times, most recently from d74cf74 to 081c693 Compare January 18, 2024 20:20
@Faless
Copy link
Contributor Author

Faless commented Jan 18, 2024

I've added support for automatically including parents (and excluding children in the case of disabled classes).

It still needs a few extra manual includes, or some methods will reference undefined symbols.

I think one option is to scan methods and either include the referenced classes, or exclude the offending methods.

binding_generator.py Outdated Show resolved Hide resolved
@Faless Faless force-pushed the build/build_profile branch 5 times, most recently from e7126c7 to 2bb9bdb Compare January 19, 2024 14:47
@Faless
Copy link
Contributor Author

Faless commented Jan 19, 2024

I've pushed some fixes and improvements:

  • The parse function now always includes ClassDB, ClassDBSingleton, and FileAccess if enabled_classes is not empty. They seem to be required even for the most basic builds.
  • Binding generators will still build all files, but the get_file_list will exclude source files based on the profile (i.e. headers classes are always included)

@dsnopek
Copy link
Collaborator

dsnopek commented May 15, 2024

@Faless What else do you want to add before taking this PR out of draft?

I just tested it again after rebasing on master, and aside from needing to add "OS" in the test/build_profile.json (because the latest version of the test project now uses the OS singleton), it worked perfectly for me!

It would be nice to have a tool to help developers create build profiles, but I think it'd be fine to merge this since it's still pretty useful as-is. We can make it easier to use in follow-ups.

@Faless
Copy link
Contributor Author

Faless commented Jun 15, 2024

@Faless What else do you want to add before taking this PR out of draft?

I missed this ping completely, but I wanted to explore the possibility of adding method-scanning to include (and exclude) non-inheritance dependencies.

I've done that that in my last update, this makes creating "enabled_classes" dependencies much easier (at the cost of possibly building slightly more files).

I'll mark this as ready.

@Faless Faless marked this pull request as ready for review June 15, 2024 13:49
@Faless Faless requested a review from a team as a code owner June 15, 2024 13:49
Allow enabling or disabling specific classes (which will not be built).
@Faless
Copy link
Contributor Author

Faless commented Jun 15, 2024

I've also backported this patch to godot-cpp 4.1, 4.0, 3.x (gdnative) to speed up webrtc-native plugin builds.

In case anyone is interested, see: godotengine/webrtc-native#147

@dsnopek
Copy link
Collaborator

dsnopek commented Jun 15, 2024

Thanks, this is looking really great to me!

I've been experimenting with it for the 'godot_openxr_vendors' extension, see: GodotVR/godot_openxr_vendors#149

Something that occurs to me for the future: if we could also eliminate methods that an extension isn't calling, that would allow reducing the classes that get built even further. On 'godot_openxr_vendors' we have some editor plugins, and that leads to tons of classes getting built that we don't use at all, just because EditorPlugin and EditorInterface have methods that use those classes. But what's in this PR is already a huge improvement!

@Faless
Copy link
Contributor Author

Faless commented Jun 17, 2024

Something that occurs to me for the future: if we could also eliminate methods that an extension isn't calling, that would allow reducing the classes that get built even further.

Yeah, sadly that's a bit more complex as far as I can tell.

Class headers seem to have something (methods or types bindings?) which references the constructor of object types in arguments / return values (thus causing undefined references even if you don't use the given method at all).

We should investigate what specifically causes the reference, and change the binding generators so the offending code is either moved to the .cpp file (if possible), or to prevent it's generation in the headers depending on the class inclusion.

@dsnopek
Copy link
Collaborator

dsnopek commented Jun 17, 2024

Class headers seem to have something (methods or types bindings?) which references the constructor of object types in arguments / return values (thus causing undefined references even if you don't use the given method at all).

It's the EngineClassRegistration here, but we need it, in order to solve issues with the correct wrapper being created. I'm not sure if there's a better way to somehow register which C++ class is associated with which class name? The way it's done currently will only trigger the registration if the given header file is included, but that leads to automatically pulling in all dependencies (including classes used as function arguments).

Anyway, that's for another issue/PR to look at :-)

@dsnopek dsnopek merged commit f2ac49a into godotengine:master Jun 17, 2024
12 checks passed
@Faless Faless deleted the build/build_profile branch June 17, 2024 17:18
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2024

Cherry-picked for 4.2 in PR #1527

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2024

Cherry-picked for 4.1 in PR #1529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants