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

Changed logic and optimized ObjectID in ObjectDB and Variant, removed… #36189

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 13, 2020

Refactored how ObjectID works in ObjectDB, it is now very fast to obtain an object pointer from an ObjectID. Also, RefPtr was removed and the referencing logic for Refecence has been replicated inside Variant. This hugely simplifies the code and logic.

Finally, the functions to check if an object pointer is invalid were removed given they are dangerous and unfixable. Instead, Variant now contains an ObjectID together with the pointer, which ensures validity can be checked at all times.

Also fixes #32383

@@ -680,8 +685,8 @@ class Object {

/* SCRIPT */

void set_script(const RefPtr &p_script);
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to do this because RefPtr no longer exists, should be fine I think

@@ -760,49 +766,63 @@ void postinitialize_handler(Object *p_object);

class ObjectDB {

struct ObjectPtrHash {
Copy link
Member Author

Choose a reason for hiding this comment

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

Doubt we will ever have more than 16 million object simultaneous instances, if we do we can always change this :P but giving priority (more bits) to the validator is usually better.

core/variant.cpp Outdated Show resolved Hide resolved
@@ -1589,14 +1617,12 @@ String Variant::stringify(List<const void *> &stack) const {
case OBJECT: {

if (_get_obj().obj) {
#ifdef DEBUG_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this #ifdef stay?

Copy link
Member Author

Choose a reason for hiding this comment

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

No... stringify is not something that will affect performance anyway so it should be fine. The ifdefs were there because the reverse hash map in 3.x at some point was removed durin release build but then kept. Now it's gone so there is no longer a point.

Copy link
Member

Choose a reason for hiding this comment

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

That's not the reason for my comment (the #endif ends anyway before the call to to_string()).

My point is that all other if (ScriptDebugger::get_singleton() [...] checks are inside an #ifdef DEBUG_ENABLED, because there's no point in non-debug builds. It's only for consistency.

@@ -979,7 +979,7 @@ void Object::set_script_and_instance(const RefPtr &p_script, ScriptInstance *p_i
script_instance = p_instance;
}

void Object::set_script(const RefPtr &p_script) {
void Object::set_script(const Variant &p_script) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Ref<Script>?

Copy link
Member Author

Choose a reason for hiding this comment

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

you can't, because Ref uses Reference, which inherits Object.. and since its a template, you can't forward declare it.

core/variant.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

This fixes the mono build:

diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 43cdd19411..c722076fe2 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -854,7 +854,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 
 		while (script->instances.front()) {
 			Object *obj = script->instances.front()->get();
-			obj->set_script(RefPtr()); // Remove script and existing script instances (placeholder are not removed before domain reload)
+			obj->set_script(REF()); // Remove script and existing script instances (placeholder are not removed before domain reload)
 		}
 
 		script->_clear();
@@ -877,7 +877,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 
 				// Use a placeholder for now to avoid losing the state when saving a scene
 
-				obj->set_script(scr.get_ref_ptr());
+				obj->set_script(scr);
 
 				PlaceHolderScriptInstance *placeholder = scr->placeholder_instance_create(obj);
 				obj->set_script_instance(placeholder);
@@ -1003,7 +1003,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 				CRASH_COND(si != NULL);
 #endif
 				// Re-create script instance
-				obj->set_script(script.get_ref_ptr()); // will create the script instance as well
+				obj->set_script(script); // will create the script instance as well
 			}
 		}
 
diff --git a/modules/mono/mono_gd/gd_mono_internals.cpp b/modules/mono/mono_gd/gd_mono_internals.cpp
index 75aa77c7b0..74ffa90cb3 100644
--- a/modules/mono/mono_gd/gd_mono_internals.cpp
+++ b/modules/mono/mono_gd/gd_mono_internals.cpp
@@ -107,7 +107,7 @@ void tie_managed_to_unmanaged(MonoObject *managed, Object *unmanaged) {
 
 	ScriptInstance *si = CSharpInstance::create_for_managed_type(unmanaged, script.ptr(), gchandle);
 
-	unmanaged->set_script_and_instance(script.get_ref_ptr(), si);
+	unmanaged->set_script_and_instance(script, si);
 }
 
 void unhandled_exception(MonoException *p_exc) {

See above note on shadowing though, this warning spams the build a lot.

@reduz reduz force-pushed the object-id-refactor branch 3 times, most recently from 1521c0b to dcbd52f Compare February 14, 2020 14:03
@akien-mga akien-mga added this to the 4.0 milestone Feb 14, 2020
@reduz reduz force-pushed the object-id-refactor branch from dcbd52f to 867d073 Compare February 15, 2020 11:39
@akien-mga akien-mga merged commit 264f20f into godotengine:master Feb 15, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference to freed object can return a random different one
5 participants