Skip to content

Make NodePath always calculate its own hash, to avoid multithreading and performance issues#113480

Open
Ivorforce wants to merge 1 commit into
godotengine:masterfrom
Ivorforce:nodepath-always-hash
Open

Make NodePath always calculate its own hash, to avoid multithreading and performance issues#113480
Ivorforce wants to merge 1 commit into
godotengine:masterfrom
Ivorforce:nodepath-always-hash

Conversation

@Ivorforce
Copy link
Copy Markdown
Member

@Ivorforce Ivorforce commented Dec 3, 2025

This eliminates a theoretical race condition where NodePath hash_cache is read while it is incorrect. I think the race condition may not actually manifest because of hardware details, but this is not guaranteed. I explain this further below.

This change also optimizes NodePath by making hash_cache more available (e.g. for operator==, which can exit early more often).

Explanation

The race condition exists if one thread is calling _update_hash_cache while another is getting the hash (e.g. operator==). In this situation, we have no guarantees that hash_cache_valid is set after hash_cache is set, so there is a race condition where hash_cache can be read with an invalid value.

I see three potential fixes:

  • Make hash_cache atomic or use thread fences (but this would incur a substantial cost on hash access).
  • Assume hash_cache and hash_cache_valid are effectively atomic by aligning them across a 64-bit boundary (they already are aligned right now, but we could make this explicit). This should be safe in most architectures in practice, but it's not guaranteed by the C++ language, and assumes that the compiler uses the 64-bit setters for the values.
  • Always calculate hash_cache (instead of lazily).

Always calculating hash_cache might seem wasteful at first, but calculating the hash cache at the constructors is actually ideal since most of the values should be in cache. This is assuming the hash cache is probably needed sooner or later — for example, nodes themselves are indexed by hash maps (which needs the hash cache), and so are properties.

A side effect benefit is that we can always assume hash_cache exists, which should make other operations (like hash() and operator==) more efficient.

Side note

prepend_period is removed because it is unused. This is better addressed in #113204, but removing it here does eliminate a potential failure case already.

…ultithreading and unforeseen performance issues.
@Ivorforce Ivorforce force-pushed the nodepath-always-hash branch from 6fae62e to 02fc6be Compare December 3, 2025 09:49
@hpvb
Copy link
Copy Markdown
Member

hpvb commented May 6, 2026

This seems good in principle, but it seems that https://github.com/godotengine/godot/blob/master/scene/resources/resource_format_text.cpp#L208 uses prepend_period() so I'm not sure we can just remove it.

It also looks like NodePath::_update_hash_cache() is pretty expensive, particularly

    StringName path = get_concatenated_names();
    StringName subpath = get_concatenated_subnames();

Intuitively I agree with you that NodePath compares are probably more common than creation. But I think this does require some benchmarks. If this isn't true then this might make things overall slower, possibly by quite a bit.

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.

2 participants