Optimize scene tree groups

[dementive]

Follow up to #108491 that does a similar optimization replacing a List with a Vector in SceneTree::get_nodes_in_group. Before this was copying the node pointers from a Vector into a List but now it just returns the Vector.

Since I was already making changes to groups I also checked if replacing the group_map HashMap with AHashMap would make any difference. I've had success speeding some things up doing that in my own code.

I wrote a quick benchmark in gdscript to check if changing the HashMap type made any difference and it seemed to work pretty well:

func _ready() -> void:
	var total_nodes: int = 10000
	
	var nodes: Array[Node]
	for i: int in range(total_nodes):
		var node: Node = Node.new()
		nodes.append(node)
		add_child(node)
	
	var start_time: int = Time.get_ticks_usec()
	for i: int in range(total_nodes):
		nodes[i].add_to_group("test_group")

	var end_time: int = Time.get_ticks_usec()
	print("Add group time: ", end_time - start_time, " microseconds")

	start_time = Time.get_ticks_usec()
	var group_nodes: Array[Node] = get_tree().get_nodes_in_group("test_group")
	end_time = Time.get_ticks_usec()
	print("Get nodes in group time: ", end_time - start_time, " microseconds")

	print(group_nodes.size(), " nodes")

Results:

Starting performance using HashMap:

Add group time: 933 microseconds
Get nodes in group time: 298 microseconds
1000 nodes


Add group time: 15751 microseconds
Get nodes in group time: 2766 microseconds
10000 nodes

With AHashMap:

Add group time: 369 microseconds
Get nodes in group time: 113 microseconds
1000 nodes


Add group time: 13414 microseconds
Get nodes in group time: 1769 microseconds
10000 nodes

[Ivorforce]

Thanks! The change to return Vector instead of List makes sense to me. I think that should be an easy win.

The change to AHashMap would be nice indeed, but unfortunately, this is not possible, at least with the current implementation.
The reason is that AHashMap moves elements around in its data on change. Therefore, it is impossible to store pointers to mutable AHashMap elements. Node stores a pointer to SceneTree::Group within the hash map, which is the reason the address sanitizer fails in the unit tests.

I recommend reverting the AHashMap change. It might be possible to pursue this change using pointers and explicit allocations, but I think that's out of scope and should be done in a follow-up PR.

[dementive]

The reason is that AHashMap moves elements around in its data on change. Therefore, it is impossible to store pointers to mutable AHashMap elements. Node stores a pointer to SceneTree::Group within the hash map, which is the reason the address sanitizer fails in the unit tests.

Thanks for figuring that out, the asan error was really confusing to me. Makes sense now though, the comments in AHashMap do say exactly that.

On another note I saw that recently in the contributing docs a nice table comparing some of the data structures was added. Might be nice to add another column there about pointer invalidation for each data structure because it isn't very obvious how each of them behave without digging into the source code.

I recommend reverting the AHashMap change. It might be possible to pursue this change using pointers and explicit allocations, but I think that's out of scope and should be done in a follow-up PR.

Sound good will revert that 👍

[Ivorforce]

I think this is worth doing.

[Repiteo]

Thanks!