Don't overnotify for updates to added state that match local updates (emberjs#9702)

gitKrystan · runspired · MehulKChaudhari · commit e56f697d34c6 · 2025-03-16T23:33:29.000+05:30
* Don't overnotify for updates to added state that match local updates

* Fix typo

* Tidy up

* stash stuffs

* Add log counts to replace-related-records

* update all the stuffs

* fixup log

---------

Co-authored-by: Chris Thoburn &lt;runspired@users.noreply.github.com&gt;
diff --git a/packages/graph/src/-private/-diff.ts b/packages/graph/src/-private/-diff.ts
@@ -1,5 +1,6 @@
 import { deprecate } from '@ember/debug';
 
+import { DEBUG_RELATIONSHIP_NOTIFICATIONS } from '@warp-drive/build-config/debugging';
 import {
   DEPRECATE_NON_UNIQUE_PAYLOADS,
   DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE,
@@ -52,7 +53,13 @@ function _deprecatedCompare<T>(
         adv = true;
 
         if (!prevSet.has(member)) {
-          changed = true;
+          // Avoid unnecessarily notifying a change that already exists locally
+          if (i < priorLocalLength) {
+            const priorLocalMember = priorLocalState![i];
+            if (priorLocalMember !== member) {
+              changed = true;
+            }
+          }
           added.add(member);
           onAdd(member);
         }
@@ -157,25 +164,59 @@ function _compare<T>(
   const finalLength = finalState.length;
   const prevLength = prevState.length;
   const iterationLength = Math.max(finalLength, prevLength);
-  const equalLength = finalLength === prevLength;
-  let changed: boolean = finalSet.size !== prevSet.size;
-  let remoteOrderChanged = false;
+  const equalLength = priorLocalState ? finalLength === priorLocalState.length : finalLength === prevLength;
+  let remoteOrderChanged = finalSet.size !== prevSet.size;
+  let changed: boolean = priorLocalState ? finalSet.size !== priorLocalState.length : remoteOrderChanged;
   const added = new Set<T>();
   const removed = new Set<T>();
   const priorLocalLength = priorLocalState?.length ?? 0;
 
+  if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+    if (changed) {
+      // console.log({
+      //   priorState: priorLocalState?.slice(),
+      //   finalState: finalState.slice(),
+      //   prevState: prevState.slice(),
+      // });
+    }
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+    changed &&
+      // eslint-disable-next-line no-console
+      console.log(
+        `changed because ${priorLocalState ? 'finalSet.size !== priorLocalState.length' : 'finalSet.size !== prevSet.size'}`
+      );
+  }
+
   for (let i = 0; i < iterationLength; i++) {
     let member: T | undefined;
 
     // accumulate anything added
     if (i < finalLength) {
       member = finalState[i];
       if (!prevSet.has(member)) {
-        // TODO: in order to avoid unnecessarily notifying a change here
-        // we would need to only notify "changed" if member is not in
-        // relationship.additions OR if localState[i] !== member
-
-        changed = true;
+        // Avoid unnecessarily notifying a change that already exists locally
+        if (i < priorLocalLength) {
+          const priorLocalMember = priorLocalState![i];
+          if (priorLocalMember !== member) {
+            if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+              if (!changed) {
+                // console.log({
+                //   priorLocalMember,
+                //   member,
+                //   i,
+                //   priorState: priorLocalState?.slice(),
+                //   finalState: finalState.slice(),
+                //   prevState: prevState.slice(),
+                // });
+              }
+
+              // eslint-disable-next-line @typescript-eslint/no-unused-expressions, no-console
+              !changed && console.log(`changed because priorLocalMember !== member && !prevSet.has(member)`);
+            }
+            changed = true;
+          }
+        }
         added.add(member);
         onAdd(member);
       }
@@ -206,9 +247,19 @@ function _compare<T>(
           if (i < priorLocalLength) {
             const priorLocalMember = priorLocalState![i];
             if (priorLocalMember !== member) {
+              if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+                // eslint-disable-next-line @typescript-eslint/no-unused-expressions, no-console
+                !changed && console.log(`changed because priorLocalMember !== member && member !== prevMember`);
+              }
               changed = true;
             }
-          } else {
+          } else if (i < finalLength) {
+            // if we have exceeded the length of priorLocalState and we are within the range
+            // of the finalState then we must have changed
+            if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+              // eslint-disable-next-line @typescript-eslint/no-unused-expressions, no-console
+              !changed && console.log(`changed because priorMember !== member && index >= priorLocalLength`);
+            }
             changed = true;
           }
         }
@@ -228,7 +279,53 @@ function _compare<T>(
       }
 
       if (!finalSet.has(prevMember)) {
-        changed = true;
+        // if we are within finalLength, we can only be "changed" if we've already exceeded
+        // the index range of priorLocalState, as otherwise the previous member may still
+        // be removed.
+        //
+        // prior local: [1, 2, 3, 4]
+        // final state: [1, 2, 3]
+        // prev remote state: [1, 2, 5, 3, 4]
+        // i === 2
+        // prevMember === 5
+        // !finalSet.has(prevMember) === true
+        //
+        // because we will become changed at i===3,
+        // we do not need to worry about becoming changed at i===2
+        // as the arrays until now are still the same
+        //
+        // prior local: [1, 2, 3]
+        // final state: [1, 2, 3, 4]
+        // prev remote state: [1, 2, 5, 3, 4]
+        // i === 2
+        // prevMember === 5
+        // !finalSet.has(prevMember) === true
+        //
+        // because we will become changed at i===3
+        // we do not need to worry about becoming changed at i===2
+        //
+        // prior local: [1, 2, 3]
+        // final state: [1, 2, 3]
+        // prev remote state: [1, 2, 5, 3, 4]
+        // i === 2
+        // prevMember === 5
+        // !finalSet.has(prevMember) === true
+        //
+        // because we have same length and same membership order
+        // we do not need to worry about becoming changed at i===2
+        //
+        // if you do not have a priorLocalState you can't be changed
+        // ergo, we never need to set changed in this branch.
+        // this log can still be useful for debugging.
+        if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+          // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+          !changed &&
+            // eslint-disable-next-line no-console
+            console.log(`changed because i >= priorLocalLength && i < finalLength && !finalSet.has(prevMember)`);
+        }
+        //
+        // we do still set remoteOrderChanged as it has
+        remoteOrderChanged = true;
         removed.add(prevMember);
         onDel(prevMember);
       }
diff --git a/packages/graph/src/-private/operations/replace-related-records.ts b/packages/graph/src/-private/operations/replace-related-records.ts
@@ -1,5 +1,6 @@
 import { deprecate } from '@ember/debug';
 
+import { DEBUG_RELATIONSHIP_NOTIFICATIONS, LOG_METRIC_COUNTS } from '@warp-drive/build-config/debugging';
 import { DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE } from '@warp-drive/build-config/deprecations';
 import { DEBUG } from '@warp-drive/build-config/env';
 import { assert } from '@warp-drive/build-config/macros';
@@ -12,6 +13,12 @@ import { assertPolymorphicType } from '../debug/assert-polymorphic-type';
 import type { CollectionEdge } from '../edges/collection';
 import type { Graph } from '../graph';
 
+function count(label: string) {
+  // @ts-expect-error
+  // eslint-disable-next-line
+  globalThis.__WarpDriveMetricCountData[label] = (globalThis.__WarpDriveMetricCountData[label] || 0) + 1;
+}
+
 /*
     case many:1
     ========
@@ -87,6 +94,10 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
   const wasDirty = relationship.isDirty;
   let localBecameDirty = false;
 
+  if (LOG_METRIC_COUNTS) {
+    count(`replaceRelatedRecordsLocal ${'type' in record ? record.type : '<document>'} ${op.field}`);
+  }
+
   const onAdd = (identifier: StableRecordIdentifier) => {
     // Since we are diffing against the remote state, we check
     // if our previous local state did not contain this identifier
@@ -164,6 +175,10 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
   const identifiers = op.value;
   const relationship = graph.get(op.record, op.field);
 
+  if (LOG_METRIC_COUNTS) {
+    count(`replaceRelatedRecordsRemote ${'type' in op.record ? op.record.type : '<document>'}  ${op.field}`);
+  }
+
   assert(
     `You can only '${op.op}' on a hasMany relationship. ${op.record.type}.${op.field} is a ${relationship.definition.kind}`,
     isHasMany(relationship)
@@ -203,6 +218,14 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
       if (relationship.additions?.has(identifier)) {
         relationship.additions.delete(identifier);
       } else {
+        if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+          if (!relationship.isDirty) {
+            // eslint-disable-next-line no-console
+            console.log(
+              `setting relationship to dirty because the remote addition was not in our previous list of local additions`
+            );
+          }
+        }
         relationship.isDirty = true;
       }
       addToInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
@@ -214,6 +237,14 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
       if (relationship.removals?.has(identifier)) {
         relationship.removals.delete(identifier);
       } else {
+        if (DEBUG_RELATIONSHIP_NOTIFICATIONS) {
+          if (!relationship.isDirty) {
+            // eslint-disable-next-line no-console
+            console.log(
+              `setting relationship to dirty because the remote removal was not in our previous list of local removals`
+            );
+          }
+        }
         relationship.isDirty = true;
       }
       removeFromInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
