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

[4.x] Faster queue free #62446

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/classes/Node.xml
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@
<return type="void" />
<description>
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]Note:[/b] For efficiency reasons, the final order of deletion is not guaranteed.
</description>
</method>
<method name="remove_child">
Expand Down
65 changes: 61 additions & 4 deletions scene/main/scene_tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1051,20 +1051,77 @@ void SceneTree::get_nodes_in_group(const StringName &p_group, List<Node *> *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<ObjectIDComparator>();

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<Node>(p_object);
if (node) {
e.child_list_id = node->data.index;

// 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 = (uint64_t)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;
Copy link
Member

Choose a reason for hiding this comment

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

Why -2?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was probably -2 for historical reasons rather than -1 (for debugging).

The actual value (as long as it is constant) only matters in terms of the sorting relation with the nodes. If you wanted the objects to be deleted after the nodes for instance you might use a high number. The negative value ensures the objects will be deleted before the nodes, and is probably one of the reasons I used int32_t over uint32_t. (If we used 0 as the constant, then there is the possibility of a node deleted before the objects, not that it probably matters that much.)

}

delete_queue.push_back(e);
}

int SceneTree::get_node_count() const {
Expand Down
7 changes: 6 additions & 1 deletion scene/main/scene_tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

#include "core/os/main_loop.h"
#include "core/os/thread_safe.h"
#include "core/templates/local_vector.h"
#include "core/templates/self_list.h"
#include "scene/resources/mesh.h"

Expand Down Expand Up @@ -137,7 +138,11 @@ class SceneTree : public MainLoop {
int call_lock = 0;
HashSet<ObjectID> call_skip; // Skip erased nodes. Store ID instead of pointer to avoid false positives when node is freed and a new node is allocated at the pointed address.

List<ObjectID> delete_queue;
struct DeleteQueueElement {
ObjectID id;
int32_t child_list_id;
};
LocalVector<DeleteQueueElement> delete_queue;

HashMap<UGCall, Vector<Variant>, UGCall> unique_group_calls;
bool ugc_locked = false;
Expand Down