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

ResourceLoader: Add thread-aware resource changed mechanism #96593

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

RandomShaper
Copy link
Member

  • ResourceLoader: Simplify handling of unregistered tasks (Preliminary work for the other commit)

The unregistered task mechnaism is used to isolate a uncached load from a cached one with the same path. This commit simplifies how unregistered loads are handled.

That brings the benefit of more code shared between both, which removes the constraint of uncached loads having to complete synchronously.

  • ResourceLoader: Add thread-aware resource changed mechanism

Signals are not thread-safe. Therefore, resource loaders shouldn't deal with them. However, there's a special signal in Resource, changed, which is a common need during resource loading. Fortunately, it has its own API.

This commit lets the ResourceLoader intervene that API during threaded loading so the handling of the changed signal can happen in a safe manner.

The general idea is that during loads, only the resources known to a loader thread participate in the signal. When a load is awaited by another, the awaited one propagates its record of connections to the awaiter, which can happen recursively. The awaited one keeps its records until it dies, so they are available to any other load chaining with it. Finally, when a leaf task is awaited, the connections are migrated to the standard signal mechanism on the main thread.

This replaces the current attempt at thread-safe changed signaling, which wasn't good enough because 1) it tried to achieve safety by deferring the calls within a load thread, which changed the expected flow too much; and 2) it couldn't avoid multiple load threads from dealing with the connection maps of the objects involved, therefore not being as robust as desirable. That said, the mechanism in this PR is much more resilient, but, at the end of the day, signals are not thread-safe in Godot, so it can only guarantee correctness within its realm (resource loading).

The implementation tries to be simple in these ways:

  • Linear search (no hashing). Should be good enough for the relatively small amount of cases expected. In case it's found to be a problem, a hashmap can be used instead.
  • No connection flags implemented. In cases where, e.g., CONNECT_REFERENCE_COUNTED is used, like AnimationNodeBlendTree, or other flags, this implementation will forward them to the standard signal mechanism, but won't honor them during loads.

Fixes #96115.

@RandomShaper RandomShaper added topic:core regression cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 5, 2024
@RandomShaper RandomShaper added this to the 4.4 milestone Sep 5, 2024
@RandomShaper RandomShaper requested a review from a team as a code owner September 5, 2024 08:21
@RandomShaper
Copy link
Member Author

Cherry-pick for 4.3 included in #96606.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 5, 2024
@akien-mga akien-mga added the bug label Sep 5, 2024
@akien-mga akien-mga merged commit 05d9854 into godotengine:master Sep 6, 2024
20 checks passed
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
This is a complement to: godotengine#96593

(cherry picked from commit ab29964)
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
This is a complement to: godotengine#96593

(cherry picked from commit ef365d0)
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 6, 2024
This is a complement to: godotengine#96593

(cherry picked from commit 97197ff)
maidopi-usagi pushed a commit to maidopi-usagi/godot that referenced this pull request Sep 11, 2024
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 13, 2024
This is a complement to: godotengine#96593

(cherry picked from commit 97197ff)
RandomShaper added a commit to RandomShaper/godot that referenced this pull request Sep 23, 2024
This is a complement to: godotengine#96593

(cherry picked from commit 97197ff)
npinsker pushed a commit to npinsker/godot that referenced this pull request Oct 3, 2024
ChrisBase pushed a commit to ChrisBase/godot that referenced this pull request Nov 15, 2024
jss2a98aj pushed a commit to jss2a98aj/blazium that referenced this pull request Nov 18, 2024
This is a complement to: godotengine#96593

(cherry picked from commit 97197ff)
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.

Race condition in Resource::connect_changed
3 participants