X Tutup
Skip to content

Optimize NodePath to String by using cached path#110478

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
beicause:opt-node-path-to-string
Oct 23, 2025
Merged

Optimize NodePath to String by using cached path#110478
Repiteo merged 1 commit intogodotengine:masterfrom
beicause:opt-node-path-to-string

Conversation

@beicause
Copy link
Contributor

@beicause beicause commented Sep 13, 2025

Found this problem when I'm working on #110439 (This code doesn't consider absolute path, which maybe wrong):

godot/scene/resources/animation.cpp

Line 1067 in bfa330d

tracks[p_track]->thash = StringName(String(track_path.get_concatenated_names()) + String(track_path.get_concatenated_subnames()) + itos(track_cache_type)).hash();

We can optimize NodePath to String by using the cached concatenated path.

Tested in gdscript

	var t1 := Time.get_ticks_msec()
	var p := NodePath("gsesfs/dawd/fesj/wadfr:dawjjfes:fnsnjk:fasef")
	var s := str(p)
	print(s)
	for i in range(1000000):
		s = str(p)
	print(Time.get_ticks_msec()-t1)

Before: 702, After: 373

@beicause beicause requested a review from a team as a code owner September 13, 2025 01:27
Ivorforce
Ivorforce approved these changes Sep 13, 2025
View reviewed changes
Copy link
Member

@Ivorforce Ivorforce left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense to me. We have caches, so we should be using them.
Note that I expect this to be slower on the first conversion to String, but I think that's an acceptable trade-off. We can optimize the get_concatenated_names and get_concatenated_subnames functions in a future PR (e.g. with reserve).

@Ivorforce Ivorforce added this to the 4.6 milestone Sep 13, 2025
@beicause beicause force-pushed the opt-node-path-to-string branch from b2d7d85 to d16413a Compare September 13, 2025 09:10
beicause
beicause commented Sep 13, 2025
View reviewed changes
core/string/node_path.cpp
void NodePath::prepend_period() {
if (data->path.size() && data->path[0].operator String() != ".") {
data->path.insert(0, ".");
data->concatenated_path = StringName();
Copy link
Contributor Author

@beicause beicause Sep 13, 2025

Choose a reason for hiding this comment

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

I'm surprised we didn't clear the cache when modifying the NodePath. Perhaps we don't have cases where NodePath is modified after calling get_concatenated_names, but this is a potential bug.

Copy link
Contributor

@Maran23 Maran23 Sep 13, 2025

Choose a reason for hiding this comment

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

Might be good to write a test case for that as well, NodePath should be easy to test

@fire fire changed the title Optimize NodePath to String by using cahced path Optimize NodePath to String by using cached path Sep 13, 2025
@Repiteo Repiteo merged commit 06bdfa5 into godotengine:master Oct 23, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 23, 2025

Thanks!

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

Reviewers

@Ivorforce Ivorforce Ivorforce approved these changes

+1 more reviewer

@Maran23 Maran23 Maran23 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement performance topic:core

Projects

None yet

Milestone

4.6

Development

Successfully merging this pull request may close these issues.

4 participants

@beicause @Repiteo @Ivorforce @Maran23
X Tutup