perf: reduce notifications on relationship remote state updates (#9655)

* perf: reduce notifications on relationship remote state updates * fix local replace semantics * fix rollback/sort-order * cleanup notifications mroe
emberjs · Feb 11, 2025 · a16b195 · a16b195
1 parent 1443246
commit a16b195
Show file tree

Hide file tree

Showing 7 changed files with 642 additions and 29 deletions.
diff --git a/packages/core-types/src/spec/document.ts b/packages/core-types/src/spec/document.ts
@@ -9,13 +9,13 @@ export interface ResourceMetaDocument {
   links?: Links | PaginationLinks;
 }
 
-export interface SingleResourceDataDocument<T = StableExistingRecordIdentifier> {
+export interface SingleResourceDataDocument<T = StableExistingRecordIdentifier, R = StableExistingRecordIdentifier> {
   // the url or cache-key associated with the structured document
   lid?: string;
   links?: Links | PaginationLinks;
   meta?: Meta;
   data: T | null;
-  included?: T[];
+  included?: R[];
 }
 
 export interface CollectionResourceDataDocument<T = StableExistingRecordIdentifier> {

diff --git a/packages/graph/src/-private/-diff.ts b/packages/graph/src/-private/-diff.ts
@@ -14,6 +14,7 @@ import replaceRelatedRecord from './operations/replace-related-record';
 import replaceRelatedRecords from './operations/replace-related-records';
 
 function _deprecatedCompare<T>(
+  priorLocalState: T[] | null,
   newState: T[],
   newMembers: Set<T>,
   prevState: T[],
@@ -30,6 +31,7 @@ function _deprecatedCompare<T>(
   const duplicates = new Map<T, number[]>();
   const finalSet = new Set<T>();
   const finalState: T[] = [];
+  const priorLocalLength = priorLocalState?.length ?? 0;
 
   for (let i = 0, j = 0; i < iterationLength; i++) {
     let adv = false;
@@ -69,6 +71,11 @@ function _deprecatedCompare<T>(
       // j is always less than i and so if i < prevLength, j < prevLength
       if (member !== prevState[j]) {
         changed = true;
+      } else if (!changed && j < priorLocalLength) {
+        const priorLocalMember = priorLocalState![j];
+        if (priorLocalMember !== member) {
+          changed = true;
+        }
       }
 
       if (!newMembers.has(prevMember)) {
@@ -100,6 +107,7 @@ function _deprecatedCompare<T>(
 }
 
 function _compare<T>(
+  priorLocalState: T[] | null,
   finalState: T[],
   finalSet: Set<T>,
   prevState: T[],
@@ -114,6 +122,7 @@ function _compare<T>(
   let changed: boolean = finalSet.size !== prevSet.size;
   const added = new Set<T>();
   const removed = new Set<T>();
+  const priorLocalLength = priorLocalState?.length ?? 0;
 
   for (let i = 0; i < iterationLength; i++) {
     let member: T | undefined;
@@ -135,6 +144,11 @@ function _compare<T>(
       // detect reordering
       if (equalLength && member !== prevMember) {
         changed = true;
+      } else if (equalLength && !changed && i < priorLocalLength) {
+        const priorLocalMember = priorLocalState![i];
+        if (priorLocalMember !== prevMember) {
+          changed = true;
+        }
       }
 
       if (!finalSet.has(prevMember)) {
@@ -169,11 +183,19 @@ export function diffCollection(
   onDel: (v: StableRecordIdentifier) => void
 ): Diff<StableRecordIdentifier> {
   const finalSet = new Set(finalState);
-  const { remoteState, remoteMembers } = relationship;
+  const { localState: priorLocalState, remoteState, remoteMembers } = relationship;
 
   if (DEPRECATE_NON_UNIQUE_PAYLOADS) {
     if (finalState.length !== finalSet.size) {
-      const { diff, duplicates } = _deprecatedCompare(finalState, finalSet, remoteState, remoteMembers, onAdd, onDel);
+      const { diff, duplicates } = _deprecatedCompare(
+        priorLocalState,
+        finalState,
+        finalSet,
+        remoteState,
+        remoteMembers,
+        onAdd,
+        onDel
+      );
 
       if (DEBUG) {
         deprecate(
@@ -199,7 +221,7 @@ export function diffCollection(
     );
   }
 
-  return _compare(finalState, finalSet, remoteState, remoteMembers, onAdd, onDel);
+  return _compare(priorLocalState, finalState, finalSet, remoteState, remoteMembers, onAdd, onDel);
 }
 
 export function computeLocalState(storage: CollectionEdge): StableRecordIdentifier[] {

diff --git a/packages/graph/src/-private/operations/replace-related-records.ts b/packages/graph/src/-private/operations/replace-related-records.ts
@@ -94,7 +94,7 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
   const { inverseKey, type } = relationship.definition;
   const { record } = op;
   const wasDirty = relationship.isDirty;
-  relationship.isDirty = false;
+  let localBecameDirty = false;
 
   const onAdd = (identifier: StableRecordIdentifier) => {
     // Since we are diffing against the remote state, we check
@@ -108,7 +108,8 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
         graph.registerPolymorphicType(type, identifier.type);
       }
 
-      relationship.isDirty = true;
+      // we've added a record locally that wasn't in the local state before
+      localBecameDirty = true;
       addToInverse(graph, identifier, inverseKey, op.record, isRemote);
 
       if (removalsHas) {
@@ -122,7 +123,8 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
     // if our previous local state had contained this identifier
     const additionsHas = additions?.has(identifier);
     if (additionsHas || !removals?.has(identifier)) {
-      relationship.isDirty = true;
+      // we've removed a record locally that was in the local state before
+      localBecameDirty = true;
       removeFromInverse(graph, identifier, inverseKey, record, isRemote);
 
       if (additionsHas) {
@@ -132,40 +134,39 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
   };
 
   const diff = diffCollection(identifiers, relationship, onAdd, onRemove);
-  // eslint-disable-next-line @typescript-eslint/no-unused-vars
-  let becameDirty = relationship.isDirty || diff.changed;
 
   // any additions no longer in the local state
-  // need to be removed from the inverse
+  // also need to be removed from the inverse
   if (additions && additions.size > 0) {
     additions.forEach((identifier) => {
       if (!diff.add.has(identifier)) {
-        becameDirty = true;
+        localBecameDirty = true;
         onRemove(identifier);
       }
     });
   }
 
   // any removals no longer in the local state
-  // need to be added back to the inverse
+  // also need to be added back to the inverse
   if (removals && removals.size > 0) {
     removals.forEach((identifier) => {
       if (!diff.del.has(identifier)) {
-        becameDirty = true;
+        localBecameDirty = true;
         onAdd(identifier);
       }
     });
   }
 
+  const becameDirty = diff.changed || localBecameDirty;
   relationship.additions = diff.add;
   relationship.removals = diff.del;
   relationship.localState = diff.finalState;
   relationship.isDirty = wasDirty;
 
-  if (
-    isMaybeFirstUpdate ||
-    !wasDirty /*&& becameDirty // TODO to guard like this we need to detect reorder when diffing local */
-  ) {
+  // we notify if this is the first update to the relationship
+  // because ?? may need to recalculate.
+  // otherwise we only notify if we are dirty and were not already dirty before
+  if (isMaybeFirstUpdate || (becameDirty && !wasDirty)) {
     notifyChange(graph, op.record, op.field);
   }
 }
@@ -181,6 +182,9 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
   if (isRemote) {
     graph._addToTransaction(relationship);
   }
+
+  // see note before flushCanonical
+  // const wasDirty = relationship.isDirty;
   relationship.state.hasReceivedData = true;
 
   // cache existing state
@@ -238,7 +242,13 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
   if (DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) {
     // only do this for legacy hasMany, not collection
     // and provide a way to incrementally migrate
-    if (relationship.definition.kind === 'hasMany' && relationship.definition.resetOnRemoteUpdate !== false) {
+    if (
+      // we do not guard by diff.changed here
+      // because we want to clear local changes even if
+      // no change has occurred to preserve the legacy behavior
+      relationship.definition.kind === 'hasMany' &&
+      relationship.definition.resetOnRemoteUpdate !== false
+    ) {
       const deprecationInfo: {
         removals: StableRecordIdentifier[];
         additions: StableRecordIdentifier[];
@@ -303,7 +313,10 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
     }
   }
 
-  if (relationship.isDirty) {
+  // we ought to only flush if we became dirty and were not before
+  // but this causes a fw test failures around unloadRecord and reference autotracking
+  // we should investigate this further
+  if (relationship.isDirty /*&& !wasDirty*/) {
     flushCanonical(graph, relationship);
   }
 }

diff --git a/packages/json-api/src/-private/cache.ts b/packages/json-api/src/-private/cache.ts
@@ -384,16 +384,22 @@ export default class JSONAPICache implements Cache {
    */
   mutate(mutation: LocalRelationshipOperation): void {
     if (LOG_MUTATIONS) {
+      logGroup('cache', 'mutate', mutation.record.type, mutation.record.lid, mutation.field, mutation.op);
       try {
         const _data = JSON.parse(JSON.stringify(mutation)) as object;
         // eslint-disable-next-line no-console
-        console.log(`EmberData | Mutation - update ${mutation.op}`, _data);
+        console.log(_data);
       } catch {
         // eslint-disable-next-line no-console
-        console.log(`EmberData | Mutation - update ${mutation.op}`, mutation);
+        console.log(mutation);
       }
     }
     this.__graph.update(mutation, false);
+
+    if (LOG_MUTATIONS) {
+      // eslint-disable-next-line no-console
+      console.groupEnd();
+    }
   }
 
   /**
@@ -543,8 +549,6 @@ export default class JSONAPICache implements Cache {
         // eslint-disable-next-line no-console
         console.log(data);
       }
-      // eslint-disable-next-line no-console
-      console.groupEnd();
     }
 
     if (cached.isNew) {
@@ -586,6 +590,11 @@ export default class JSONAPICache implements Cache {
       notifyAttributes(this._capabilities, identifier, changedKeys);
     }
 
+    if (LOG_OPERATIONS) {
+      // eslint-disable-next-line no-console
+      console.groupEnd();
+    }
+
     return changedKeys?.size ? Array.from(changedKeys) : undefined;
   }
 

diff --git a/tests/main/tests/integration/relationships/collection/mutating-has-many-test-infra.ts b/tests/main/tests/integration/relationships/collection/mutating-has-many-test-infra.ts
@@ -1,3 +1,4 @@
+import type { TestContext } from '@ember/test-helpers';
 import { settled } from '@ember/test-helpers';
 
 import { module, test } from 'qunit';
@@ -195,7 +196,14 @@ function generateAppliedMutation(store: Store, record: User, mutation: Mutation)
   };
 }
 
-async function applyMutation(assert: Assert, store: Store, record: User, mutation: Mutation, rc: ReactiveContext) {
+async function applyMutation(
+  this: TestContext,
+  assert: Assert,
+  store: Store,
+  record: User,
+  mutation: Mutation,
+  rc: ReactiveContext
+) {
   assert.ok(true, `LOG: applying "${mutation.name}" with ids [${mutation.values.map((v) => v.id).join(',')}]`);
 
   const { counters, fieldOrder } = rc;
@@ -433,8 +441,8 @@ export function runTestGroup(splitNum: number, offset: number) {
             });
             rc.reset();
 
-            await applyMutation(assert, store, user, mutation, rc);
-            await applyMutation(assert, store, user, mutation2, rc);
+            await applyMutation.call(this, assert, store, user, mutation, rc);
+            await applyMutation.call(this, assert, store, user, mutation2, rc);
           });
         });
       }