Defer change callbacks to a worker thread #212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change fundamentally changes how change callbacks work on Dynamics. Prior to this change, callbacks executed on the thread that was performing the change. This could lead to situations where multiple threads were executing callback chains which leads to unpredictable locking patterns on the dynamics. The basic deadlock detection was not enough. This change defers callbacks to a single callback thread. This thread ensures that no dynamic can have callbacks enqueued more than once. By limiting execution to one set of callbacks at any given time, this greatly reduces the surface for locks to contend with each other. The next issue was how tuple-related functions like for_each/map_each were acquiring their locks. By calling a.read() then b.read(), this was causing a to be held in a locked state while b was being aquired. If users are careful to always acquire their locks in this order, everything is fine. But with Cushy there can be unexpected situations where these locks are being held. This change also refactors lock acquisition for tuples to try to acquire all the locks in a non-blocking way. If any lock woould block, the initial locks are dropped while the lock that would block is waited on. After this is acquired the process starts over again to gain all the locks. This isn't perfect, but it doesn't require unsafe. With unsafe, we could in theory create a ring of callbacks that handles acquiring all of the locks into MaybeUninits. Upon successfully calling all callbacks, the values can be assumed init. But writing all of this in macro_rules isn't fun, and the current solution alleviates the main problem
Currently only at the trace level, this diagnostic can be helpeful in identifying if there are any callback cycles that are causing repeated firings.
At some point the old steps started failing, this seems to be the new set of packages the wgpu project is using.
This reverts commit 31883e5.
One of the fallouts of executing callbacks in a deferred state is that the locks no longer prevent cyclic updates. For well defined values, this works perfectly. But for something like floating points, this can cause not-quite-the-same values to continue to propagate until they stabilize. The 7guis-temperature-converter, for example, would sometimes replace the currently editing text with an imprecise float result computed from a roundtrip temperature conversion. This change adds tracking to these deferred callbacks that ensures for each "root" invocation of a dynamic's callbacks, all affected change callbacks are invoked a single time in any given execution chain, taking care to support divergent chains causing an individual set of callbacks to be invoked more than one time as long as their individiaul call chains have not yet invoked the callbacks.
The last change had a few expects fail in some examples. The issue was caused by inserting the node incorrectly into the linked list.
The weak_clones are no longer necessary with the new change callback model
Also fixed merge artifact -- the example moved but was restored during a conflict.
|
It appears this is causing issues in my app after updating to a version of cushy with this PR. More details + a video in discord, here: https://discord.com/channels/578968877866811403/842093272540643339/1328796671638765641 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fundamentally changes how change callbacks work on Dynamics. Prior to this change, callbacks executed on the thread that was performing the change. This could lead to situations where multiple threads were executing callback chains which leads to unpredictable locking patterns on the dynamics. The basic deadlock detection was not enough.
This change defers callbacks to a single callback thread. This thread ensures that no dynamic can have callbacks enqueued more than once. By limiting execution to one set of callbacks at any given time, this greatly reduces the surface for locks to contend with each other.
The next issue was how tuple-related functions like for_each/map_each were acquiring their locks. By calling a.read() then b.read(), this was causing a to be held in a locked state while b was being aquired. If users are careful to always acquire their locks in this order, everything is fine. But with Cushy there can be unexpected situations where these locks are being held.
This change also refactors lock acquisition for tuples to try to acquire all the locks in a non-blocking way. If any lock woould block, the initial locks are dropped while the lock that would block is waited on. After this is acquired the process starts over again to gain all the locks. This isn't perfect, but it doesn't require unsafe. With unsafe, we could in theory create a ring of callbacks that handles acquiring all of the locks into MaybeUninits. Upon successfully calling all callbacks, the values can be assumed init. But writing all of this in macro_rules isn't fun, and the current solution alleviates the main problem