From 1ec5381c1612bd739e31aa2bd04ed287b5fe2aba Mon Sep 17 00:00:00 2001 From: myaaaaaaaaa <103326468+myaaaaaaaaa@users.noreply.github.com> Date: Mon, 30 Jan 2023 11:46:56 -0500 Subject: [PATCH] Store Object signals in a HashMap rather than a VMap --- core/object/object.cpp | 41 ++++++++++++++++---------------- core/object/object.h | 3 +-- core/templates/hashfuncs.h | 6 +++++ tests/core/object/test_object.h | 42 +++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 22 deletions(-) diff --git a/core/object/object.cpp b/core/object/object.cpp index 57aa1339ece..9382674c4e5 100644 --- a/core/object/object.cpp +++ b/core/object/object.cpp @@ -38,6 +38,7 @@ #include "core/os/os.h" #include "core/string/print_string.h" #include "core/string/translation.h" +#include "core/templates/local_vector.h" #include "core/variant/typed_array.h" #ifdef DEBUG_ENABLED @@ -1014,20 +1015,23 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int List<_ObjectSignalDisconnectData> disconnect_data; - //copy on write will ensure that disconnecting the signal or even deleting the object will not affect the signal calling. - //this happens automatically and will not change the performance of calling. - //awesome, isn't it? - VMap slot_map = s->slot_map; - - int ssize = slot_map.size(); + // Ensure that disconnecting the signal or even deleting the object + // will not affect the signal calling. + LocalVector slot_conns; + slot_conns.resize(s->slot_map.size()); + { + uint32_t idx = 0; + for (const KeyValue &slot_kv : s->slot_map) { + slot_conns[idx++] = slot_kv.value.conn; + } + DEV_ASSERT(idx == s->slot_map.size()); + } OBJ_DEBUG_LOCK Error err = OK; - for (int i = 0; i < ssize; i++) { - const Connection &c = slot_map.getv(i).conn; - + for (const Connection &c : slot_conns) { Object *target = c.callable.get_object(); if (!target) { // Target might have been deleted during signal callback, this is expected and OK. @@ -1190,8 +1194,8 @@ void Object::get_all_signal_connections(List *p_connections) const { for (const KeyValue &E : signal_map) { const SignalData *s = &E.value; - for (int i = 0; i < s->slot_map.size(); i++) { - p_connections->push_back(s->slot_map.getv(i).conn); + for (const KeyValue &slot_kv : s->slot_map) { + p_connections->push_back(slot_kv.value.conn); } } } @@ -1202,8 +1206,8 @@ void Object::get_signal_connection_list(const StringName &p_signal, Listslot_map.size(); i++) { - p_connections->push_back(s->slot_map.getv(i).conn); + for (const KeyValue &slot_kv : s->slot_map) { + p_connections->push_back(slot_kv.value.conn); } } @@ -1213,8 +1217,8 @@ int Object::get_persistent_signal_connection_count() const { for (const KeyValue &E : signal_map) { const SignalData *s = &E.value; - for (int i = 0; i < s->slot_map.size(); i++) { - if (s->slot_map.getv(i).conn.flags & CONNECT_PERSIST) { + for (const KeyValue &slot_kv : s->slot_map) { + if (slot_kv.value.conn.flags & CONNECT_PERSIST) { count += 1; } } @@ -1789,11 +1793,8 @@ Object::~Object() { SignalData *s = &E.value; //brute force disconnect for performance - int slot_count = s->slot_map.size(); - const VMap::Pair *slot_list = s->slot_map.get_array(); - - for (int i = 0; i < slot_count; i++) { - slot_list[i].value.conn.callable.get_object()->connections.erase(slot_list[i].value.cE); + for (const KeyValue &slot_kv : s->slot_map) { + slot_kv.value.conn.callable.get_object()->connections.erase(slot_kv.value.cE); } signal_map.erase(E.key); diff --git a/core/object/object.h b/core/object/object.h index 5ec69a371b0..013bccb372e 100644 --- a/core/object/object.h +++ b/core/object/object.h @@ -41,7 +41,6 @@ #include "core/templates/list.h" #include "core/templates/rb_map.h" #include "core/templates/safe_refcount.h" -#include "core/templates/vmap.h" #include "core/variant/callable_bind.h" #include "core/variant/variant.h" @@ -590,7 +589,7 @@ private: }; MethodInfo user; - VMap slot_map; + HashMap> slot_map; }; HashMap signal_map; diff --git a/core/templates/hashfuncs.h b/core/templates/hashfuncs.h index 95e6bad2f23..2a212f3dcb1 100644 --- a/core/templates/hashfuncs.h +++ b/core/templates/hashfuncs.h @@ -386,6 +386,12 @@ struct HashMapHasherDefault { } }; +// TODO: Fold this into HashMapHasherDefault once C++20 concepts are allowed +template +struct HashableHasher { + static _FORCE_INLINE_ uint32_t hash(const T &hashable) { return hashable.hash(); } +}; + template struct HashMapComparatorDefault { static bool compare(const T &p_lhs, const T &p_rhs) { diff --git a/tests/core/object/test_object.h b/tests/core/object/test_object.h index 7e8f23a14f0..98f9b3da65c 100644 --- a/tests/core/object/test_object.h +++ b/tests/core/object/test_object.h @@ -339,6 +339,48 @@ TEST_CASE("[Object] Signals") { CHECK_EQ(signals_after.size(), signals_after_empty_added.size()); } + SUBCASE("Deleting an object with connected signals should disconnect them") { + List signal_connections; + + { + Object target; + target.add_user_signal(MethodInfo("my_custom_signal")); + + ERR_PRINT_OFF; + target.connect("nonexistent_signal1", callable_mp(&object, &Object::notify_property_list_changed)); + target.connect("my_custom_signal", callable_mp(&object, &Object::notify_property_list_changed)); + target.connect("nonexistent_signal2", callable_mp(&object, &Object::notify_property_list_changed)); + ERR_PRINT_ON; + + signal_connections.clear(); + object.get_all_signal_connections(&signal_connections); + CHECK(signal_connections.size() == 0); + signal_connections.clear(); + object.get_signals_connected_to_this(&signal_connections); + CHECK(signal_connections.size() == 1); + + ERR_PRINT_OFF; + object.connect("nonexistent_signal1", callable_mp(&target, &Object::notify_property_list_changed)); + object.connect("my_custom_signal", callable_mp(&target, &Object::notify_property_list_changed)); + object.connect("nonexistent_signal2", 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() == 1); + signal_connections.clear(); + object.get_signals_connected_to_this(&signal_connections); + CHECK(signal_connections.size() == 1); + } + + signal_connections.clear(); + object.get_all_signal_connections(&signal_connections); + CHECK(signal_connections.size() == 0); + signal_connections.clear(); + object.get_signals_connected_to_this(&signal_connections); + CHECK(signal_connections.size() == 0); + } + SUBCASE("Emitting a non existing signal will return an error") { Error err = object.emit_signal("some_signal"); CHECK(err == ERR_UNAVAILABLE);