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

Store Object signals in a HashMap rather than a VMap #72421

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Store Object signals in a HashMap rather than a VMap #72421

merged 1 commit into from
Apr 25, 2023

Conversation

myaaaaaaaaa
Copy link
Contributor

Fixes #69874

@myaaaaaaaaa myaaaaaaaaa requested a review from a team as a code owner January 30, 2023 22:41
@clayjohn clayjohn added this to the 4.x milestone Jan 30, 2023
@akien-mga
Copy link
Member

Can you share benchmark results to assess the gain?

@myaaaaaaaaa
Copy link
Contributor Author

myaaaaaaaaa commented Jan 30, 2023

Sure, with the caveat that these numbers are from a debug build

Code:

extends Benchmark

func build_node(num_signals: int) -> Node3D:
	var mesh := ArrayMesh.new()
	var rt := MeshInstance3D.new()
	for i in num_signals:
		var n := MeshInstance3D.new()
		rt.add_child(n)

		# Implicitly connects a changed signal
		n.mesh = mesh

	return rt

func benchmark_signals():
	for i in [2,20,200,2000]:
		print()
		print("num_signals: ", i)
		var m := Time.get_ticks_usec()
		for j in 10: #More reliable timing
			build_node(i).free()
		print("%.3f ms" % ((Time.get_ticks_usec() - m) / 10 / 1e3))

Before: ( Using #72346 for consistent behavior, see that for details )

num_signals: 2
0.223 ms

num_signals: 20
1.612 ms

num_signals: 200
25.631 ms

num_signals: 2000
1421.480 ms

After: ( Note how scaling is now linear )

num_signals: 2
0.221 ms

num_signals: 20
1.368 ms

num_signals: 200
13.461 ms

num_signals: 2000
124.343 ms

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased against latest master), it works. I ran a few projects of mine without issue.

@akien-mga akien-mga modified the milestones: 4.x, 4.1 Apr 3, 2023
@reduz
Copy link
Member

reduz commented Apr 7, 2023

The only problem with this approach is that emission (which is more critical than connection time) could be slower. It would be nice to benchmark this in exchange.

@akien-mga
Copy link
Member

The only problem with this approach is that emission (which is more critical than connection time) could be slower. It would be nice to benchmark this in exchange.

@myaaaaaaaaa Would you have time to do a quick test for this?

@myaaaaaaaaa
Copy link
Contributor Author

The only problem with this approach is that emission (which is more critical than connection time) could be slower. It would be nice to benchmark this in exchange.

@myaaaaaaaaa Would you have time to do a quick test for this?

Preliminary testing (with dev builds) seems to show that the difference is within the margin of error (1.8%)

Code: https://github.com/godotengine/godot-benchmarks/pull/30/files

Results:

{
	"category": "Core > Signal",
	"name": "Emission",
	"results": {
		"time": {
			"a": 1934,
			"b": 1969,
			"a_div_b": 0.9822244794311833
		}
	}
}

@akien-mga akien-mga merged commit e0e93ce into godotengine:master Apr 25, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extreme slowdowns when freeing and reallocating many nodes that share the same Mesh
5 participants