From 484909c4fc400a3bcc85f46d0145fd89839884bc Mon Sep 17 00:00:00 2001
From: "David J. Hamilton" <dhamilton@linkedin.com>
Date: Wed, 19 Oct 2016 07:44:56 -0700
Subject: [PATCH] Free up `internalModel`s

`internalModel`s need to stay around as long as they're connected to a live
record via relationships.  Once an entire subgraph is removed however, we can
free all of it.

Also add a couple of "late private" fields to `internalModel`'s constructor for
shape preservation.

Includes work from @igort and @sly7-7

[Fix #3296]
[Supercede #3301]
---
 addon/-private/system/model/internal-model.js | 137 ++++++++++++++++--
 addon/-private/system/model/model.js          |   6 +-
 addon/-private/system/model/states.js         |  10 --
 addon/-private/system/record-array-manager.js |   5 +-
 addon/-private/system/store.js                |  21 +--
 tests/integration/records/load-test.js        |   9 +-
 tests/integration/records/unload-test.js      | 108 ++++++++++----
 .../relationships/has-many-test.js            |   4 +-
 tests/integration/store-test.js               |   3 -
 tests/unit/store/unload-test.js               |  37 ++---
 10 files changed, 236 insertions(+), 104 deletions(-)

diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js
index aa15574a875..5aeca0164bb 100644
--- a/addon/-private/system/model/internal-model.js
+++ b/addon/-private/system/model/internal-model.js
@@ -59,6 +59,16 @@ function extractPivotName(name) {
   );
 }
 
+function areAllModelsUnloaded(models) {
+  for (let i=0; i<models.length; ++i) {
+    let record = models[i].record;
+    if (record && !(record.get('isDestroyed') || record.get('isDestroying'))) {
+      return false;
+    }
+  }
+  return true;
+}
+
 // this (and all heimdall instrumentation) will be stripped by a babel transform
 //  https://github.com/heimdalljs/babel5-plugin-strip-heimdall
 const {
@@ -87,6 +97,8 @@ const {
   'updateChangedAttributes'
 );
 
+let nextBfsId = 1;
+
 /*
   `InternalModel` is the Model class that we use internally inside Ember Data to represent models.
   Internal ED methods should only deal with `InternalModel` objects. It is a fast, plain Javascript class.
@@ -111,28 +123,28 @@ export default class InternalModel {
     this.store = store;
     this._data = data || new EmptyObject();
     this.modelName = type.modelName;
-    this.dataHasInitialized = false;
     this._loadingPromise = null;
     this._recordArrays = undefined;
-    this.currentState = RootState.empty;
-    this.isReloading = false;
+    this._isDestroying = false;
     this._isDestroyed = false;
     this.isError = false;
-    this.error = null;
+
+    this.resetRecord();
 
     // caches for lazy getters
     this.__deferredTriggers = null;
     this._references = null;
     this._recordReference = null;
-    this.__inFlightAttributes = null;
     this.__relationships = null;
-    this.__attributes = null;
     this.__implicitRelationships = null;
+
+    // Used for coloring during BFS
+    this._bfsId = 0;
   }
 
   get recordReference() {
     if (this._recordReference === null) {
-      this._recordReference = new RecordReference(this.store, this)
+      this._recordReference = new RecordReference(this.store, this);
     }
     return this._recordReference;
   }
@@ -278,8 +290,22 @@ export default class InternalModel {
     this._triggerDeferredTriggers();
   }
 
-  recordObjectWillDestroy() {
+  resetRecord() {
     this.record = null;
+    this.dataHasInitialized = false;
+    this.isReloading = false;
+    this.error = null;
+    this.currentState = RootState.empty;
+    this.__attributes = null;
+    this.__inFlightAttributes = null;
+  }
+
+  dematerializeRecord() {
+    if (this.record) {
+      this.record.destroy();
+      this.clearRelationships();
+      this.updateRecordArrays();
+    }
   }
 
   deleteRecord() {
@@ -333,14 +359,102 @@ export default class InternalModel {
     return this.record;
   }
 
+
+  /**
+    Computes the set of internal models reachable from `this` acrosxactly one
+    relationship.
+
+    @return {Array} An array containing the internal models that `t` belongs
+    to or has many.
+  */
+  _directlyRelatedInternalModels() {
+    let array = [];
+    this.type.eachRelationship((key, relationship) => {
+      if (this._relationships.has(key)) {
+        let related = this._relationships.get(key).members.toArray();
+        for (let i=0; i<related.length; ++i) {
+          array.push(related[i]);
+        }
+      }
+    });
+    return array;
+  }
+
+
+  /**
+    Computes the set of internal models reachable from this internaodel.
+
+    Reachability is determind over the relatinoship graph (ie a grawhere
+    nodes are internal models and edges are belongs to or has many
+    relationships.
+
+    @return {Array} An array including `this` and all internal modereachable
+    from `this`.
+  */
+  _allRelatedInternalModels() {
+    let array = [];
+    let queue = [];
+    let bfsId = nextBfsId++;
+    queue.push(this);
+    this._bfsId = bfsId;
+    while (queue.length > 0) {
+      let node = queue.shift();
+      array.push(node);
+      let related = node._directlyRelatedInternalModels();
+      for (let i=0; i<related.length; ++i) {
+        let internalModel = related[i];
+        if (internalModel._bfsId < bfsId) {
+          queue.push(internalModel);
+          internalModel._bfsId = bfsId;
+        }
+      }
+    }
+    return array;
+  }
+
+
+  /**
+    Unload the record for this internal model.  This will cause thecord to be
+    destroyed and freed up for garbage collection.  It will also trer a check
+    for cleaning up internal models.
+
+    This check is performed by first computing the set of related irnal
+    models.  If all records in this set are unloaded, then the entiset is
+    destroyed.  Otherwise, nothing in the set is destroyed.
+
+    This means that this internal model will be freed up for garbagollection
+    once all models that refer to it via some relationship are alsoloaded.
+  */
   unloadRecord() {
     this.send('unloadRecord');
+    this.dematerializeRecord();
+
+    let relatedInternalModels = this._allRelatedInternalModels();
+    if (areAllModelsUnloaded(relatedInternalModels)) {
+      for (let i=0; i<relatedInternalModels.length; ++i) {
+        let internalModel = relatedInternalModels[i];
+        if (!internalModel.isDestroying) {
+          internalModel.destroy();
+        }
+      }
+    }
   }
 
   eachRelationship(callback, binding) {
     return this.type.eachRelationship(callback, binding);
   }
 
+  destroy() {
+    this._isDestroying = true;
+    Ember.run.schedule('destroy', this, this.willDestroy, this);
+  }
+
+  willDestroy() {
+    assert("Cannot destroy an internalModel while its record materialized", !this.record);
+    this.store._removeFromIdMap(this);
+    this._isDestroyed = true;
+  }
+
   eachAttribute(callback, binding) {
     return this.type.eachAttribute(callback, binding);
   }
@@ -375,11 +489,8 @@ export default class InternalModel {
     return this._isDestroyed;
   }
 
-  destroy() {
-    this._isDestroyed = true;
-    if (this.record) {
-      return this.record.destroy();
-    }
+  get isDestroying() {
+    return this._isDestroying;
   }
 
   /*
diff --git a/addon/-private/system/model/model.js b/addon/-private/system/model/model.js
index fffc6e50af1..b3af5cd5759 100644
--- a/addon/-private/system/model/model.js
+++ b/addon/-private/system/model/model.js
@@ -808,9 +808,9 @@ var Model = Ember.Object.extend(Ember.Evented, {
   willDestroy() {
     //TODO Move!
     this._super(...arguments);
-    this._internalModel.clearRelationships();
-    this._internalModel.recordObjectWillDestroy();
-    //TODO should we set internalModel to null here?
+
+    this._internalModel.resetRecord();
+    this._internalModel = null;
   },
 
   // This is a temporary solution until we refactor DS.Model to not
diff --git a/addon/-private/system/model/states.js b/addon/-private/system/model/states.js
index 85cecb4c06c..493274de0eb 100644
--- a/addon/-private/system/model/states.js
+++ b/addon/-private/system/model/states.js
@@ -459,10 +459,6 @@ const RootState = {
   // you out of the in-flight state.
   rolledBack() { },
   unloadRecord(internalModel) {
-    // clear relationships before moving to deleted state
-    // otherwise it fails
-    internalModel.clearRelationships();
-    internalModel.transitionTo('deleted.saved');
   },
 
   propertyWasReset() { },
@@ -573,10 +569,6 @@ const RootState = {
       },
 
       unloadRecord(internalModel) {
-        // clear relationships before moving to deleted state
-        // otherwise it fails
-        internalModel.clearRelationships();
-        internalModel.transitionTo('deleted.saved');
       },
 
       didCommit() {},
@@ -680,8 +672,6 @@ const RootState = {
 
       setup(internalModel) {
         internalModel.clearRelationships();
-        var store = internalModel.store;
-        store._dematerializeRecord(internalModel);
       },
 
       invokeLifecycleCallbacks(internalModel) {
diff --git a/addon/-private/system/record-array-manager.js b/addon/-private/system/record-array-manager.js
index c87795376cc..96119744bfa 100644
--- a/addon/-private/system/record-array-manager.js
+++ b/addon/-private/system/record-array-manager.js
@@ -105,9 +105,8 @@ export default Ember.Object.extend({
   updateRecordArrays() {
     heimdall.increment(updateRecordArrays);
     this.changedRecords.forEach(internalModel => {
-
-      if (internalModel.isDestroyed ||
-          internalModel.currentState.stateName === 'root.deleted.saved') {
+      if (internalModel.isDestroying ||
+           internalModel.currentState.stateName === 'root.deleted.saved') {
         this._recordWasDeleted(internalModel);
       } else {
         this._recordWasChanged(internalModel);
diff --git a/addon/-private/system/store.js b/addon/-private/system/store.js
index e30e25bc857..840f8660565 100644
--- a/addon/-private/system/store.js
+++ b/addon/-private/system/store.js
@@ -1600,7 +1600,7 @@ Store = Service.extend({
       let types = new Array(keys.length);
 
       for (let i = 0; i < keys.length; i++) {
-        types[i] = typeMaps[keys[i]]['type'].modelName;
+        types[i] = typeMaps[keys[i]].type.modelName;
       }
 
       types.forEach(this.unloadAll, this);
@@ -1608,12 +1608,11 @@ Store = Service.extend({
       let typeClass = this.modelFor(modelName);
       let typeMap = this.typeMapFor(typeClass);
       let records = typeMap.records.slice();
-      let record;
+      let internalModel;
 
       for (let i = 0; i < records.length; i++) {
-        record = records[i];
-        record.unloadRecord();
-        record.destroy(); // maybe within unloadRecord
+        internalModel = records[i];
+        internalModel.unloadRecord();
       }
 
       typeMap.metadata = new EmptyObject();
@@ -2441,21 +2440,11 @@ Store = Service.extend({
   // . DESTRUCTION .
   // ...............
 
-  /**
-    When a record is destroyed, this un-indexes it and
-    removes it from any record arrays so it can be GCed.
-
-    @method _dematerializeRecord
-    @private
-    @param {InternalModel} internalModel
-  */
-  _dematerializeRecord(internalModel) {
+  _removeFromIdMap(internalModel) {
     var type = internalModel.type;
     var typeMap = this.typeMapFor(type);
     var id = internalModel.id;
 
-    internalModel.updateRecordArrays();
-
     if (id) {
       delete typeMap.idToRecord[id];
     }
diff --git a/tests/integration/records/load-test.js b/tests/integration/records/load-test.js
index d3e4b96b2fe..02e070c0c1c 100644
--- a/tests/integration/records/load-test.js
+++ b/tests/integration/records/load-test.js
@@ -25,19 +25,14 @@ module("integration/load - Loading Records", {
   }
 });
 
-test("When loading a record fails, the isLoading is set to false", function(assert) {
+test("When loading a record fails, the record is not left behind", function(assert) {
   env.adapter.findRecord = function(store, type, id, snapshot) {
     return Ember.RSVP.reject();
   };
 
   run(function() {
     env.store.findRecord('post', 1).then(null, assert.wait(function() {
-      // store.recordForId is private, but there is currently no other way to
-      // get the specific record instance, since it is not passed to this
-      // rejection handler
-      var post = env.store.recordForId('post', 1);
-
-      assert.equal(post.get("isLoading"), false, "post is not loading anymore");
+      assert.equal(env.store.hasRecordForId('post', 1), false);
     }));
   });
 });
diff --git a/tests/integration/records/unload-test.js b/tests/integration/records/unload-test.js
index 5dd4c95bf15..188f8d32b4b 100644
--- a/tests/integration/records/unload-test.js
+++ b/tests/integration/records/unload-test.js
@@ -230,7 +230,7 @@ test("unloading all records also updates record array from peekAll()", function(
 });
 
 
-test("unloading a record also clears its relationship", function(assert) {
+test("a relationship to an unloaded record can be restored", function(assert) {
   var adam, bob;
 
   // disable background reloading so we do not re-create the relationship.
@@ -276,62 +276,112 @@ test("unloading a record also clears its relationship", function(assert) {
   });
 
   run(function() {
-    env.store.findRecord('person', 1).then(function(person) {
-      assert.equal(person.get('cars.length'), 1, 'The inital length of cars is correct');
+    let person = env.store.peekRecord('person', 1);
+    assert.equal(person.get('cars.length'), 1, 'The inital length of cars is correct');
 
-      run(function() {
-        person.unloadRecord();
-      });
+    assert.equal(env.store.hasRecordForId('person', 1), true, 'The person is in the store');
 
-      assert.equal(person.get('cars.length'), undefined);
+    run(function() {
+      person.unloadRecord();
     });
-  });
-});
 
-test("unloading a record also clears the implicit inverse relationships", function(assert) {
-  var adam, bob;
-  // disable background reloading so we do not re-create the relationship.
-  env.adapter.shouldBackgroundReloadRecord = () => false;
+    assert.equal(env.store.hasRecordForId('person', 1), false, 'The person is unloaded');
+  });
 
-  run(function() {
+  run(() => {
     env.store.push({
       data: {
         type: 'person',
         id: '1',
         attributes: {
           name: 'Adam Sunderland'
+        },
+        relationships: {
+          cars: {
+            data: [
+              { type: 'car', id: '1' }
+            ]
+          }
         }
       }
     });
-    adam = env.store.peekRecord('person', 1);
   });
 
-  run(function() {
+  var rematerializedPerson = bob.get('person');
+  assert.equal(rematerializedPerson.get('id'), '1');
+  // the person is rematerialized; the previous person is *not* re-used
+  assert.notEqual(rematerializedPerson, adam, 'the person is rematerialized, not recycled');
+});
+
+
+test('unloading a disconnected subgraph clears the relevant internal models', function(assert) {
+  env.adapter.shouldBackgroundReloadRecord = () => false;
+
+  run(() => {
     env.store.push({
       data: {
-        type: 'group',
+        type: 'person',
         id: '1',
+        attributes: {
+          name: 'Could be Anybody'
+        },
         relationships: {
-          people: {
+          cars: {
             data: [
-              { type: 'person', id: '1' }
+              { type: 'car', id: '1' },
+              { type: 'car', id: '2' }
             ]
           }
         }
       }
     });
-    bob = env.store.peekRecord('group', 1);
   });
 
-  run(function() {
-    env.store.findRecord('group', 1).then(function(group) {
-      assert.equal(group.get('people.length'), 1, 'The inital length of people is correct');
-      var person = env.store.peekRecord('person', 1);
-      run(function() {
-        person.unloadRecord();
-      });
-
-      assert.equal(group.get('people.length'), 0, 'Person was removed from the people array');
+  run(() => {
+    env.store.push({
+      data: {
+        type: 'car',
+        id: '1',
+        attributes: {
+          make: 'Nissan',
+          model: 'Altima'
+        },
+        relationships: {
+          person: {
+            data: { type: 'person', id: '1' }
+          }
+        }
+      }
+    });
+  });
+
+  run(() => {
+    env.store.push({
+      data: {
+        type: 'car',
+        id: '2',
+        attributes: {
+          make: 'Tesla',
+          model: 'S'
+        },
+        relationships: {
+          person: {
+            data: { type: 'person', id: '1' }
+          }
+        }
+      }
     });
   });
+
+  assert.equal(env.store.typeMapFor(env.store.modelFor('person')).records.length, 1);
+  assert.equal(env.store.typeMapFor(env.store.modelFor('car')).records.length, 2);
+
+  run(() => {
+    env.store.peekRecord('person', 1).unloadRecord();
+    env.store.peekRecord('car', 1).unloadRecord();
+    env.store.peekRecord('car', 2).unloadRecord();
+  });
+
+  assert.equal(env.store.typeMapFor(env.store.modelFor('person')).records.length, 0);
+  assert.equal(env.store.typeMapFor(env.store.modelFor('car')).records.length, 0);
 });
diff --git a/tests/integration/relationships/has-many-test.js b/tests/integration/relationships/has-many-test.js
index 427b0f77ab2..eb55fddab17 100644
--- a/tests/integration/relationships/has-many-test.js
+++ b/tests/integration/relationships/has-many-test.js
@@ -2787,7 +2787,7 @@ test("unloading and reloading a record with hasMany relationship - #3084", funct
 
     user = env.store.peekRecord('user', 'user-1');
 
-    assert.equal(get(user, 'messages.firstObject.id'), 'message-1');
-    assert.equal(get(message, 'user.id'), 'user-1');
+    assert.equal(get(user, 'messages.firstObject.id'), 'message-1', 'user points to message');
+    assert.equal(get(message, 'user.id'), 'user-1', 'message points to user');
   });
 });
diff --git a/tests/integration/store-test.js b/tests/integration/store-test.js
index c21a4e2c86b..c5767002db5 100644
--- a/tests/integration/store-test.js
+++ b/tests/integration/store-test.js
@@ -197,11 +197,8 @@ test("destroying the store correctly cleans everything up", function(assert) {
   assert.equal(car.get('person'), person, "expected car's person to be the correct person");
   assert.equal(person.get('cars.firstObject'), car, " expected persons cars's firstRecord to be the correct car");
 
-  Ember.run(person, person.destroy);
   Ember.run(store, 'destroy');
 
-  assert.equal(car.get('person'), null, "expected car.person to no longer be present");
-
   assert.equal(personWillDestroy.called.length, 1, 'expected person to have recieved willDestroy once');
   assert.equal(carWillDestroy.called.length, 1, 'expected car to recieve willDestroy once');
   assert.equal(carsWillDestroy.called.length, 1, 'expected cars to recieve willDestroy once');
diff --git a/tests/unit/store/unload-test.js b/tests/unit/store/unload-test.js
index e3d451420f2..0470864be2a 100644
--- a/tests/unit/store/unload-test.js
+++ b/tests/unit/store/unload-test.js
@@ -49,7 +49,7 @@ testInDebug("unload a dirty record asserts", function(assert) {
       }
     });
 
-    store.findRecord('record', 1).then(function(record) {
+    store.findRecord('record', 1, { backgroundReload: false }).then(function(record) {
       record.set('title', 'toto2');
       record._internalModel.send('willCommit');
 
@@ -67,8 +67,8 @@ testInDebug("unload a dirty record asserts", function(assert) {
   });
 });
 
-test("unload a record", function(assert) {
-  assert.expect(5);
+test('unload a record', function(assert) {
+  assert.expect(2);
 
   run(function() {
     store.push({
@@ -82,15 +82,11 @@ test("unload a record", function(assert) {
     });
     store.findRecord('record', 1).then(function(record) {
       assert.equal(get(record, 'id'), 1, "found record with id 1");
-      assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty");
 
       run(function() {
         store.unloadRecord(record);
       });
 
-      assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty");
-      assert.equal(get(record, 'isDeleted'), true, "record is deleted");
-
       tryToFind = false;
       return store.findRecord('record', 1).then(function() {
         assert.equal(tryToFind, true, "not found record with id 1");
@@ -105,25 +101,25 @@ module("DS.Store - unload record with relationships");
 test("can commit store after unload record with relationships", function(assert) {
   assert.expect(1);
 
-  var like, product;
+  let like, product;
 
-  var Brand = DS.Model.extend({
+  let Brand = DS.Model.extend({
     name: DS.attr('string')
   });
   Brand.reopenClass({ toString: () => 'Brand'});
 
-  var Product = DS.Model.extend({
+  let Product = DS.Model.extend({
     description: DS.attr('string'),
     brand: DS.belongsTo('brand', { async: false })
   });
   Product.reopenClass({ toString: () => 'Product'});
 
-  var Like = DS.Model.extend({
+  let Like = DS.Model.extend({
     product: DS.belongsTo('product', { async: false })
   });
   Like.reopenClass({ toString: () => 'Like'});
 
-  var store = createStore({
+  let store = createStore({
     adapter: DS.Adapter.extend({
       findRecord(store, type, id, snapshot) {
         return Ember.RSVP.resolve({ id: 1, description: 'cuisinart', brand: 1 });
@@ -136,9 +132,9 @@ test("can commit store after unload record with relationships", function(assert)
     product: Product,
     like: Like
   });
-  var asyncRecords;
+  let asyncRecords;
 
-  run(function() {
+  let promise = run(function() {
     store.push({
       data: [{
         type: 'brand',
@@ -162,18 +158,23 @@ test("can commit store after unload record with relationships", function(assert)
 
     asyncRecords = Ember.RSVP.hash({
       brand: store.findRecord('brand', 1),
-      product: store.findRecord('product', 1)
+      product: store.findRecord('product', 1, { backgroundReload: false })
     });
-    asyncRecords.then(function(records) {
+
+    return asyncRecords.then(function(records) {
       like = store.createRecord('like', { id: 1, product: product });
       records.like = like.save();
       return Ember.RSVP.hash(records);
     }).then(function(records) {
       store.unloadRecord(records.product);
+    });
+  });
+
+  run(function () {
+    promise.then(function () {
       return store.findRecord('product', 1);
-    }).then(function(product) {
+    }).then(function (product) {
       assert.equal(product.get('description'), 'cuisinart', "The record was unloaded and the adapter's `findRecord` was called");
-      store.destroy();
     });
   });
 });