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

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

Open
DavidANeil opened this issue Jan 22, 2025 · 7 comments
Open

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

DavidANeil opened this issue Jan 22, 2025 · 7 comments

Comments

@DavidANeil
Copy link

(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
Copy link
Author

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)

@alxhub
Copy link
Collaborator

alxhub commented Jan 22, 2025

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
Copy link
Collaborator

alxhub commented Jan 22, 2025

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.

@DavidANeil
Copy link
Author

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
Copy link
Author

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.

@alxhub
Copy link
Collaborator

alxhub commented Jan 23, 2025

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.

@divdavem
Copy link

divdavem commented Jan 23, 2025

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.

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

3 participants