Optimize GDScriptInstance::notification for better performance

[CrazyRoka]

Optimize GDScriptInstance::notification for better performance

This PR optimizes the GDScriptInstance::notification function to improve performance and reduce CPU usage.

Changes

• Replace List<GDScript *> with a LocalVector<GDScript *> to reduce allocations and deallocations.
• Minimize List push_front() and push_back() operations by reusing the LocalVector across calls.
• Add unlikely() hint for vector growth condition to optimize the common case.
• Reduce overall CPU usage in notification traversal.

Performance Impact

• Eliminates the need for frequent List operations, which were causing significant CPU usage.
• Reduces memory allocations by reusing the vector across calls.
• Improves cache efficiency by using a contiguous memory structure (Vector) instead of a linked list.

Testing

• Tested locally with a scene that contains 20K cubes, all of them were moving around.
• Profiled application before and after my change. Initial implementation was causing 3.35% of CPU usage on my laptop. Optimized version is using 0.5% of CPU.

Profiling data with current mainline

Profiling data with my changes

This optimization should significantly reduce the CPU time spent in GDScriptInstance::notification, particularly for projects with frequent notifications or deep GDScript inheritance hierarchies.

[adamscott]

I see thread_local, I tag @RandomShaper. Just to be sure.

[adamscott]

@CrazyRoka Can you share your test project? I'm curious to run the profiler on my machine before approving your PR.

[CrazyRoka]

@CrazyRoka Can you share your test project? I'm curious to run the profiler on my machine before approving your PR.

I've just created a new repository with my test project. Have a look: https://github.com/CrazyRoka/godot-benchmark-moving-cubes/tree/main

[adamscott]

This is quite an improvement. I tested out @CrazyRoka's test repo and I've seen an improvement of ~61% over one minute of benchmark testing. I've only tested once, so don't take my results too seriously.

Results

Master branch	This PR

I would wait though for @RandomShaper's input though.

[RandomShaper]

First, as a disclaimer, note that I haven't reviewed if the logic is exactly equivalent, but it has been reviewed by others and any failure I guess would be pretty notorious.

Now, I think the changes are very good. If anything, I'd use LocalVector instead of Vector for the thread-local optimization. By the way, the thread_local LocalVector is a pattern we're already using in some other areas of the engine (i.e., rendering). In the future we may want to merge all of them into some kind of arena allocators or something (to try to squeeze even a bit more perf), but at this point, this is already an excellent optimization.

[KoBeWi]

Yeah this should use LocalVector.

[Ivorforce]

I'd be interested which of the changes cause how much of a difference. Specifically, I'm somewhat skeptical of the thread_local use - the rest is harmless i suppose - but if it contributes a significant margin to the speed, it's of course worth it.

[aaronp64]

I dont think we can use thread_local here, as one notification could cause another to be called before it's finished - the second call would overwrite the values in the thread_local vector from the first call while it's still iterating through them. For example, the code below prints "Base1 Base2 Child2 Child1" on master, but "Base1 Base2 Child2 Child2" with this change - the second inheritance chain is overwriting the first in between the Base1 and Child1 calls, so Child1 is replaced with Child2:

func _ready():
	var child1 := Child1.new()
	child1.notification(NOTIFICATION_ENABLED)

class Base1 extends Node:
	func _notification(what: int) -> void:
		if what == NOTIFICATION_ENABLED:
			print("Base1")
			var child2 := Child2.new()
			child2.notification(NOTIFICATION_ENABLED)

class Child1 extends Base1:
	func _notification(what: int) -> void:
		if what == NOTIFICATION_ENABLED:
			print("Child1")
		
class Base2 extends Node:
	func _notification(what: int) -> void:
		if what == NOTIFICATION_ENABLED:
			print("Base2")

class Child2 extends Base2:
	func _notification(what: int) -> void:
		if what == NOTIFICATION_ENABLED:
			print("Child2")

Maybe we could loop through the _base pointers to get the count, then use alloca instead of the List to get a similar speedup?

[DeeJayLSP]

Out of curiosity I tested both removing thread_local and using LocalVector. The profiler results in my machine:

Build	cycles:Pu
Original	0.105%
PR	0.0299%
PR (no thread_local)	0.022%
PR (no thread_local, LocalVector)	0.0209%

This was my first time using a profiler, so please take this with a grain of salt.

Edit: tested again using the test project above:

Build	cycles:Pu	Improvement
Original	0.0355%	-
PR	0.0258%	37.59%
PR (no thread_local)	0.0258%	37.59%
PR (no thread_local, LocalVector)	0.0103%	244.66%
PR (LocalVector)	0.00694%	411.52%

Given the issue with thread_local above, I'd go without it just to be safe.

[CrazyRoka]

@DeeJayLSP Thanks for the advice and deep investigations. I applied your suggestion and used LocalVector.

[Ivorforce]

Changes still don't address Aaron's concerns (#94118 (review)). This needs to be fixed by removing static.
While not using a static might limit the performance somewhat, replacing List is the most important part, so this should still be faster than master in either case.
If we wanted to improve this further, we should use reserve or alloca, but that would require a cached depth on the GDScript, so this is for another time.

[Ivorforce]

While there are more ways to improve this, this should be a straight upgrade on all fronts. Thanks for keeping up with the reviews! Let's finally get this merged.

[Repiteo]

Thanks!