proposal-signals icon indicating copy to clipboard operation
proposal-signals copied to clipboard

Consumers should be held weakly to allow all nodes to be live

Open DavidANeil opened this issue 10 months ago • 8 comments

(This issue summarizes a discussion from the community Zoom call, to create a public record for any design decisions made in this space.)

The current design only holds reverse dependencies for the purpose of stale signal propagation on nodes that are being watched/live. Otherwise the "stale" state of a computed is determined by transitively checking the entire graph, which is O(n) on the number of nodes that are transitively depended on. (There is an optimization where this can be short-circuited if no nodes in any graph in the entire system have been marked stale since the last read, but that isn't helpful in most real applications).

This is a performance foot-gun, where a Signal can have drastically different performance characteristics depending on if some other code is making it live or not.

The proposed alternative design that avoids the issue would be to use WeakRef (in JS parlance) to always store the consumers (reverse dependencies) of a node. This allows for constant time checking if any node is stale, by eagerly propagating the dirty flag along all the weak edges.

There seems to be some suspicion of using WeakRef from certain individuals. I'm not totally certain what the concern is, but I believe it is a vague sense of "WeakRef looks nice on benchmarks, but those don't take into account the extra pressure put on the garbage collector by their use". I would love for any individuals with more information in that regard to contribute here to the discussion.

If that concern is true, then we also need to determine if the browser-native equivalent of WeakRef also causes the same issue.

DavidANeil avatar Jan 22 '25 20:01 DavidANeil

In my opinion Signal.subtle.unwatched and Signal.subtle.watched would be invoked when the number of consumers moves to/away from 0, regardless of the type of that consumer (Watcher or a read of a Computed)

DavidANeil avatar Jan 22 '25 22:01 DavidANeil

There seems to be some suspicion of using WeakRef from certain individuals. I'm not totally certain what the concern is, but I believe it is a vague sense of "WeakRef looks nice on benchmarks, but those don't take into account the extra pressure put on the garbage collector by their use". I would love for any individuals with more information in that regard to contribute here to the discussion.

Actually, WeakRef looks terrible in benchmarks. It's incredibly slow to both create and to .deref(). We did exactly this in Angular Signals, and abandoned it because of the poor performance.

alxhub avatar Jan 22 '25 22:01 alxhub

That said, there is a similar design which I think is quite promising: use a strong reference from producer to consumer but use FinalizationRegistry to clean up those references once those consumers are dropped. This is a bit tricky to set up, but from my initial testing FinalizationRegistry has promising performance characteristics.

alxhub avatar Jan 22 '25 22:01 alxhub

Actually, WeakRef looks terrible in benchmarks.

IIRC, Angular used it for both producers and consumers. I remember benchmarking that implementation with only using WeakRef for consumers and had almost comparable performance to a version with no WeakRef.

DavidANeil avatar Jan 23 '25 01:01 DavidANeil

use FinalizationRegistry to clean up those references once those consumers are dropped.

I would love to learn more about this, because on paper it seems like holding the strong references would prevent it from being garbage collected, and hence FinalizationRegistry would be impotent.

DavidANeil avatar Jan 23 '25 01:01 DavidANeil

I would love to learn more about this, because on paper it seems like holding the strong references would prevent it from being garbage collected, and hence FinalizationRegistry would be impotent.

It's a little more challenging with the TC39 design since the reactive node and the user-facing signal object are the same entity.

In Angular signals, a computed signal is a "getter function" which closes over the reactive node object and interacts with it to produce a value. Only the user's code ever references the getter function, the reactive graph pointers are always from node to node.

We could use FinalizationRegistry to watch for disposal of the getter function. Once a user no longer holds a reference to the getter, we can clean up the node object.

Sometimes an orphan node object might still have consumers which previously read the computed and are still alive. If this is the case, then we can add the dependencies of the computed node as dependencies of the computed node's consumers directly. This would look like A <- B <- C where C depends on B which depends on A. If B is orphaned (no longer readable), we cut it out of this chain and record A <- C directly, effectively merging B's dependencies into C. This allows us to free the B node.

alxhub avatar Jan 23 '25 01:01 alxhub

In my opinion Signal.subtle.unwatched and Signal.subtle.watched would be invoked when the number of consumers moves to/away from 0, regardless of the type of that consumer (Watcher or a read of a Computed)

I agree, cf #227

However, if (unlike #227) all nodes are held live (with a WeakRef), it means Signal.subtle.unwatched would be invoked when the garbage collector collects some nodes, which makes the timing of the call non-deterministic, which I don't like at all.

divdavem avatar Jan 23 '25 08:01 divdavem

FYI @alxhub I attempted to implement a FinalizationRegistry version of Signals, and it breaks down in the following case:

export const externalState = new Signal.State(1);
class Foo {
    public readonly internalState = new Signal.State(2);
    public readonly myComputed = new Signal.Computed(() => this.internalState.get() + externalState.get());
}
function shouldDropFooAfterCompletion() {
    return new Foo().myComputed.get();
}
shouldDropFooAfterCompletion();

externalState[InternalNode] would have a reference to its consumer myComputed[InternalNode], which has a reference to its calculate callback, which has a reference to the this (Foo) in its context, which has a reference to myComputed (external node). So now as long as externalState is held in memory, then myComputed will be held as well. So if we had a FinalizationRegistry trying to clean up myComputed[InternalNode] if myComputed is collected, it would be stuck waiting forever.

DavidANeil avatar Nov 14 '25 17:11 DavidANeil