perf(core): detect existing signal dependency without checking all producer links

This commit addresses a scaling issue in the signal dependency graph where
the detection of duplicate dependency links would perform a linear scan across
all consumer links of all producers. The linear scan is replaced with a version
comparison of the dependency edge against the current epoch; if they are
equal the existing dependency edge is known to be valid in this epoch. This means
that the link won't be eligible for removal and therefore doesn't have to be
recreated.

Author: JoostK

packages/core/primitives/signals/src/graph.ts

@@ -82,6 +82,13 @@ export const REACTIVE_NODE: ReactiveNode = {
 interface ReactiveLink {
   producer: ReactiveNode;
   consumer: ReactiveNode;
+
+  /**
+   * Stores the epoch that holds when this link was observed, allowing subsequent observations of the same producer to
+   * realize that there's an existing link, avoiding the creation of a new, redundant link. A value of `null` indicates
+   * that the link cannot be assumed to be valid based on the epoch counter.
+   */
+  knownValidAtEpoch: Version | null;
   lastReadVersion: number;
   prevConsumer: ReactiveLink | undefined;
   nextConsumer: ReactiveLink | undefined;
@@ -240,27 +247,26 @@ export function producerAccessed(node: ReactiveNode): void {
       // last read version, update the tail of the producers list of this rerun, and return.
       activeConsumer.producersTail = nextProducerLink;
       nextProducerLink.lastReadVersion = node.version;
+      nextProducerLink.knownValidAtEpoch = epoch;
       return;
     }
   }
 
   const prevConsumerLink = node.consumersTail;
 
   // If the producer we're accessing already has a link to this consumer, we can skip adding a new
-  // link. This can short circuit the creation of a new link in the case where the consumer reads alternating ReeactiveNodes
+  // link. This can short circuit the creation of a new link in the case where the consumer reads alternating ReactiveNodes
   if (
     prevConsumerLink !== undefined &&
     prevConsumerLink.consumer === activeConsumer &&
-    // However, we have to make sure that the link we've discovered isn't from a node that is incrementally rebuilding its producer list
-    (!isRecomputing || isValidLink(prevConsumerLink, activeConsumer))
+    (!isRecomputing || prevConsumerLink.knownValidAtEpoch === epoch)
   ) {
-    // If we found an existing link to the consumer we can just return.
     return;
   }
 
   // If we got here, it means that we need to create a new link between the producer and the consumer.
   const isLive = consumerIsLive(activeConsumer);
-  const newLink = {
+  const newLink: ReactiveLink = {
     producer: node,
     consumer: activeConsumer,
     // instead of eagerly destroying the previous link, we delay until we've finished recomputing
@@ -271,6 +277,7 @@ export function producerAccessed(node: ReactiveNode): void {
     // the link is actually inserted. Setting it eagerly would create a dangling
     // reference into the consumer list that prevents GC of removed entries.
     prevConsumer: undefined,
+    knownValidAtEpoch: epoch,
     lastReadVersion: node.version,
     nextConsumer: undefined,
   };
@@ -393,6 +400,18 @@ export function consumerBeforeComputation(node: ReactiveNode | null): ReactiveNo
  * `consumerBeforeComputation` instead of calling this directly.
  */
 export function resetConsumerBeforeComputation(node: ReactiveNode): void {
+  // Clear link validity state before running a computation, such that links that were captured in a prior computation
+  // (which may have happened in the same epoch) are not mistakenly considered valid. This is only necessary if any of
+  // the links has `knownValidAtEpoch` equal to the current epoch, for which the producer that was accessed last is used
+  // as proxy: any earlier producers cannot exceed its epoch.
+  if (node.producersTail?.knownValidAtEpoch === epoch) {
+    let producer = node.producers;
+    while (producer !== undefined) {
+      producer.knownValidAtEpoch = null;
+      producer = producer.nextProducer;
+    }
+  }
+
   node.producersTail = undefined;
   node.recomputing = true;
 }
@@ -562,22 +581,3 @@ export function setPostProducerCreatedFn(fn: ReactiveHookFn | null): ReactiveHoo
   postProducerCreatedFn = fn;
   return prev;
 }
-
-// While a ReactiveNode is recomputing, it may not have destroyed previous links
-// This allows us to check if a given link will be destroyed by a reactivenode if it were to finish running immediately without accesing any more producers
-function isValidLink(checkLink: ReactiveLink, consumer: ReactiveNode): boolean {
-  const producersTail = consumer.producersTail;
-  if (producersTail !== undefined) {
-    let link = consumer.producers!;
-    do {
-      if (link === checkLink) {
-        return true;
-      }
-      if (link === producersTail) {
-        break;
-      }
-      link = link.nextProducer!;
-    } while (link !== undefined);
-  }
-  return false;
-}

packages/core/test/bundling/create_component/bundle.golden_symbols.json

@@ -546,7 +546,6 @@
       "isSubscription",
       "isTemplateNode",
       "isTypeProvider",
-      "isValidLink",
       "isValueProvider",
       "lastNodeWasCreated",
       "lastSelectedElementIdx",

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

@@ -787,7 +787,6 @@
       "isSubscription",
       "isTemplateNode",
       "isTypeProvider",
-      "isValidLink",
       "isValidatorFn",
       "isValueProvider",
       "iterateListLike",

packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json

@@ -781,7 +781,6 @@
       "isSubscription",
       "isTemplateNode",
       "isTypeProvider",
-      "isValidLink",
       "isValidatorFn",
       "isValueProvider",
       "isWritableSignal",

packages/core/test/bundling/router/bundle.golden_symbols.json

@@ -865,7 +865,6 @@
       "isTemplateNode",
       "isTypeProvider",
       "isUrlTree",
-      "isValidLink",
       "isValueProvider",
       "isWrappedDefaultExport",
       "iterator",

packages/core/test/render3/reactivity_spec.ts

@@ -1010,4 +1010,37 @@ describe('reactivity', () => {
       });
     });
   });
+
+  describe('graph', () => {
+    it('should keep links alive in a dynamic graph', () => {
+      const source = signal('initial');
+
+      @Component({
+        selector: 'test-cmp',
+        template: `{{ dynamic() }}{{ source() }}`,
+      })
+      class TestCmp {
+        @Input()
+        nonreactive: unknown;
+
+        get dynamic() {
+          return signal('');
+        }
+
+        source = source;
+      }
+
+      const fixture = TestBed.createComponent(TestCmp);
+      fixture.detectChanges();
+
+      fixture.componentRef.setInput('nonreactive', 'force_check');
+      fixture.detectChanges();
+      expect(fixture.nativeElement.textContent.trim()).toBe('initial');
+
+      source.set('updated');
+
+      fixture.detectChanges();
+      expect(fixture.nativeElement.textContent.trim()).toBe('updated');
+    });
+  });
 });

packages/core/test/signals/watch_spec.ts

@@ -161,6 +161,80 @@ describe('watchers', () => {
     expect(seenCounterValues).toEqual([2, 3]);
   });
 
+  it('should keep links alive in a dynamic graph', () => {
+    // This test verifies that reactive links between producer <> consumers are correctly maintained in a dynamic graph.
+    const decoy = signal(0);
+    const dynamic = signal(0);
+    const trigger = signal(false);
+
+    let executed = 0;
+    testingEffect(() => {
+      // Let the initial execution of the effect evaluate the decoy signal, incurring a consumer edge in the graph.
+      if (executed === 0) {
+        decoy();
+      }
+
+      // Evaluate a second signal; in the first execution this is the second consumer while it will be the first
+      // consumer in subsequent executions. The dynamic nature of this consumer means that its reactive link from the
+      // initial execution is not being reused in the second execution, as it is masked by the presence of the by-then
+      // stale link of `decoy`. Since the decoy is set to be unlinked, so will its followers as a mismatch in
+      // consumer ordering cause the entire chain of consumers to become invalid.
+      dynamic();
+
+      // Evaluate another signal last such that `dynamic` is not at the tail end of the effect's producer links, as that
+      // would also allow the consumer link of `dynamic` to be found and reused.
+      trigger();
+
+      executed++;
+    });
+    flushEffects();
+    expect(executed).toEqual(1);
+
+    // Initiate a change through the trigger signal, causing the removal of `decoy` to be noticed without touching the
+    // value of `dynamic`.
+    trigger.set(true);
+    flushEffects();
+    expect(executed).toEqual(2);
+
+    // Verify that updates to the decoy no longer cause the effect to run.
+    decoy.update((v) => v + 1);
+    flushEffects();
+    expect(executed).toEqual(2);
+
+    // Also verify that updates of the dynamic consumer are still tracked, causing the effect to rerun.
+    dynamic.update((v) => v + 1);
+    flushEffects();
+    expect(executed).toEqual(3);
+  });
+
+  it('should keep links alive when retriggered by a signal write', () => {
+    const a = signal(0);
+    const trigger = signal(0);
+
+    let executed = 0;
+    testingEffect(() => {
+      if (executed === 0) {
+        a();
+
+        // Update `a`, incrementing the epoch. This marks the effect dirty, causing it to be rerun again.
+        a.update((v) => v + 1);
+      }
+
+      // Read `trigger` when the epoch has already been incremented. The effect's reevaluation will therefore execute
+      // with the same epoch counter when reading `trigger`. Despite the epoch being equal, the read of `trigger` has
+      // to be recorded as reactive link during the reevaluation.
+      trigger();
+
+      executed++;
+    });
+    flushEffects();
+    expect(executed).toEqual(2);
+
+    trigger.update((v) => v + 1);
+    flushEffects();
+    expect(executed).toEqual(3);
+  });
+
   it('should throw an error when reading a signal during the notification phase', () => {
     const source = signal(0);
     let ranScheduler = false;