Skip to content

Commit

Permalink
fix(core): don't coerce all producers to consumers on liveness change (
Browse files Browse the repository at this point in the history
…#56140)

When a consumer switches its liveness state, it gets added to / removed from
the consumer list of all of its producers. This operation is transitive, so
if its producer is *also* a consumer and *its* liveness state is switched,
then the change is applied recursively.

Note that this only matters *if* the producer is also a consumer. However,
the logic in `producerAddLiveConsumer` / `producerRemoveLiveConsumerAtIndex`
coerced the producer node into a producer & consumer node, which allocated
extra arrays into the node structure that are never used. This didn't affect
correctness, but increased the memory usage of plain signal nodes (which are
just producers, never consumers).

This fix changes the logic in those operations to simply check if a producer
is also a consumer instead of coercing it into one.

PR Close #56140
  • Loading branch information
alxhub authored and thePunderWoman committed May 30, 2024
1 parent 7659dff commit 616cdef
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions packages/core/primitives/signals/src/graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,7 @@ function producerAddLiveConsumer(
indexOfThis: number,
): number {
assertProducerNode(node);
assertConsumerNode(node);
if (node.liveConsumerNode.length === 0) {
if (node.liveConsumerNode.length === 0 && isConsumerNode(node)) {
// When going from 0 to 1 live consumers, we become a live consumer to our producers.
for (let i = 0; i < node.producerNode.length; i++) {
node.producerIndexOfThis[i] = producerAddLiveConsumer(node.producerNode[i], node, i);
Expand All @@ -455,15 +454,14 @@ function producerAddLiveConsumer(
*/
function producerRemoveLiveConsumerAtIndex(node: ReactiveNode, idx: number): void {
assertProducerNode(node);
assertConsumerNode(node);

if (typeof ngDevMode !== 'undefined' && ngDevMode && idx >= node.liveConsumerNode.length) {
throw new Error(
`Assertion error: active consumer index ${idx} is out of bounds of ${node.liveConsumerNode.length} consumers)`,
);
}

if (node.liveConsumerNode.length === 1) {
if (node.liveConsumerNode.length === 1 && isConsumerNode(node)) {
// When removing the last live consumer, we will no longer be live. We need to remove
// ourselves from our producers' tracking (which may cause consumer-producers to lose
// liveness as well).
Expand Down Expand Up @@ -506,3 +504,7 @@ function assertProducerNode(node: ReactiveNode): asserts node is ProducerNode {
node.liveConsumerNode ??= [];
node.liveConsumerIndexOfThis ??= [];
}

function isConsumerNode(node: ReactiveNode): node is ConsumerNode {
return node.producerNode !== undefined;
}

0 comments on commit 616cdef

Please sign in to comment.