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

Watcher simplification #222

Closed
dead-claudia opened this issue May 15, 2024 · 12 comments
Closed

Watcher simplification #222

dead-claudia opened this issue May 15, 2024 · 12 comments

Comments

@dead-claudia
Copy link
Contributor

dead-claudia commented May 15, 2024

Have you considered one-shot notification for watchers, pushing the managed Watcher object itself to userland?

I'm thinking something like this: a promise that resolves as soon as a signal's made dirty.

  • On each signal, let there be a "tracked" flag (initially clear) and a list of watching promise resolvers.
    • The flag ensures untrack isn't called prematurely.
  • promise = Signal.subtle.waitForUpdate(signal, token?) resolves as soon as the signal becomes outdated again.
    • If the promise resolver list was previously zero and the "track" flag is clear, the flag should be set then the track hook called synchronously. If it throws, the error is reported similarly to uncaught rejections.
    • Watcher promises are resolved after the equals option is called in State#set (as equals-ish doesn't dirty it), but before everything in Computed#get.
    • The optional token is whatever your favorite cancellation object is. If it cancels the last promise before it resolves, the same pre-resolve job above is executed immediately without scheduling, and the resolvers are dropped.
  • On signal.get(), if the promise resolver list is empty and the "track" flag is set, the flag should be cleared then the untrack hook called synchronously. This needs done after everything else, to allow cells to re-watch inside their bodies (and avoid needless untrack/track calls).
  • signal.isTracked returns the value of that "track" flag.
  • Signal.subtle.stopTrackingUpdates(signal) drops all promises, then does the same final cleanup that signal.get() does.

I'm pulling a page out of embedded and low-level systems development with this one. Also, one could think of it as similar in spirit to Atomics.waitAsync.

Here's my line of thinking:

  1. Synchronous subscription avoids race conditions with notification.
  2. Asynchronous delivery avoids the data corruption issue inherent in watchers.
  3. One-shot notification much more easily avoids duplicate notifications, and fits into async/await much better.
  4. One-shot subscriptions also allows for optimizations like if (!signal.isDirty) await Signal.subtle.waitForUpdate(signal); schedule(doGet) where you entirely avoid waiting at all.
  5. Engines can use iterable weak sets of promise states internally to much more efficiently ensure objects are properly cleaned up. This weak link also would allow promise states to be collected if the signal becomes unreachable.
This was referenced May 16, 2024
@dead-claudia
Copy link
Contributor Author

Here's how sync effects could be implemented using this and the DOM's AbortSignal as a token, for reference:

function effect(token, body) {
    if (token.aborted) return
    const signal = new Signal.Computed(body)
    Promise.resolve().then(function loop() {
        if (token.aborted) return
        Signal.subtle.waitForUpdate(signal, token).then(loop)
        signal.get()
    })
}

@EisenbergEffect
Copy link
Collaborator

I find this pretty intriguing. If the performance were as good as the watcher approach, this would be simpler to manage.

@littledan @alxhub What do you all think of this idea?

@dead-claudia
Copy link
Contributor Author

dead-claudia commented May 19, 2024

Edit: clarity, hide long quote

Will note that this algorithm also gives a way to solve a concern I highlighted in #195 (comment) about the existing signal.{,un}watch(signal) API.

Important bits copied here

Being able to batch calls to watch and unwatch is very useful, and I think we do want to figure out a good mechanism for that. One option would be to defer them until you're outside of the outermost Computed's .get, which seems reasonable as a baseline and (maybe less reasonably) lets you use a dummy computed with an untrack inside to force batching. Watcher track as a way to get batching seems just as reasonable.

It sounds reasonable on the surface, but it can cause abstractions to inexplicably break. Consider this situation, where one watcher watches a signal, but that same signal is also accessed by a parent computed watched by a different watcher:

  • In a framework, you watch a computed and invoke its .get().
  • You also limit a framework function to only be called during the scope of that .get(). Calling it during watch/unwatch is documented to be fine.
  • A component library your user uses happens to rely on that framework function being callable during watch/unwatch. It also does animations, and so while that's playing, it needs to update signals a lot.
  • The end developer, using two different frameworks, is calling the function to mount the component inside the signal handler of another library.

Now, you have two problems:

  1. The component library starts inexplicably throwing. The end developer tracks it down pretty easily to the library and files a bug report against it. That library's framework documents the throwing function as safe in that given instant, so the library developer files a bug against the framework. The framework developer can't reproduce the issue since they don't know about the integration issue, and so they close it. End developer has no idea it's actually the other framework causing it, and is now raging about the whole situation on Twitter/X.
  2. During animations, the library component sets an inner signal's value. This causes the parent framework to recompute the tree. The tree is the same as before, since nothing functionally changed, but it takes long enough to cause occasional but frequent stutters on mobile. The end developer tracks this down, profiles their app, and is utterly baffled that their framework's even rendering at that time. They don't know about Signal.subtle.untrack or the fact the inner component's using signals, so they spend a few days straight, only to give up on figuring out what's causing the stutters.

You need a clean boundary between watcher and parent computed, and the current proposed .watch(...) API doesn't afford that.

With this suggestion, to ensure proper subscribe/unsubscribe batching across multiple disjoint signals, you'd just do the following:

  1. Run and do all your Signal.subtle.waitForUpdate(signal, token)s first on new signals and collecting existing signals. This can happen at any time before the next step.
  2. Cancel the existing pending signal watcher promises.

@dead-claudia
Copy link
Contributor Author

Oops, forgot to cite specific precedent here to explain my "I'm pulling a page out of embedded and low-level systems development with this one" statement in my initial issue comment:

  • POSIX poll is basically this for file descriptors, except that it monitors multiple at once.
  • ARM's wfi and wfe instructions are similar in spirit, telling the thread of execution to wait. Other CPUs almost always have equivalent functionality, like x86 hlt and RISC-V wfi.
    • Those instructions do not guarantee that the next poll of the updated state will return anything different. It's just merely a hint that it may have changed. This is precisely the same behavior my proposed Signal.subtle.waitForUpdate would have (and what Watchers have in the current proposal spec).
  • Rust's streams use a strikingly similar model at the lowest level:
    • stream.poll_next(&mut context) returns Poll::Ready(Some(value)) on new value, Poll::Pending if waiting for an update, and (inapplicable to this proposal) Poll::Ready(None) on end. It's valid to return Poll::Ready(Some(value)) after Poll::Pending, unlike for futures' poll method (where Poll::Ready is only valid the first time).
    • Instead of a promise or future resolution, waker.wake{,_by_ref}() from a mid-poll context.waker() notifies of a potential need to re-poll. They can be single-shot, but aren't always.
  • Edge-triggered, one-shot, single-source futures like this are everywhere in the Tokio ecosystem, from intervals to file descriptor readability to channel receiver loss detection to even client connection shutdown (in the case of Hyper).

@dead-claudia
Copy link
Contributor Author

As for performance, I don't have benchmarks, but suspect it to be margin of error.

It does preclude refreshing inside the same job (required for same-animation-frame scheduling), but I doubt it'd be a problem in practice when you can just check if it's dirty again to know right away, and re-getting the value synchronously will cause the queued Signal.subtle.untrack callback to get called synchronously, even if it didn't change.

@justinfagnani
Copy link
Collaborator

justinfagnani commented May 21, 2024

I think the forced asynchronous nature of this API would be a problem for some use cases. Right now you can respond to changes sooner than a microtask as long as the notification callback has returned.

I also worry about the number of Promise allocations in cases where you have a lot of signals that you want to watch continuously and independently. Watchers are at least persistent already, and I'm hoping that we could consolidate watchers if we get notifiedBy.

I wonder how many watcher use cases are even one-shot? Is that the exception?

@dead-claudia
Copy link
Contributor Author

I think the forced asynchronous nature of this API would be a problem for some use cases. Right now you can respond to changes sooner than a microtask as long as the notification callback has returned.

@justinfagnani I did list one case where it could bring a material limitation, but I doubt it'd be much of one in practice - it's bad practice to do same-frame re-rendering like that in most cases anyways and most frameworks just don't support it.

Did you have any others in mind? I'd love to hear. (I'm always happy to be proven wrong about my intuition.)

I also worry about the number of Promise allocations in cases where you have a lot of signals that you want to watch continuously and independently. Watchers are at least persistent already, and I'm hoping that we could consolidate watchers if we get notifiedBy.

I considered a Watcher extends Promise class partly for this reason. I also suggested an "is dirty" bit in #178 independently, and that kind of thing could just as easily go both signals and watcher objects

I wonder how many watcher use cases are even one-shot? Is that the exception?

Most aren't, but IMHO it's a bit easier to reason about the state truly needed if you at least start from the one-shot angle (even if you don't end there).

@alxhub
Copy link
Collaborator

alxhub commented May 22, 2024

I think the forced asynchronous nature of this API would be a problem for some use cases. Right now you can respond to changes sooner than a microtask as long as the notification callback has returned.

@justinfagnani I did list one case where it could bring a material limitation, but I doubt it'd be much of one in practice - it's bad practice to do same-frame re-rendering like that in most cases anyways and most frameworks just don't support it.

Did you have any others in mind? I'd love to hear. (I'm always happy to be proven wrong about my intuition.)

Angular depends on the faster-than-microtask / semi-synchronous watch behavior as well. In general:

  • Refresh of the UI is sync (a tick() call synchronously walks the UI hierarchy and refreshes dirty components)
  • Refreshing one component might set signals (props) which dirty child components (effects)
  • We need to receive those notifications synchronously in order for those child components to be properly marked dirty when the traversal reaches them.

@dead-claudia
Copy link
Contributor Author

dead-claudia commented May 22, 2024

@alxhub Would a computed.isDirty check after computed.get() suffice? Because that's what I've been thinking could provide just enough of an escape hatch.

Edit: or a watcherPromise.isResolved or similar. (If promises allowed synchronous inspection, that'd of course also be equivalent.)

Edit 2: used this as justification to (again) push for synchronous promose state inspection: https://es.discourse.group/t/synchronous-promise-inspection/2037

@alxhub
Copy link
Collaborator

alxhub commented May 22, 2024

No, computed.isDirty isn't sufficient. On notification, we mark our tree structure with flags to indicate which subtrees contain components/effects which are dirty. That allows us to efficiently run dirty effects in order, while skipping over parts of the tree which aren't. Needing to poll the dirtiness of every effect in the traversal without any tree pruning would lose this important optimization.

@dead-claudia
Copy link
Contributor Author

dead-claudia commented May 23, 2024

Okay, then on that note, I'll rescind my signal.watch(...) suggestion.

So, the current design plus my evaluateAndMarkReady from #195 (comment) that, together, is roughly this is best:

class Watcher {
    constructor(notify) {
        this._notify = notify
        this._phase = "ready"
        this._signals = new Set()
        this._newlyAdded = new Set()
        this._newlyRemoved = new Set()
    }

    watch(signal) {
        if (this._phase === "notify") throw new TypeError()
        if (this._signals.has(signal)) return
        this._signals.add(signal)
        this._newlyAdded.add(signal)
        this._newlyRemoved.delete(signal)
        maybeAddWatchedParentToChildren(signal)
    }

    unwatch(signal) {
        if (this._phase === "notify") throw new TypeError()
        if (this._signals.has(signal)) return
        this._signals.add(signal)
        this._newlyAdded.delete(signal)
        this._newlyRemoved.add(signal)
        maybeRemoveWatchedParentFromChildren(signal)
    }

    evaluateAndMarkReady() {
        if (this._phase === "notify") throw new TypeError()
        const added = this._newlyAdded
        const removed = this._newlyRemoved
        this._newlyAdded = new Set()
        this._newlyRemoved = new Set()
        this._phase = "ready"
        for (const signal of added) signal.options?.tracked?.()
        for (const signal of removed) signal.options?.untracked?.()
    }
}

function notifyWatcherOfSignalSet(watcher) {
    if (watcher._phase !== "ready") return
    watcher._phase = "notify"
    try {
        watcher._notify()
    } finally {
        watcher._phase = "pending"
    }
}

@dead-claudia
Copy link
Contributor Author

I'll go ahead and close this. My concern was addressed.

@dead-claudia dead-claudia closed this as not planned Won't fix, can't repro, duplicate, stale Jun 3, 2024
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

No branches or pull requests

4 participants