Change Node `set_name` to use StringName, slightly improves performance #76560

aaronfranke · 2023-04-28T22:07:04Z

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 to validate_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 up Node::set_name(). The logic in invalid_node_name_index() is even slightly simpler because it doesn't have to keep track of bool valid, it just returns if invalid. However, the existing validate_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:

func _ready():
	var nodes: Array = []
	for i in range(1000000):
		nodes.append(Node.new())
	var start_time = Time.get_ticks_usec()
	for i in range(1000000):
		nodes[i].set_name(&"MyStringName")
	var end_time = Time.get_ticks_usec()
	print("Time elapsed: " + str(end_time - start_time))

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.

Bugsquad edit, closes: Node::set_name and Node::get_name have inconsistent types #96805

aaronfranke · 2023-09-04T05:10:52Z

Since this breaks ABI compatibility, what can I do to preserve compatibility?

dsnopek · 2023-09-25T00:30:44Z

Since this breaks ABI compatibility, what can I do to preserve compatibility?

This should be addressable via bind_compatibility_method() (following the usual pattern with node.compat.inc, and _bind_compatibility_methods(), etc - take a look at one of the examples already in the engine, ie in TileMap)

And after that you should remove this line from the expected errors (it won't be needed once you add the compatibility method):

Validate extension JSON: Error: Hash changed for 'classes/Node/methods/set_name', from 04FD3184 to C4FB126E.

RedworkDE

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.

core/string/ustring.cpp

scene/main/node.h

raulsntos · 2023-11-01T17:29:40Z

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

    
           // Special case for Node::set_name 
        
           bool whitelisted = getter->return_type.cname == name_cache.type_StringName && 
        
           		setter_first_arg.type.cname == name_cache.type_String;

core/string/ustring.h

scene/main/node.h

misc/extension_api_validation/4.2-stable.expected

Ivorforce

This PR is very welcome! I had actually considered proposing this myself at some point, since it has very obvious performance improvement potential.
The slight slow down on invalid name calls is expected but acceptable as the cold path.

I think however your change can be made a bit simpler by focusing on the main proposal of changing the parameter, I don't think the change to validate_node_name is needed :)

core/string/ustring.cpp

Ivorforce

Looks good code wise. I cannot vouch for the correctness of the compat header, but it looks correct too.

To re-iterate, I agree this is a good change. It's always good to match parameters closely to what is actually needed, and the tests show it's impactful.

I think there is one case where it breaks compatibility to GDScript code, which would be NodePath arguments:

set_name(^"Test")

However, I think this is acceptable.

scene/main/node.cpp

Calinou

Tested locally, it works as expected. Code looks good to me.

Testing project: test_node_stringname_performance.zip
Same benchmark as in OP, but with 5 million iterations for each scenario instead of 1 million to reduce variance. (10 million will crash due to the node count being too high, even in master.)

Benchmark

PC specifications

CPU: Intel Core i9-13900K
GPU: NVIDIA GeForce RTX 4090
RAM: 64 GB (2×32 GB DDR5-5800 C30)
SSD: Solidigm P44 Pro 2 TB
OS: Linux (Fedora 41)

Using a release optimized export template (production=yes lto=full).

`master`

Time elapsed (valid name): 514795 usecs
Time elapsed (invalid name): 642047 usecs

This PR

Time elapsed (valid name): 458555 usecs
Time elapsed (invalid name): 654856 usecs

Repiteo · 2025-05-01T19:25:54Z

Needs rebase

Repiteo · 2025-05-02T14:31:51Z

Thanks!

This fixes `Node::set_name()` to release the old unique name before performing the rename. godotengine#76560 changed the code to update `data.name` before calling `_release_unique_name_in_owner()`, causing to incorrectly try releasing the new name instead of the old name. Fixes godotengine#108683

aaronfranke added enhancement topic:core performance labels Apr 28, 2023

aaronfranke requested a review from a team as a code owner April 28, 2023 22:07

Calinou added this to the 4.x milestone Apr 28, 2023

aaronfranke force-pushed the node-set-string-name branch from 6c86774 to 5d28466 Compare May 19, 2023 19:55

aaronfranke force-pushed the node-set-string-name branch from 5d28466 to 905e400 Compare June 11, 2023 06:46

aaronfranke force-pushed the node-set-string-name branch from 905e400 to 695b04f Compare August 13, 2023 08:19

aaronfranke force-pushed the node-set-string-name branch from 695b04f to 2fb90cb Compare September 4, 2023 05:10

aaronfranke force-pushed the node-set-string-name branch from 2fb90cb to 7b3b337 Compare September 24, 2023 22:04

aaronfranke requested review from a team as code owners September 24, 2023 22:04

aaronfranke force-pushed the node-set-string-name branch from 7b3b337 to 5ec8236 Compare September 25, 2023 01:05

RedworkDE reviewed Sep 25, 2023

View reviewed changes

core/string/ustring.cpp Outdated Show resolved Hide resolved

scene/main/node.h Outdated Show resolved Hide resolved

aaronfranke force-pushed the node-set-string-name branch from 5ec8236 to 50b1830 Compare September 25, 2023 06:10

aaronfranke force-pushed the node-set-string-name branch from 50b1830 to 54e9fb9 Compare November 1, 2023 00:06

aaronfranke force-pushed the node-set-string-name branch 2 times, most recently from f272666 to 5129f1a Compare December 6, 2023 06:13

aaronfranke requested a review from a team as a code owner December 6, 2023 06:13

aaronfranke force-pushed the node-set-string-name branch 2 times, most recently from f8117cd to 5578944 Compare December 8, 2023 08:04

aaronfranke mentioned this pull request Dec 8, 2023

CI: Add 4.2-stable.expected file for GDExtension compatibility checks #85748

Merged

aaronfranke force-pushed the node-set-string-name branch from 5578944 to 2dacd28 Compare January 6, 2024 08:28

AThousandShips reviewed Jan 6, 2024

View reviewed changes

core/string/ustring.h Outdated Show resolved Hide resolved

scene/main/node.h Outdated Show resolved Hide resolved

misc/extension_api_validation/4.2-stable.expected Outdated Show resolved Hide resolved

aaronfranke force-pushed the node-set-string-name branch from 2dacd28 to ed0f992 Compare January 6, 2024 10:51

aaronfranke force-pushed the node-set-string-name branch 2 times, most recently from 79feb41 to 9b711b5 Compare January 31, 2024 06:07

aaronfranke force-pushed the node-set-string-name branch from 3e3a3eb to 2d35330 Compare August 29, 2024 12:48

aaronfranke force-pushed the node-set-string-name branch from 2d35330 to ffca5e8 Compare October 2, 2024 23:15

aaronfranke force-pushed the node-set-string-name branch from ffca5e8 to 8aede51 Compare October 31, 2024 03:49

aaronfranke force-pushed the node-set-string-name branch 2 times, most recently from 38dc65d to f727340 Compare December 18, 2024 07:58

aaronfranke force-pushed the node-set-string-name branch from f727340 to 9eda6b3 Compare January 15, 2025 23:30

aaronfranke force-pushed the node-set-string-name branch from 9eda6b3 to 8b0c1ae Compare February 15, 2025 14:46

aaronfranke force-pushed the node-set-string-name branch from 8b0c1ae to 79ca6f4 Compare March 8, 2025 10:56

Ivorforce reviewed Mar 24, 2025

View reviewed changes

core/string/ustring.cpp Outdated Show resolved Hide resolved

Ivorforce approved these changes Mar 27, 2025

View reviewed changes

scene/main/node.cpp Show resolved Hide resolved

scene/main/node.cpp Outdated Show resolved Hide resolved

aaronfranke force-pushed the node-set-string-name branch from 79ca6f4 to 4c942d5 Compare March 27, 2025 22:51

aaronfranke force-pushed the node-set-string-name branch 3 times, most recently from 0a02b62 to 8831bdc Compare April 10, 2025 08:30

aaronfranke force-pushed the node-set-string-name branch from 8831bdc to a7cc734 Compare April 17, 2025 12:52

Calinou approved these changes Apr 17, 2025

View reviewed changes

Repiteo modified the milestones: 4.x, 4.5 May 1, 2025

Change Node set_name to use StringName

a404b66

aaronfranke force-pushed the node-set-string-name branch from a7cc734 to a404b66 Compare May 1, 2025 22:22

Repiteo merged commit acf38b2 into godotengine:master May 2, 2025
20 checks passed

aaronfranke deleted the node-set-string-name branch May 2, 2025 20:28

Bromeon mentioned this pull request May 3, 2025

Adjust test to account for Node::set_name() change (String -> StringName) godot-rust/gdext#1153

Merged

simpkins mentioned this pull request Jul 16, 2025

Renaming a node does not release the old unique name #108683

Closed

simpkins mentioned this pull request Jul 16, 2025

Fix releasing the old unique name when renaming a Node #108684

Merged

Uh oh!

Change Node set_name to use StringName, slightly improves performance #76560

Change Node set_name to use StringName, slightly improves performance #76560

Uh oh!

Conversation

aaronfranke commented Apr 28, 2023 • edited by AThousandShips Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronfranke commented Sep 4, 2023

Uh oh!

dsnopek commented Sep 25, 2023

Uh oh!

RedworkDE left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

raulsntos commented Nov 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ivorforce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ivorforce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Calinou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Benchmark

master

This PR

Uh oh!

Repiteo commented May 1, 2025

Uh oh!

Uh oh!

Repiteo commented May 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Change Node `set_name` to use StringName, slightly improves performance #76560

Change Node `set_name` to use StringName, slightly improves performance #76560

aaronfranke commented Apr 28, 2023 •

edited by AThousandShips

Loading

Ivorforce left a comment •

edited

Loading

Calinou left a comment •

edited

Loading

`master`