From edc85d22fc818db527940408cbd44b45ee750a1d Mon Sep 17 00:00:00 2001 From: lawnjelly Date: Sat, 11 Jun 2022 12:35:32 +0100 Subject: [PATCH] Faster queue free Calling queue_free() for large numbers of siblings could previously be very slow, with the time taken rising exponentially with number of children. This looked partly due to ordered_remove from the child list and notifications. This PR identifies objects that are nodes, and sorts the deletion queue so that children are deleted in reverse child order. This minimizes the costs of reordering. --- doc/classes/Node.xml | 1 + scene/main/scene_tree.cpp | 65 ++++++++++++++++++++++++++++++++++++--- scene/main/scene_tree.h | 6 +++- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/doc/classes/Node.xml b/doc/classes/Node.xml index f2ecb8d06d3..ec4960470bc 100644 --- a/doc/classes/Node.xml +++ b/doc/classes/Node.xml @@ -510,6 +510,7 @@ Queues a node for deletion at the end of the current frame. When deleted, all of its child nodes will be deleted as well. This method ensures it's safe to delete the node, contrary to [method Object.free]. Use [method Object.is_queued_for_deletion] to check whether a node will be deleted at the end of the frame. [b]Important:[/b] If you have a variable pointing to a node, it will [i]not[/i] be assigned to [code]null[/code] once the node is freed. Instead, it will point to a [i]previously freed instance[/i] and you should validate it with [method @GDScript.is_instance_valid] before attempting to call its methods or access its properties. + [b]Note:[/b] For efficiency reasons, the final order of deletion is not guaranteed. diff --git a/scene/main/scene_tree.cpp b/scene/main/scene_tree.cpp index 3f3794600d3..7156b6d1283 100644 --- a/scene/main/scene_tree.cpp +++ b/scene/main/scene_tree.cpp @@ -1217,20 +1217,77 @@ void SceneTree::get_nodes_in_group(const StringName &p_group, List *p_li void SceneTree::_flush_delete_queue() { _THREAD_SAFE_METHOD_ - while (delete_queue.size()) { - Object *obj = ObjectDB::get_instance(delete_queue.front()->get()); + // Sorting the delete queue by child count (in respect to their parent) + // is an optimization because nodes benefit immensely from being deleted + // in reverse order to their child count. This is partly due to ordered_remove(), and partly + // due to notifications being sent to children that are moved, further in the child list. + struct ObjectIDComparator { + _FORCE_INLINE_ bool operator()(const DeleteQueueElement &p, const DeleteQueueElement &q) const { + return (p.child_list_id > q.child_list_id); + } + }; + + delete_queue.sort_custom(); + + for (uint32_t e = 0; e < delete_queue.size(); e++) { + ObjectID id = delete_queue[e].id; + Object *obj = ObjectDB::get_instance(id); if (obj) { memdelete(obj); } - delete_queue.pop_front(); } + + delete_queue.clear(); } void SceneTree::queue_delete(Object *p_object) { _THREAD_SAFE_METHOD_ ERR_FAIL_NULL(p_object); + + // Guard against the user queueing multiple times, + // which is unnecessary. + if (p_object->is_queued_for_deletion()) { + return; + } + p_object->_is_queued_for_deletion = true; - delete_queue.push_back(p_object->get_instance_id()); + + DeleteQueueElement e; + e.id = p_object->get_instance_id(); + + // Storing the list id within the parent allows us + // to sort the delete queue in reverse for more efficient + // deletion. + // Note that data.pos could alternatively be read during flush_delete_queue(), + // however reading it here avoids an extra lookup, and should be correct in most cases. + // And worst case if the child_list_id changes in the meantime, it will still work, it may just + // be slightly slower. + const Node *node = Object::cast_to(p_object); + if (node) { + e.child_list_id = node->data.pos; + + // Have some grouping by parent object ID, + // so that children tend to be deleted together. + // This should be more cache friendly. + if (node->data.parent) { + ObjectID parent_id = node->data.parent->get_instance_id(); + + // Use a prime number to combine the group with the child id. + // Provided there are less than the prime number children in a node, + // there will be no collisions. Even if there are collisions, it is no problem. + uint32_t group = parent_id * 937; + + // Rollover the group, we never want the group + the child id + // to overflow 31 bits + group &= ~(0b111 << 29); + e.child_list_id += (int32_t)group; + } + } else { + // For non-nodes, there is no point in sorting them. + e.child_list_id = -2; + } + + delete_queue.push_back(e); } int SceneTree::get_node_count() const { diff --git a/scene/main/scene_tree.h b/scene/main/scene_tree.h index 6ec06c4ffbd..80613d73cb8 100644 --- a/scene/main/scene_tree.h +++ b/scene/main/scene_tree.h @@ -163,7 +163,11 @@ private: void _update_font_oversampling(float p_ratio); void _update_root_rect(); - List delete_queue; + struct DeleteQueueElement { + ObjectID id; + int32_t child_list_id; + }; + LocalVector delete_queue; Map> unique_group_calls; bool ugc_locked;