save

packages/-ember-data/tests/test-helper.js

@@ -21,7 +21,7 @@ if (window.Promise === undefined) {
 if (QUnit.urlParams.enableoptionalfeatures) {
   window.EmberDataENV = { ENABLE_OPTIONAL_FEATURES: true };
 }
-
+QUnit.dump.maxDepth = 3;
 setup(QUnit.assert);
 
 configureAsserts();

packages/model/addon/-private/legacy-relationships-support.ts

@@ -10,7 +10,7 @@ import type { ImplicitRelationship } from '@ember-data/record-data/-private/grap
 import type BelongsToRelationship from '@ember-data/record-data/-private/relationships/state/belongs-to';
 import type ManyRelationship from '@ember-data/record-data/-private/relationships/state/has-many';
 import type Store from '@ember-data/store';
-import { recordIdentifierFor, SOURCE, storeFor } from '@ember-data/store/-private';
+import { fastPush, recordIdentifierFor, SOURCE, storeFor } from '@ember-data/store/-private';
 import { IdentifierCache } from '@ember-data/store/-private/caches/identifier-cache';
 import { NonSingletonRecordDataManager } from '@ember-data/store/-private/managers/record-data-manager';
 import type { DSModel } from '@ember-data/types/q/ds-model';
@@ -109,13 +109,13 @@ export class LegacySupport {
         // we found a change
         array.arrayContentWillChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount);
         currentState.length = 0;
-        Array.prototype.push.apply(currentState, identifiers);
+        fastPush(currentState, identifiers);
         array.arrayContentDidChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount);
       }
     } else {
       array._hasNotified = false;
       currentState.length = 0;
-      Array.prototype.push.apply(currentState, identifiers);
+      fastPush(currentState, identifiers);
     }
 
     array.isUpdating = false;

packages/model/addon/-private/model.js

@@ -840,12 +840,15 @@ class Model extends EmberObject {
   rollbackAttributes() {
     const { currentState } = this;
     const { isNew } = currentState;
-    recordDataFor(this).rollbackAttrs(recordIdentifierFor(this));
-    this.errors.clear();
-    currentState.cleanErrorRequests();
-    if (isNew) {
-      this.unloadRecord();
-    }
+
+    storeFor(this)._join(() => {
+      recordDataFor(this).rollbackAttrs(recordIdentifierFor(this));
+      this.errors.clear();
+      currentState.cleanErrorRequests();
+      if (isNew) {
+        this.unloadRecord();
+      }
+    });
   }
 
   /**

packages/store/addon/-private/caches/instance-cache.ts

@@ -120,7 +120,6 @@ type Caches = {
 export class InstanceCache {
   declare store: Store;
   declare _storeWrapper: RecordDataStoreWrapper;
-  declare peekList: Dict<Set<StableRecordIdentifier>>;
   declare __recordDataFor: (resource: RecordIdentifier) => RecordData;
 
   declare __cacheManager: NonSingletonRecordDataManager;
@@ -170,7 +169,6 @@ export class InstanceCache {
 
   constructor(store: Store) {
     this.store = store;
-    this.peekList = Object.create(null) as Dict<Set<StableRecordIdentifier>>;
 
     this._storeWrapper = new RecordDataStoreWrapper(this.store);
     this.__recordDataFor = (resource: RecordIdentifier) => {
@@ -227,7 +225,6 @@ export class InstanceCache {
         if (otherHasRecord) {
           // TODO probably need to release other things
           // TODO notify recordArrayManager
-          this.peekList[altIdentifier.type]?.delete(altIdentifier);
         }
 
         if (imRecordData === null && otherRecordData === null) {
@@ -243,14 +240,11 @@ export class InstanceCache {
           if (imRecordData) {
             // TODO check if we are retained in any async relationships
             // TODO probably need to release other things
-            this.peekList[intendedIdentifier.type]?.delete(intendedIdentifier);
             // im.destroy();
           }
           imRecordData = otherRecordData!;
           // TODO do we need to notify the id change?
           // TODO swap recordIdentifierFor result?
-          this.peekList[intendedIdentifier.type] = this.peekList[intendedIdentifier.type] || new Set();
-          this.peekList[intendedIdentifier.type]!.add(intendedIdentifier);
 
           // just use im
         } else {
@@ -353,8 +347,6 @@ export class InstanceCache {
       setRecordDataFor(identifier, recordData);
 
       this.__instances.recordData.set(identifier, recordData);
-      this.peekList[identifier.type] = this.peekList[identifier.type] || new Set();
-      this.peekList[identifier.type]!.add(identifier);
       if (LOG_INSTANCE_CACHE) {
         // eslint-disable-next-line no-console
         console.log(`InstanceCache: created RecordData for ${String(identifier)}`);
@@ -420,7 +412,6 @@ export class InstanceCache {
     this.store._join(() => {
       const record = this.__instances.record.get(identifier);
       const recordData = this.__instances.recordData.get(identifier);
-      this.peekList[identifier.type]?.delete(identifier);
 
       if (record) {
         this.store.teardownRecord(record);
@@ -455,19 +446,14 @@ export class InstanceCache {
   }
 
   clear(type?: string) {
+    const typeCache = this.store.identifierCache._cache.types;
     if (type === undefined) {
-      let keys = Object.keys(this.peekList);
+      let keys = Object.keys(typeCache);
       keys.forEach((key) => this.clear(key));
     } else {
-      let identifiers = this.peekList[type];
+      let identifiers = typeCache[type]?.lid;
       if (identifiers) {
         identifiers.forEach((identifier) => {
-          // TODO we rely on not removing the main cache
-          // and only removing the peekList cache apparently.
-          // we should figure out this duality and codify whatever
-          // signal it is actually trying to give us.
-          // this.cache.delete(identifier);
-          this.peekList[identifier.type]!.delete(identifier);
           this.unloadRecord(identifier);
           // TODO we don't remove the identifier, should we?
         });

packages/store/addon/-private/index.ts

@@ -44,6 +44,7 @@ export { default as coerceId } from './utils/coerce-id';
 export {
   default as RecordArray,
   default as IdentifierArray,
+  fastPush,
   SOURCE,
   IDENTIFIER_ARRAY_TAG,
 } from './record-arrays/identifier-array';

packages/store/addon/-private/managers/record-array-manager.ts

@@ -9,6 +9,7 @@ import { InstanceCache } from '../caches/instance-cache';
 import IdentifierArray, {
   Collection,
   CollectionCreateOptions,
+  fastPush,
   IDENTIFIER_ARRAY_TAG,
   SOURCE,
   sync,
@@ -173,7 +174,7 @@ class RecordArrayManager {
     const source = array[SOURCE];
     const old = source.slice();
     source.length = 0;
-    Array.prototype.push.apply(source, identifiers);
+    fastPush(source, identifiers);
     array[IDENTIFIER_ARRAY_TAG].ref = null;
     array.meta = payload.meta || null;
     array.links = payload.links || null;
@@ -257,9 +258,14 @@ function disassociateIdentifier(array: IdentifierArray, identifier: StableRecord
   }
 }
 
-function visibleIdentifiersByType(cache: InstanceCache, type: string) {
-  const list = cache.peekList[type];
-  let visible: StableRecordIdentifier[] = [];
+// TODO we can probably get rid of this and build up the list
+// as we are notified of changes
+// doing so *might* decrease costs by allowing us to avoid
+// the `recordIsLoaded` check.
+// for 100k records this is like 35ms currently
+function visibleIdentifiersByType(cache: InstanceCache, type: string): StableRecordIdentifier[] {
+  const list = cache.store.identifierCache._cache.types[type]?.lid;
+  const visible: StableRecordIdentifier[] = [];
   const getLoaded = (identifier: StableRecordIdentifier) => {
     if (cache.recordIsLoaded(identifier, true)) {
       visible.push(identifier);

packages/store/addon/-private/record-arrays/identifier-array.ts

@@ -16,6 +16,7 @@ import type { StableRecordIdentifier } from '@ember-data/types/q/identifier';
 import type { RecordInstance } from '@ember-data/types/q/record-instance';
 import { Dict } from '@ember-data/types/q/utils';
 
+import { recordIdentifierFor } from '../caches/instance-cache';
 import type RecordArrayManager from '../managers/record-array-manager';
 import { PromiseArray, promiseArray } from '../proxies/promise-proxies';
 import type Store from '../store-service';
@@ -41,14 +42,66 @@ const ARRAY_GETTER_METHODS = new Set<string | symbol | number>([
   'reduce',
   'reduceRight',
   'slice',
-  'splice',
   'some',
   'values',
 ]);
+const ARRAY_SETTER_METHODS = new Set<string>(['push', 'pop', 'unshift', 'shift', 'splice']);
 
 export const IDENTIFIER_ARRAY_TAG = Symbol('#tag');
 export const SOURCE = Symbol('#source');
 
+const SLICE_BATCH_SIZE = 1200;
+/**
+ * This is a clever optimization.
+ *
+ * clever optimizations rarely stand the test of time, so if you're
+ * ever curious or think something better is possible please benchmark
+ * and discuss. The benchmark for this at the time of writing is in
+ * `scripts/benchmark-push.js`
+ *
+ * This approach turns out to be 150x faster in Chrome and node than
+ * simply using push or concat. It's highly susceptible to the specifics
+ * of the batch size, and may require tuning.
+ *
+ * Clever optimizations should always come with a `why`. This optimization
+ * exists for two reasons.
+ *
+ * 1) array.push(...objects) and Array.prototype.push.apply(arr, objects)
+ *   are susceptible to stack overflows. The size of objects at which this
+ *   occurs varies by environment, browser, and current stack depth and memory
+ *   pressure; however, it occurs in all browsers in fairly pristine conditions
+ *   somewhere around 125k to 200k elements. Since EmberData regularly encounters
+ *   arrays larger than this in size, we cannot use push.
+ *
+ * 2) `array.concat` or simply setting the array to a new reference is often an
+ *   easier approach; however, native Proxy to an array cannot swap it's target array
+ *   and attempts at juggling multiple array sources have proven to be victim to a number
+ *   of browser implementation bugs. Should these bugs be addressed then we could
+ *   simplify to using `concat`, however, do note this is currently 150x faster
+ *   than concat, and due to the overloaded signature of concat will likely always
+ *   be faster.
+ *
+ * Sincerely,
+ *   - runspired (Chris Thoburn) 08/21/2022
+ *
+ * @method fastPush
+ * @private
+ * @param target the array to push into
+ * @param source the items to push into target
+ */
+export function fastPush<T>(target: T[], source: T[]) {
+  let startLength = 0;
+  let newLength = source.length;
+  let batch: T[];
+  while (newLength - startLength > SLICE_BATCH_SIZE) {
+    batch = source.slice(startLength, SLICE_BATCH_SIZE);
+    target.push(...batch);
+    startLength += SLICE_BATCH_SIZE;
+  }
+  batch = source.slice(startLength);
+  target.push(...batch);
+}
+
 function convertToInt(prop: number | string | symbol): number | null {
   if (typeof prop === 'symbol') return null;
 
@@ -108,8 +161,9 @@ export function sync(array: IdentifierArray, changes: Map<StableRecordIdentifier
   }
 
   if (adds.length) {
-    Array.prototype.push.apply(state, adds);
+    fastPush(state, adds);
     // changing the reference breaks the Proxy
+    // else we could do this
     /*
     if (state.length === 0) {
       array[SOURCE] = adds;
@@ -211,6 +265,10 @@ class IdentifierArray {
     const boundFns = new Map<string | symbol, ProxiedMethod>();
     const _TAG = this[IDENTIFIER_ARRAY_TAG];
 
+    // when a mutation occurs
+    // we track all mutations within the call
+    // and forward them as one
+
     return new Proxy<StableRecordIdentifier[], RecordInstance[]>(this[SOURCE], {
       get(target, prop, receiver): unknown {
         let index = convertToInt(prop);
@@ -260,7 +318,7 @@ class IdentifierArray {
           assert(`Mutating ${String(prop)} on this RecordArray is not allowed.`, options.allowMutation);
           return false;
         }
-        (target as unknown as Record<string | symbol, unknown>)[prop] = value;
+        (target as unknown as Record<string | symbol, unknown>)[prop] = recordIdentifierFor(value as RecordInstance);
         _TAG.ref = null;
 
         return true;
@@ -576,7 +634,7 @@ if (DEPRECATE_ARRAY_LIKE) {
   IdentifierArray.prototype.setObjects = function (objects: RecordInstance[]) {
     deprecateArrayLike(this.DEPRECATED_CLASS_NAME, 'clear', 'length = 0');
     this.splice(0, this.length);
-    Array.prototype.push.apply(this, objects);
+    this.push(...objects);
     return this;
   };
 

packages/store/addon/-private/store-service.ts

@@ -627,7 +627,7 @@ class Store extends Service {
     if (DEBUG) {
       assertDestroyingStore(this, 'unloadRecord');
     }
-    let identifier = peekRecordIdentifier(record);
+    const identifier = peekRecordIdentifier(record);
     if (identifier) {
       this._instanceCache.unloadRecord(identifier);
     }