Merge pull request #72346 from myaaaaaaaaa/disconnect-order

Avoid sorting CallableCustomMethodPointers by their actual address values
godotengine · Jul 26, 2023 · 53ba9cc · 53ba9cc
2 parents 3bc842b + 5cc9616
commit 53ba9cc
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 16 deletions.
diff --git a/core/object/callable_method_pointer.cpp b/core/object/callable_method_pointer.cpp
@@ -38,13 +38,10 @@ bool CallableCustomMethodPointerBase::compare_equal(const CallableCustom *p_a, c
 		return false;
 	}
 
-	for (uint32_t i = 0; i < a->comp_size; i++) {
-		if (a->comp_ptr[i] != b->comp_ptr[i]) {
-			return false;
-		}
-	}
-
-	return true;
+	// Avoid sorting by memory address proximity, which leads to unpredictable performance over time
+	// due to the reuse of old addresses for newer objects. Use byte-wise comparison to leverage the
+	// backwards encoding of little-endian systems as a way to decouple spatiality and time.
+	return memcmp(a->comp_ptr, b->comp_ptr, a->comp_size * 4) == 0;
 }
 
 bool CallableCustomMethodPointerBase::compare_less(const CallableCustom *p_a, const CallableCustom *p_b) {
@@ -55,15 +52,8 @@ bool CallableCustomMethodPointerBase::compare_less(const CallableCustom *p_a, co
 		return a->comp_size < b->comp_size;
 	}
 
-	for (uint32_t i = 0; i < a->comp_size; i++) {
-		if (a->comp_ptr[i] == b->comp_ptr[i]) {
-			continue;
-		}
-
-		return a->comp_ptr[i] < b->comp_ptr[i];
-	}
-
-	return false;
+	// See note in compare_equal().
+	return memcmp(a->comp_ptr, b->comp_ptr, a->comp_size * 4) < 0;
 }
 
 CallableCustom::CompareEqualFunc CallableCustomMethodPointerBase::get_compare_equal_func() const {

diff --git a/tests/core/object/test_object.h b/tests/core/object/test_object.h
@@ -399,6 +399,29 @@ TEST_CASE("[Object] Signals") {
 		SIGNAL_CHECK("my_custom_signal", empty_signal_args);
 		SIGNAL_UNWATCH(&object, "my_custom_signal");
 	}
+
+	SUBCASE("Connecting and then disconnecting many signals should not leave anything behind") {
+		List<Object::Connection> signal_connections;
+		Object targets[100];
+
+		for (int i = 0; i < 10; i++) {
+			ERR_PRINT_OFF;
+			for (Object &target : targets) {
+				object.connect("my_custom_signal", callable_mp(&target, &Object::notify_property_list_changed));
+			}
+			ERR_PRINT_ON;
+			signal_connections.clear();
+			object.get_all_signal_connections(&signal_connections);
+			CHECK(signal_connections.size() == 100);
+		}
+
+		for (Object &target : targets) {
+			object.disconnect("my_custom_signal", callable_mp(&target, &Object::notify_property_list_changed));
+		}
+		signal_connections.clear();
+		object.get_all_signal_connections(&signal_connections);
+		CHECK(signal_connections.size() == 0);
+	}
 }
 
 } // namespace TestObject