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

Adds subscriptions to variables #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

theSherwood
Copy link
Contributor

I added subscribe and unsubscribe methods to Variable. I don't know if it would be better to subscribe to individual variables or en masse to Solver 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

@theSherwood theSherwood marked this pull request as ready for review January 23, 2024 22:50
this._value = value
if (this._callback && previousValue !== value) {
this._callback(value, previousValue)
}
Copy link
Member

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 ...
}

Copy link
Contributor Author

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?

Copy link
Member

@trusktr trusktr Jan 26, 2024

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).

Copy link
Member

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.

Copy link
Contributor Author

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:

  1. batching across updates to different variables
  2. batching across multiple updates to a single variable across time
  3. 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.

Copy link
Contributor Author

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:

  1. 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-tracking createEffect, maybe it is best to just make the variables easily consumable as signals by Solidjs and the rest of the gang.
  2. 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?

@trusktr trusktr linked an issue Jan 25, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] incremental update notification
2 participants