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

Optimize Node::add_child validation #75760

Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 6, 2023

Adding 10k nodes is almost twice as fast.

Code I used to test:

	for count in range(1000, 10001, 1000):
		var start = Time.get_ticks_msec()
		var root = Node.new()
		var name = StringName("pepo") # worst case, triggers name conflict (vs unnamed)
		for i in count:
			var n = Node.new()
			n.set_name(name)
			root.add_child(n)
		var end = Time.get_ticks_msec()
		print("create %s\t%d" % [count, end - start])

Closes godotengine/godot-proposals#6225

@reduz reduz requested a review from a team as a code owner April 6, 2023 15:57
@KoBeWi KoBeWi added this to the 4.1 milestone Apr 6, 2023
uint32_t cn_length = cn.length();
uint32_t c_chars = String::num_characters(c);
uint32_t len = 2 + cn_length + c_chars;
char32_t *str = (char32_t *)alloca(sizeof(char32_t) * (len + 1));
Copy link
Member

Choose a reason for hiding this comment

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

This buffer isn't deleted (the string constructor doesn't take ownership of it)

Also would it make sense to use a thread static shared buffer of like size 64 or so and only allocate this temporary buffer if that is too small?

Alternatively make this buffer a CowData<char32_t> and add a String constructor that takes ownership of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

alloca() allocates on the stack. It's the most efficient way of allocation for this kind of usage and it does not need to be freed.

Copy link
Member

Choose a reason for hiding this comment

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

Or you can just simplest solution that is already implemented; nvm then

scene/main/node.cpp Show resolved Hide resolved
p_int = -p_int;
}
}
while (p_int >= 10) {
Copy link
Member

Choose a reason for hiding this comment

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

if (p_int < 10) return r+0;
if (p_int < 100) return r+1;
// ...

may be faster because it doesn't do divisions, but I didn't test.

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 is probably faster, but the code is a mess, specially because this is int64 :P Still, in this context the code should be ok as-is, you won't get a really meaningful performance gain.

@reduz reduz force-pushed the optimize-node-add-child-validation branch 2 times, most recently from fc071b5 to 0081e5b Compare April 7, 2023 00:11
@reduz reduz requested a review from a team as a code owner April 7, 2023 00:11
@reduz reduz force-pushed the optimize-node-add-child-validation branch from 0081e5b to f4be7c7 Compare April 7, 2023 09:12
Adding 10k nodes is almost twice as fast.
@reduz reduz force-pushed the optimize-node-add-child-validation branch from f4be7c7 to 223ce4f Compare April 7, 2023 11:18
@akien-mga akien-mga merged commit c151d32 into godotengine:master Apr 7, 2023
@akien-mga
Copy link
Member

Thanks!

@lyuma
Copy link
Contributor

lyuma commented Jul 5, 2023

I know this was already merged. I want to put a note for the future that this optimization also changed scene import behavior between Godot 4.0 and Godot 4.1:

As discovered in issue #78881, this changed the rules so that invalid characters are replaced with an underscore _ rather than removed . Given that reduz changed the test to include underscores, this was a deliberate change, and permanent. This affects Import and other things.

Neither this PR nor the commit made a note of this change in behavior. So that's why I'm writing this comment, for folks like you coming from a google search.

Prior to this commit, an object named Widget.001 coming from blender would have been imported as Widget001. Now it's named Widget_001 .

@akien-mga akien-mga changed the title Optimize Node::add_child validation Optimize Node::add_child validation Jul 5, 2023
@dalexeev
Copy link
Member

dalexeev commented Jul 5, 2023

Neither this PR nor the commit made a note of this change in behavior.

See #75760 (comment) and godotengine/godot-proposals#6225 (linked).

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.

Replace invalid node name symbols with an underscore instead of nothing
6 participants