I can confirm that this PR increased type-checking performance using is_class (at least in my test scenarios). I didn't do the most scientific testing, so don't wish to report the numbers, but it is a definite improvement. (interestingly, it seems GDScript's is operator doesn't run any faster, so might be we need to look at optimizing that?)

The duplication of data and relationships irks me a bit, but I imagine it's out of scope of this PR to look at changing these. Looking forward to the ClassDB cleanup! :)

I don't understand the purpose of clearing _gdtype_ptr in Object::_predelete(). This results in Object::get_gdtype() returning _get_typev(). Which should be it's value in most cases anyway. Which would mean that for the duration of the Dispose() call, C# wouldn't have access to accurate type information? (Just a lot of stuff I know nothing about tho. And I felt it was necessary to express my concern and lack of knowledge.)

I feel like it should be set during the Object constructor, instead of during initialization? This would allow us to get rid of the check in get_gdtype() (saving a few CPU cycles) and ensure that it is always valid. It also doesn't really need to be set to nullptr during destruction.

This is the first time I saw the GDType changes! Looks like a solid start! :D

Would it be worthwhile changing these to static variables and using static constructors for in-built classes? That way we don't have to dynamically allocate memory for them, and only have to initialize the properties.

PR looks good overall!

Only major issue is the possible memory leak concern. Maybe some confusion/concern around _reset_gdtype().

The ClassDB changes look good! I'm really excited for the future of this! :D

Thanks a lot for the review!

I can confirm that this PR increased type-checking performance using is_class (at least in my test scenarios). I didn't do the most scientific testing, so don't wish to report the numbers, but it is a definite improvement. (interestingly, it seems GDScript's is operator doesn't run any faster, so might be we need to look at optimizing that?)

The singular branch is unlikely to be a bottleneck, especially when compared to some of the other logic slowing these functions down. So I wouldn't expect any observable performance improvements just from this change, especially on hot execution.

I don't understand the purpose of clearing _gdtype_ptr in Object::_predelete(). This results in Object::get_gdtype() returning _get_typev(). Which should be it's value in most cases anyway. Which would mean that for the duration of the Dispose() call, C# wouldn't have access to accurate type information? (Just a lot of stuff I know nothing about tho. And I felt it was necessary to express my concern and lack of knowledge.)
I feel like it should be set during the Object constructor, instead of during initialization? This would allow us to get rid of the check in get_gdtype() (saving a few CPU cycles) and ensure that it is always valid. It also doesn't really need to be set to nullptr during destruction.

It's part of the Object lifecycle, and has been the case for a while. It's essentially a type safety feature, though it's unlikely to be very impactful. The short explanation is that it's part of the C++ contract of object destruction that subtypes leave no traces. For this reason, for example, vtables are exchanged with supertypes' vtables after each subtype is done destructing.

It's possible we can change this eventually, but now is not the time.

Would it be worthwhile changing these to static variables and using static constructors for in-built classes? That way we don't have to dynamically allocate memory for them, and only have to initialize the properties.

Built-in classes are already created statically, the only classes that are created dynamically are those created from GDExtension (which this PR focuses on).

@dsnopek Thanks a lot for testing! I had another look, and it seems like I failed to realize that GDExtension::_register_extension_class_internal is capable of hot reload.
For now, there are no properties that GDType needs to hot reload, so I was able to fix the issue simply be removing the create_gdtype call entirely when the function is called as a hot reload. But it's good that I know this now, because hot reload will need to be considered for all future additions to GDType.

Thanks, however, in the latest version of this PR, I'm still getting those errors on reload.

I suspect the right thing to do is to have GDExtension::_clear_extension() delete the GDType from GDExtension, so it'll get recreated. Something like this:

diff --git a/core/extension/gdextension.cpp b/core/extension/gdextension.cpp
index ad5b6a8442f..8c38ae0be48 100644
--- a/core/extension/gdextension.cpp
+++ b/core/extension/gdextension.cpp
@@ -976,6 +976,8 @@ void GDExtension::_clear_extension(Extension *p_extension) {
 
                obj->clear_internal_extension();
        }
+
+       p_extension->gdextension.destroy_gdtype();
 }
 
 void GDExtension::track_instance_binding(Object *p_object) {
diff --git a/core/object/object.cpp b/core/object/object.cpp
index 2ffea4aefa3..10bd258b707 100644
--- a/core/object/object.cpp
+++ b/core/object/object.cpp
@@ -236,6 +236,19 @@ void ObjectGDExtension::create_gdtype() {
        gdtype = memnew(GDType(ClassDB::get_gdtype(parent_class_name), class_name));
 }
 
+void ObjectGDExtension::destroy_gdtype() {
+       ERR_FAIL_COND(!gdtype);
+
+       memdelete(const_cast<GDType *>(gdtype));
+       gdtype = nullptr;
+}
+
+ObjectGDExtension::~ObjectGDExtension() {
+       if (gdtype) {
+               memdelete(const_cast<GDType *>(gdtype));
+       }
+}
+
 bool Object::Connection::operator<(const Connection &p_conn) const {
        if (signal == p_conn.signal) {
                return callable < p_conn.callable;
diff --git a/core/object/object.h b/core/object/object.h
index 11cb9095855..3438e5ece56 100644
--- a/core/object/object.h
+++ b/core/object/object.h
@@ -374,6 +374,9 @@ struct ObjectGDExtension {
        /// This is not exposed through the GDExtension API (yet) so it is inferred from above parameters.
        const GDType *gdtype;
        void create_gdtype();
+       void destroy_gdtype();
+
+       ~ObjectGDExtension();
 };
 
 #define GDVIRTUAL_CALL(m_name, ...) _gdvirtual_##m_name##_call(__VA_ARGS__)

When I use that, I don't get the errors anymore!

Also, it appears the reload isn't full working with the latest version of this PR (whereas before there were the errors, but it still worked), at least using the same test as I used last time (I've got a Node in the scene with _process() printing something to the log - after the reload it just stops printing). This issue isn't fixed by my patch, and it appears to be broken on master as well - more investigation is needed

Also, it appears the reload isn't full working with the latest version of this PR (whereas before there were the errors, but it still worked), at least using the same test as I used last time (I've got a Node in the scene with _process() printing something to the log - after the reload it just stops printing). This issue isn't fixed by my patch, and it appears to be broken on master as well - more investigation is needed

This turned out to be a godot-cpp issue godotengine/godot-cpp#1876

Thanks, however, in the latest version of this PR, I'm still getting those errors on reload.

Sorry about that! Somehow, I managed to patch the wrong part of the PR with the fix. I've corrected this now. With the latest version, I can hot reload without any error messages (macOS).

I suspect the right thing to do is to have GDExtension::_clear_extension() delete the GDType from GDExtension, so it'll get recreated. Something like this:

Patch
When I use that, I don't get the errors anymore!

Unfortunately, this is not an option. The GDType must persist, because existing Object classes may have a reference to it. Remember that we're doing this because Object should have a pointer to its GDType, to avoid needing to jump through hoops during use to find out what the right thing to do is given its internal subclass, gdextension subclass, and scripting subclass.
GDType will therefore have to support hot reload in the debug builds (or rather, step by step adopt the hot reload implementation that is currently in GDExtension).

Unfortunately, this is not an option. The GDType must persist, because existing Object classes may have a reference to it.

The reload process goes through all instances and resets the GDType to the one from the native class. Unless you're saying there's something that is getting missed?

But assuming nothing is getting missed, if you look at my patch above, in the first chunk where it's calling destroy_gdtype() is right after it has finished looping over all the instances and and calling clear_internal_extension(), so there shouldn't be any remaining references to the GDType from GDExtension.

Unfortunately, this is not an option. The GDType must persist, because existing Object classes may have a reference to it.

The reload process goes through all instances and resets the GDType to the one from the native class. Unless you're saying there's something that is getting missed?

Oh that's interesting! That totally explains your suggestion.
I'm a bit surprised though. How does exchanging _extension deal with potential multithreading problems? During the brief moment of writing to the _extension field, any reads to _extension might fail.

How does exchanging _extension deal with potential multithreading problems? During the brief moment of writing to the _extension field, any reads to _extension might fail.

It doesn't - at the moment, reload is unsafe with regard to multithreading. This was one of the "let's get this out now, and fix it later" things. :-) That said, I don't think multithreading issues affect very many users - maybe more often with editor plugins that spawn threads? And they could maybe handle it manually by stopping their threads on NOTIFICATION_EXTENSION_RELOAD

The solution is probably some variation of protecting access to _extension with a mutex, but only if that GDExtension has reload enabled, so we aren't paying for that on extensions that don't need it? Something for another PR or issue, though

Alright, thanks for clearing that up. I'm totally happy to go with your suggestion then!

The hot reload seems to work for me with your suggestions applied, without any errors printed¹.

And they could maybe handle it manually by stopping their threads on NOTIFICATION_EXTENSION_RELOAD
The solution is probably some variation of protecting access to _extension with a mutex, but only if that GDExtension has reload enabled, so we aren't paying for that on extensions that don't need it? Something for another PR or issue, though

An idea i had was to synchronize threads when a reload is scheduled, and only start exchanging extension pointers when they're all paused. That would make hot reload compatibility completely free, but if there's any long running threads, it would take a while to reload.
Anyway, if people haven't been complaining so far, it probably isn't a priority.

Thanks!