-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Change Node set_name
to use StringName, slightly improves performance
#76560
base: master
Are you sure you want to change the base?
Conversation
6c86774
to
5d28466
Compare
5d28466
to
905e400
Compare
905e400
to
695b04f
Compare
695b04f
to
2fb90cb
Compare
Since this breaks ABI compatibility, what can I do to preserve compatibility? |
2fb90cb
to
7b3b337
Compare
This should be addressable via And after that you should remove this line from the expected errors (it won't be needed once you add the compatibility method):
|
7b3b337
to
5ec8236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And Validate extension JSON: Error: Field 'classes/Node/methods/set_name/arguments/0': type changed value in new API, from "String" to "StringName".
needs to be added to 4.1-stable.expected
instead of the hash error that you removed.
5ec8236
to
50b1830
Compare
50b1830
to
54e9fb9
Compare
This doesn't break compatibility in C# because we don't expose the property accessors, so no additional changes should be needed for C#. With this PR, we likely don't need to special-case this anymore in the C# bindings generator: godot/modules/mono/editor/bindings_generator.cpp Lines 1951 to 1953 in 6afd320
|
f272666
to
5129f1a
Compare
f8117cd
to
5578944
Compare
5578944
to
2dacd28
Compare
2dacd28
to
ed0f992
Compare
79feb41
to
9b711b5
Compare
9b711b5
to
b628896
Compare
b628896
to
ef9b8c8
Compare
ef9b8c8
to
3f2b192
Compare
3f2b192
to
db0179d
Compare
db0179d
to
b23622e
Compare
b23622e
to
9a8cbb6
Compare
3e3a3eb
to
2d35330
Compare
2d35330
to
ffca5e8
Compare
ffca5e8
to
8aede51
Compare
This PR changes the
Node::set_name()
method to use StringName. This code skips StringName construction in the case where the desired name is already a valid node name. This improves performance by about 15% to 20% in my testing in the case where the desired name is already valid (the common case, the happy code path).The code has been split up into multiple methods.
invalid_node_name_index()
finds the index of the first character that would be invalid for a node name. Then this is passed tovalidate_node_name_internal()
which starts replacing the invalid characters from that index. The same logic used to exist combined into one method, but having it be separate allows us to speed upNode::set_name()
. The logic ininvalid_node_name_index()
is even slightly simpler because it doesn't have to keep track ofbool valid
, it just returns if invalid. However, the existingvalidate_node_name()
is still present as a wrapper, for being exposed to GDScript and is also used in many other parts of the codebase. I also renamed some local variables for readability. I'm open to any suggestions if this can be improved further.Test and benchmarking code:
Results:
Without calling set_name, just accessing the array:
Time elapsed: 28971 (varies between about 25k and 30k)
This PR with valid name, skips StringName construction:
Time elapsed: 132565 (varies between about 120k and 140k)
This PR with invalid name:
Time elapsed: 220087 (varies between about 200k and 230k)
Current master with valid name:
Time elapsed: 160325 (varies between about 150k and 170k)
Current master with invalid name:
Time elapsed: 213259 (varies between about 190k and 220k)
I also did some testing with longer StringNames and the speed improvement gets more significant as length increases.