-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adds subscriptions to variables #8
base: main
Are you sure you want to change the base?
Conversation
this._value = value | ||
if (this._callback && previousValue !== value) { | ||
this._callback(value, previousValue) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea! If you need it to do what you need more easily, let's get it in. I believe its good to at least have the subscribe on the Variables, and having a subscribe for all variables of a Solver would be a bonus. But if this works for you, let's get this in first, and if you need the Solver subscription, we can add that subsequently.
It seems that we should probably have a list of callbacks, in case multiple processes want to subscribe. Then subscribe
would return an unsubscribe
function that removes the callback from the list.
Another thing is synchronous data notifications can cause performance issues and impartial state errors (f.e. derived values could be incorrect at mid-calculation and we would not want to observe those values mid-way, and reactions may also happen too many times if a variable needs to be updated multiple times to complete a calculation). A simple way we can mitigate these issues is to make reactivity deferred, typically with a microtask, f.e. with queueMicrotask
:
if (!this._queued) {
queueMicrotask(this._runCallbacks)
this._queued = true
}
.....
private _runCallbacks() {
this._queued = false
... run each callback ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe its good to at least have the subscribe on the Variables, and having a subscribe for all variables of a Solver would be a bonus.
Can you explain what you mean a little more by "all variables of a Solver"?
It seems that we should probably have a list of callbacks, in case multiple processes want to subscribe. Then subscribe would return an unsubscribe function that removes the callback from the list.
I'm happy to make this change.
A simple way we can mitigate these issues is to make reactivity deferred, typically with a microtask, f.e. with queueMicrotask
Any objections to not using microtasks but instead flushing the queue at the end of the call to Solver.updateVariables
? That seems like it would solve the perf and impartial state errors disappear. And it still keeps things synchronous.
Another alternative is to provide a flushSubscriptionQueue
method to the solver so that users have control over when subscriptions will run. That way, if they want to queue a microtask, or perform multiple updates without the subscriptions running, they can do that:
solver.updateVariables()
/* ... user code that reads some variables and conditionally updates a few more ... */
solver.updateVariables()
solver.flushSubscriptionQueue() // all subscription callbacks have been called
/* ... more user code that depends on all the subscription callbacks to have been called ... */
of
queueMicrotask(solver.flushSubscriptionQueue()
solver.updateVariables()
/* ... some more code ... */
solver.updateVariables()
/* ... subscription callbacks are called at the end of the current task */
I've recently been dealing with some of the ramifications of not having control over when reactive systems update and it seems like it would be easy enough to head off. Thoughts on this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what you mean a little more by "all variables of a Solver"?
I was simply referring to your second idea of subscribing "en masse to Solver
and get a list of changed variables". I think I lean more towards deriving state on a per value basis, but maybe someone could also want to react to all vars in a single callback for differing patterns.
I've recently been dealing with some of the ramifications of not having control over when reactive systems update
I find that microtasks are ideal most of the time, and in those systems I just embrace it. For example, web's ResizeObserver
fires on animation frames, and MutationObserver
fires on microtasks, both of them naturally avoiding too much work.
Out of curiosity, what issue would you want to avoid with synchronous updates?
What if the subscription can choose? For example, it could default to microtasks for avoiding too much worm by default, but an option could make it happen on solver updates:
const unsub = someVar.subscribe(() => {...}, {microtask: false}) // or similar
I think it would be great if we were working with signals and effects, but that would be a bit of a change to the existing implementation:
createEffect(() => {
// This reruns any time either var1 or var2 change
console.log(var1.value, var2.value)
})
With the subscription pattern on Solver
, it would get easier to write single functions that react to multiple variables like with that createEffect
example, whereas subscribing to multiple variables individually to log them at the same time becomes more verbose, and also more reactive-glitch-prone (even on microtasks because of the iterative execution of each subscription). Effects basically give us the best of both, without glitches (on microtasks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if this helps you get moving as is, happy to just merge it like this, and we can update the implementation a little later while this one is marked experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what issue would you want to avoid with synchronous updates?
Microtask updates are generally pretty great if what needs to react is just the render/ui. But if you have business logic that needs to update, I find it much simpler to know when that is going to happen/be completed. My current project makes heavy use of that.
What if the subscription can choose?
This seems more likely to create reactive glitches to me.
With the subscription pattern on Solver, it would get easier to write single functions that react to multiple variables like with that createEffect example
This is true. I think a subscription on the solver would make for a nice solution. Or maybe just passing a flag to updateVariables
to return an array of the changed variables. Though that approach is also somewhat problematic for other reasons, not least of which is that Autolayout makes its own calls to updateVariables
within SubView
with no simple way to get those values back to the user. So that option is probably off the table. Whatever is done needs to make things easier for Autolayout, I should think, since that will be a principle use-case of Kiwi.
To my mind, we are talking about 3 problems that are related but not the same thing:
- batching across updates to different variables
- batching across multiple updates to a single variable across time
- synchronicity control
createEffect
gets us 1 and 2.
The discussed approach to queueMicrotask
gets us 2.
The current implementation of this PR gets us really none of the above automatically but all them can be built atop it at a performance cost. I think you're right that it's too costly.
solver.flushSubscriptionQueue
in conjunction with individual subscriptions to variables gets us 2 and 3 and a peculiar version of 1, in that it batches across ALL updated variables but doesn't have the specificity of createEffect
and the subscriptions are called separately.
I think there's a solution in the direction of a some kind of flush
or batch
primitives that get us 1, 2, and 3. I'll have another think on it.
Another thing to consider is that if we rely on queueMicrotask
for 2, how will that affect porting to assemblyscript?
Also, if this helps you get moving as is, happy to just merge it like this, and we can update the implementation a little later while this one is marked experimental.
I appreciate it. I am not currently in a big rush. But I'll let you know if anything changes in the next few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a little more, I've noticed 2 things:
createEffect
(at least the kind with auto-tracking) runs on top of other observables. So we would still need individual subscriptions of some kind on each variable. (Unless we decided not to use auto-tracking and simply passed in a list of variables as a dependency list). If we want auto-trackingcreateEffect
, maybe it is best to just make the variables easily consumable as signals by Solidjs and the rest of the gang.- There's an important difference between "I want to watch these variables" and "I want to know what changed". You can get "I want to know what changed" by way of "I want to watch these variables", but only by watching every single variable, and even then, it's not very ergonomic. It might be best to consider these as 2 different approaches. And in the case of Kiwi, I'm inclined to think that knowing what changed is more important than watching some subset of variables. So do we want one or the other? Or do we want both?
I've also realized that I have been making some false assumptions about the algorithm. I had made the mistake of thinking that updateVariables
was where the work of the algorithm is happening. But it isn't. It's just updating the variables by taking the values from the rows. The real work (the simplex solver?) is happening eagerly in _optimize
and _dualOptimize
every time a variable or constraint is added or removed, or when a value is suggested for a variable. I think the api is a little misleading in that regard. The user needs to call updateVariables
before reading from the variables, but all it does is take the answers it calculated during addConstraint
and pass them over to the Variable
interface where the user can then look for them. So it's running the potentially expensive calculations (usually polynomial but exponential in the worst case?) during every addConstraint
but it's bothering to batch the update to the variables until it's called for, even though that process has linear time complexity. Maybe the linear updateVariables
dominates the incremental simplex solver in practice. But it does look to me like there shouldn't be any thrashing on the variables in practice, as updateVariables
sets each variable exactly once. That seems to suggest that we don't really need to batch variable updates over time. Thoughts?
I added
subscribe
andunsubscribe
methods toVariable
. I don't know if it would be better to subscribe to individual variables or en masse toSolver
and get a list of changed variables. But this was straightforward. Thoughts?For context, here's the link to an issue I opened about it: #6