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

static inline is bad. Generates too much code that the linker can't remove. #1621

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

static inline is bad. Generates too much code that the linker can't remove. #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/godot_cpp/classes/ref.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class Ref {

// Used exclusively in the bindings to recreate the Ref Godot encapsulates in return values,
// without adding to the refcount.
inline static Ref<T> _gde_internal_constructor(Object *obj) {
static Ref<T> _gde_internal_constructor(Object *obj) {
Ref<T> r;
r.reference = (T *)obj;
return r;
Expand Down
4 changes: 2 additions & 2 deletions include/godot_cpp/classes/wrapped.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class Wrapped {
GDExtensionObjectPtr owner;
RecreateInstance *next;
};
inline static RecreateInstance *recreate_instance = nullptr;
static RecreateInstance *recreate_instance;
Copy link
Collaborator

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

#endif

void _notification(int p_what) {}
Expand Down Expand Up @@ -408,7 +408,7 @@ protected:
// Don't use this for your classes, use GDCLASS() instead.
#define GDEXTENSION_CLASS_ALIAS(m_class, m_alias_for, m_inherits) /******************************************************************************************************************/ \
private: \
inline static ::godot::internal::EngineClassRegistration<m_class> _gde_engine_class_registration_helper; \
static ::godot::internal::EngineClassRegistration<m_class> _gde_engine_class_registration_helper; \
Copy link
Collaborator

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 #included.

Copy link
Author

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.

Copy link
Collaborator

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.

void operator=(const m_class &p_rval) {} \
friend class ::godot::ClassDB; \
friend class ::godot::Wrapped; \
Expand Down
4 changes: 4 additions & 0 deletions src/classes/wrapped.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ namespace godot {
thread_local const StringName *Wrapped::_constructing_extension_class_name = nullptr;
thread_local const GDExtensionInstanceBindingCallbacks *Wrapped::_constructing_class_binding_callbacks = nullptr;

#ifdef HOT_RELOAD_ENABLED
Wrapped::RecreateInstance *Wrapped::recreate_instance = nullptr;
#endif

const StringName *Wrapped::_get_extension_class_name() {
return nullptr;
}
Expand Down
Loading