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

Automatically register only engine classes whose header has been included #1266

Merged

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Oct 12, 2023

This is based on an idea from @maiself at a GDExtension meeting from a couple months ago!

In PR #1050, we added a HashMap that stores the binding callbacks for all engine types, in order to fix a serious bug where the wrapper class used could be an arbitrary parent class (rather than the actual engine class) leading to unexpected behavior in some situations.

Unfortunately, this led to issue #1160 where builds would be ~1-5mb bigger than previously.

This was due to the linker previously being able to optimize away any class that wasn't actually used in the GDExtension. However, if we have to reference every class in order to add their binding callbacks to the HashMap, they are all technically "used", and so none could be optimized away.

@maiself had this really clever idea, which was recorded in the meeting notes as:

// some register header

template<Class>
class Register {
	Register(class_name) {
		hash_map.insert({class_name, type_info(Class)});
	}
};

// node.hpp

static inline Register<Node>("Node"); 

This would mean that classes are only added to the HashMap, if the developer actually includes the header file for them! This should allow the linker to do it's old optimization again (I hope :-))

This PR attempts to implement that, however, I encountered some interesting issues when actually trying to do it:

  • static inline variables can't be anonymous -- apparently the variable name is used to tell that each declaration refers to the same object, so all but one can be removed by the linker (which what the inline is aiming to do here).
  • We can't directly add the items to the HashMap in the registration object, because the HashMap is keyed by StringName, and we can't create StringName's until after godot-cpp has been initialized. So, instead, the EngineClassRegistration<T> constructor is adding a callback which will do the actual registration at the appropriate time.
  • We can't simply declare the std::vector as a static variable at the compilation unit level, because static variables can be initialized in any order, and we can't know that our inline static EngineClassRegistration<T> variables won't be initialized before the std::vector. This is why that is a static variable at the function level, so that it will be initialized on demand.

None of those are really necessarily a problem, but it's why the code in this PR is more complicated than the simple psuedo-code that I started with :-)

Any feedback on how I could simplify this would be appreciated! But, personally, assuming it actually solves the problem, I think even with all these complications this is still an acceptable way to do this.

Fixes #1160

@dsnopek dsnopek added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation cherrypick:4.1 labels Oct 12, 2023
@dsnopek dsnopek added this to the 4.x milestone Oct 12, 2023
@dsnopek dsnopek requested a review from a team as a code owner October 12, 2023 15:24
@dsnopek dsnopek removed the request for review from a team October 12, 2023 15:25
@dsnopek dsnopek force-pushed the automatic-engine-class-registration branch from 497bc13 to 09a3fa6 Compare October 12, 2023 15:27
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 12, 2023

Compiling the test GDExtension (used in the automated tests) with the default scons flags (so, it's the debug template) on Ubuntu 20.04 with g++ 11.4.0, I'm seeing:

  • 1.6mb (with this PR)
  • 4.7mb (on master at ef2f63a)

So, it appears to be working, at least in my case!

I'd be very curious to see what results others get with other compilers on other OS's.

EDIT: Updated after getting editor plugin related classes correctly excluded as well.

@mihe
Copy link
Contributor

mihe commented Oct 12, 2023

My setup in Godot Jolt is admittedly somewhat unorthodox compared to most other godot-cpp usage, so this is not really a representative sample, but here are the numbers I'm seeing on Windows:

Before After Change (KB) Change (%)
MSVC (debug) 3204 KB 3073 KB -131 KB -4.1%
MSVC (release) 3094 KB 2959 KB -135 KB -4.4%
clang-cl (debug) 3704 KB 3529 KB -175 KB -4.7%
clang-cl (release) 3588 KB 3395 KB -193 KB -5.4%

It's certainly an improvement across the board, but nothing astronomical in my particular case. I can give it a try on other platforms later to see if it's specific to the linker/platform perhaps.

For reference, I saw about a ~1.2 MB increase with clang-cl when #1050 was introduced, and a ~2 MB bump with MSVC. I have since added quite a few godot-cpp includes though (the total of which can be seen here) so I assume I wouldn't win it all back even if this PR had cancelled out #1050 completely.

@dsnopek dsnopek force-pushed the automatic-engine-class-registration branch 3 times, most recently from 78d7257 to 7222c38 Compare October 12, 2023 19:16
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 12, 2023

Based on a RocketChat conversation with @mihe and @maiself, I've come to realize that I was trying to use static inline variables incorrectly -- I've adjusted it and now the callbacks can be stored in a std::vector rather than std::unordered_set, eliminating one of the complicated workarounds.

Also, I noticed that we were still including some editor plugin classes, even if the GDExtension didn't do anything with editor plugins. So, I rearranged some of that code, and now those will be excluded too! It saved an additional ~200kb in my testing.

@dsnopek dsnopek force-pushed the automatic-engine-class-registration branch from 7222c38 to ecadb65 Compare October 12, 2023 19:29
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 12, 2023

@mihe Thanks for testing and posting your numbers!

For reference, I saw about a ~1.2 MB increase with clang-cl when #1050 was introduced, and a ~2 MB bump with MSVC. I have since added quite a few godot-cpp includes though (the total of which can be seen here) so I assume I wouldn't win it all back even if this PR had cancelled out #1050 completely.

Hm, you're probably right that you have enough includes now that it won't cancel out all of the size gains from #1050, but the difference still seems quite large (~200kb vs 1.2mb). It'd be really interesting to try reverting #1050 and see if you reclaim the same amount of memory, or if it's somewhat less, although, reverting it would probably be pretty messy at this point given all the changes that have been made since then.

@dsnopek dsnopek force-pushed the automatic-engine-class-registration branch from ecadb65 to 5c9ee1c Compare October 12, 2023 19:36
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 12, 2023

I just tried the latest PR with my SG Physics 2D extension (using the same platform, compiler and setup as described in my previous comment) and got this result:

  • 3.5mb (with this PR)
  • 6.4mb (on master at ef2f63a)

So, I'm personally seeing a pretty dramatic improvement in this case!

@mihe
Copy link
Contributor

mihe commented Oct 12, 2023

It'd be really interesting to try reverting #1050 and see if you reclaim the same amount of memory, or if it's somewhat less, although, reverting it would probably be pretty messy at this point given all the changes that have been made since then.

Seeing as how this PR has gotten rid of the generated register_engine_classes, I guess it largely already has reverted it.

As such, I just now tried simply removing that new inline static variable you added to GDEXTENSION_CLASS_ALIAS, which obviously won't function correctly, but still compiles, and I saw a reduction of about -0.6 MB.

I also tried building on Linux with both Clang and GCC, and with this PR I'm still seeing very modest reductions in my project, in the range of -0.1-ish MB, but when I omit this new inline static variable I'm seeing reductions up to -1.3 MB.

I guess I'll need to fire up SizeBench to better understand what's happening. Either way, clearly this PR is a net positive, so I'm all for it.

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

I didn't test the size reduction but code-wise this looks good, and the discussion seems to agree it's an improvement.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Just a tiny nitpick, looks good otherwise

include/godot_cpp/classes/editor_plugin_registration.hpp Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the automatic-engine-class-registration branch from 5c9ee1c to b507b3e Compare October 16, 2023 15:19
@dsnopek dsnopek merged commit 64eac01 into godotengine:master Oct 16, 2023
12 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 23, 2023

Cherry-picked for 4.1 in PR #1281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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