Free up `internalModel`s #4593

hjdivad · 2016-10-19T14:56:49Z

internalModels 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]

hjdivad · 2016-10-19T16:04:39Z

addon/-private/system/model/states.js

+        // var store = internalModel.store;
+        // store._dematerializeRecord(internalModel);
+
+        // TODO: internalModel.dematerializeRecord() ?


@igorT do we actually want to dematerialize on deleted.saved? seems suspect

per our discussion offline we'll unload the record (which will dematerialize)

@igorT a consequence of this is that we'll destroy deleted records. This may be surprising for eg

let record = store.createRecord('whatever'); record.rollbackAttributes(); // => record is now destroyed

hjdivad · 2016-10-19T16:05:16Z

cc @stefanpenner @pangratz @igorT @sly7-7

stefanpenner · 2016-10-19T16:20:21Z

addon/-private/system/model/internal-model.js

+    }
+  },
+
+  destroy: function() {


concise function

stefanpenner · 2016-10-19T16:20:38Z

addon/-private/system/model/internal-model.js

@@ -324,6 +383,7 @@ InternalModel.prototype = {
    @private
  */
  pushedData() {
+    this._isDestroyed = false;


this revives internalModels ?

stefanpenner · 2016-10-19T16:21:10Z

addon/-private/system/model/model.js

-    this._internalModel.recordObjectWillDestroy();
-    //TODO should we set internalModel to null here?
+
+    this._internalModel._resetRecord();


is there a point to resetting the internal model here, doesn't it just "go dead" once destroyed?

No, the internal model lives on, the record goes dead

stefanpenner · 2016-10-19T16:21:45Z

tests/integration/adapter/find-all-test.js

@@ -19,6 +19,7 @@ module("integration/adapter/find_all - Finding All Records of a Type", {
      firstName: attr('string'),
      lastName: attr('string')
    });
+    Person.reopenClass({ toString: () => 'Person' });


concise function.

I am curious why this is needed. Which tests are failing without this changes? I would guess only tests wich check for assertions, right? Maybe we should only specify the toString in those tests...

@pangratz this has no effect on tests passing or failing; it makes debugging tests easier because you can inspect things and get output more useful than "Subclass of DS.Model"

stefanpenner · 2016-10-19T16:22:04Z

tests/integration/records/unload-test.js

@@ -17,16 +17,25 @@ var Person = DS.Model.extend({
  name: attr('string'),
  cars: hasMany('car', { async: false })
 });
+Person.reopenClass({
+  toString: () => 'Person'


stefanpenner · 2016-10-19T16:28:07Z

addon/-private/system/model/internal-model.js

+    return array;
+  },
+
+  unloadRecord: function() {


concise function

stefanpenner · 2016-10-19T16:29:10Z

addon/-private/system/model/internal-model.js

@@ -235,8 +241,68 @@ InternalModel.prototype = {
    return this.record;
  },

-  unloadRecord() {
+  directlyRelatedInternalModels() {
+    var array = [];


stefanpenner · 2016-10-19T16:33:07Z

addon/-private/system/model/internal-model.js

+      }
+    }
+    if (allUnloaded) {
+      relatedInternalModels.forEach((internalModel) => {


why for loop on 281, but forEach here

because of break

stefanpenner · 2016-10-19T16:33:24Z

addon/-private/system/model/internal-model.js

+  },
+
+  destroy: function() {
+    this._isDestroyed = true;


we should likely mimic embers flags:

isDestroying = true during distruction

isDestroyed = true once destroyed

stefanpenner · 2016-10-19T16:37:34Z

addon/-private/system/model/internal-model.js

+    this.type.eachRelationship((key, relationship) => {
+      if (this._relationships.has(key)) {
+        var related = this._relationships.get(key).members.toArray();
+        array.push(...related);


this may be fine, but there is an upper limit (in the low hundreds of thousands) that one can push onto an array.

stefanpenner · 2016-10-19T16:58:55Z

addon/-private/system/model/internal-model.js

+      let node = queue.shift();
+      array.push(node);
+      let related = node.directlyRelatedInternalModels();
+      related.forEach(function(internalModel) {


for loop, for perfy reasons

stefanpenner · 2016-10-19T17:05:14Z

addon/-private/system/model/internal-model.js

-  unloadRecord() {
+  directlyRelatedInternalModels() {
+    var array = [];
+    this.type.eachRelationship((key, relationship) => {


we should benchmark

igorT · 2016-10-19T16:34:31Z

addon/-private/system/model/internal-model.js

+    var relatedInternalModels = this.allRelatedInternalModels();
+    var allUnloaded = true;
+    var record;
+    for (var i=0; i < relatedInternalModels.length; i++) {


Can you break this method up a bit please

Can you write a comment documenting our mark & sweep algorithm

igorT · 2016-10-19T16:42:06Z

addon/-private/system/model/internal-model.js

+    return array;
+  },
+
+  allRelatedInternalModels() {


Can you write a short comment describing what this does

igorT · 2016-10-19T16:57:06Z

addon/-private/system/model/internal-model.js

@@ -324,6 +383,7 @@ InternalModel.prototype = {
    @private
  */
  pushedData() {
+    this._isDestroyed = false;


This seems wrong

igorT · 2016-10-19T16:59:12Z

addon/-private/system/model/internal-model.js

+  willDestroy() {
+    Ember.assert("Cannot destroy an internalModel while its record is materialized", !this.record);
+    this.store._removeFromIdMap(this);
+    //other cleanup


Other cleanup?

This is your comment.

What other cleanup did you have in mind?

igorT · 2016-10-19T17:03:25Z

addon/-private/system/model/model.js

-    this._internalModel.recordObjectWillDestroy();
-    //TODO should we set internalModel to null here?
+
+    this._internalModel._resetRecord();


No, the internal model lives on, the record goes dead

igorT · 2016-10-19T17:06:05Z

addon/-private/system/model/internal-model.js

@@ -584,6 +644,15 @@ InternalModel.prototype = {
    });
  },

+  _resetRecord() {


Can you put a comment saying that this doesn't reset relationships/attributes on purpose, given that constraint, the name is a bit unfortunate

i'll put in a comment, but even so, can you suggest a better name?

igorT · 2016-10-19T17:06:25Z

addon/-private/system/store.js

@@ -794,6 +794,7 @@ Store = Service.extend({

    var resolver = Ember.RSVP.defer('Fetching ' + typeClass + 'with id: ' + internalModel.id);
    var pendingFetchItem = {
+      // TODO: s/record/internalModel


igorT · 2016-10-19T17:09:18Z

addon/-private/system/store.js

@@ -2430,12 +2430,14 @@ Store = Service.extend({
    @param {InternalModel} internalModel
  */
  _dematerializeRecord(internalModel) {


Maybe move to the internalModel

igorT · 2016-10-19T17:41:55Z

tests/integration/records/unload-test.js

+});
+
+
+test('unloading a disconnected subgraph clears the relevant internal models', function(assert) {


Maybe a test for unloading a part of a connected graph doesn't do that?

stefanpenner · 2016-10-19T17:06:22Z

addon/-private/system/model/internal-model.js

+  allRelatedInternalModels() {
+    let array = [];
+    let queue = [];
+    let seen = [];


we should consider coloring algo instead of seen. Color via incrementing ID.

pangratz · 2016-10-19T18:52:41Z

addon/-private/system/model/internal-model.js

+  },
+
+  willDestroy() {
+    Ember.assert("Cannot destroy an internalModel while its record is materialized", !this.record);


import { assert } from "ember-data/-private/debug";

pangratz · 2016-10-19T19:01:42Z

tests/unit/store/unload-test.js

@@ -47,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) {


The store.findRecord is unnecessary for this test. Let's use store.peekRecord instead.

pangratz · 2016-10-19T19:03:24Z

tests/unit/store/unload-test.js

@@ -157,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 })


store.peekRecord

pangratz · 2016-10-19T19:07:00Z

tests/unit/store/unload-test.js

+  });
+
+  run(function () {
+    promise.then(function () {
      return store.findRecord('product', 1);


store.peekRecord

pangratz · 2016-10-19T19:08:14Z

addon/-private/system/model/internal-model.js

@@ -95,20 +95,18 @@ export default function InternalModel(type, id, store, _, data) {
  this.store = store;
  this._data = data || new EmptyObject();
  this.modelName = type.modelName;
-  this.dataHasInitialized = false;
+  this._loadingPromise = null;


Seems unused?

it is written to elsewhere; i have added it here to preserve shape.

Really though we should add a setter and not have other parts of the system write private fields

wecc · 2016-10-21T21:56:36Z

I think this issue would close #4126

hjdivad · 2016-10-22T08:48:38Z

I think this issue would close #4126

yes it should

hjdivad · 2016-10-22T17:07:46Z

appveyor build is complaining about downloading phantom. Just need someone to restart.

@pangratz this should resolve your concerns
@igorT this should resolve your concerns except for deleting implicitly unloading: we should do this in a separate effort.

@stefanpenner this should resolve your concerns except for eachRelationship vs eachRelationshipByName. That shouldn't block the PR; we can switch it after doing some benchmarks

bmac · 2016-10-24T15:08:32Z

This looks good to me. 👍

@hjdivad I think @igorT is the only one who can restart an AppVeyor build. So in the past we've been comfortable merged prs when AppVeyor is red and travis is green, if the reason for the AppVeyor failure has to do with a network error during install and there aren't any build changes in the pr.

pangratz · 2016-10-24T16:03:11Z

addon/-private/system/model/internal-model.js

+    Computes the set of internal models reachable from `this` acrosxactly one
+    relationship.
+
+    @return {Array} An array containing the internal models that `t` belongs


s/t/this/?

Also hjdivad#2

hjdivad · 2016-10-25T21:02:50Z

@bmac turns out I can also restart an AppVeyor build by writing comments with such atrocious spelling that @pangratz has to save me with a new commit.

stefanpenner · 2016-10-25T21:47:13Z

addon/-private/system/model/internal-model.js

    this.__implicitRelationships = null;
+
+    // Used for coloring during BFS


should likely explain just a-bit more. BFS doesn't help us understand why/where this is used. Likely would be enough to say "during marking phase of unloading"

stefanpenner · 2016-10-25T21:47:59Z

addon/-private/system/model/internal-model.js

@@ -278,8 +290,22 @@ export default class InternalModel {
    this._triggerDeferredTriggers();
  }

-  recordObjectWillDestroy() {
+  resetRecord() {
    this.record = null;


is there actual value in reseting the record during destroy, or can we just let it go dead.

@stefanpenner the goal is to at least do enough to ensure

The internal model doesn't keep the record alive

The internal model knows the record is dematerialized

The internal model is able to resurrect the record

stefanpenner · 2016-10-25T21:51:32Z

addon/-private/system/model/internal-model.js

+  }
+
+  dematerializeRecord() {
+    if (this.record) {


Curious, should we assert in development if we attempt to dematerialize something already dematerialized? Wouldn't it mean we are doing duplicate work?

The case where we wind up here with !this.record is store.unloadAll(type) which is public API.

We could change this to assert !!this.record and then tweak the store to only unload materialized internal models. thoughts?

stefanpenner · 2016-10-25T21:52:59Z

addon/-private/system/model/internal-model.js

+    let array = [];
+    this.type.eachRelationship((key, relationship) => {
+      if (this._relationships.has(key)) {
+        let related = this._relationships.get(key).members.toArray();


should we create this array, to just iterate then throw it away?

bmac · 2016-10-28T17:43:48Z

@hjdivad do you mind rebasing this pr?

bmac · 2016-11-11T18:03:07Z

ping @hjdivad

runspired · 2016-11-18T08:08:54Z

ping @hjdivad, if you don't have time (as I suspect you don't, hi office budy! 👋 ) just say so and I'll pull down your work and try to move it along.

trumb1mj · 2017-01-11T22:05:58Z

Any updates on this? I'm running into just this problem. Is there any brute force method to get by this until it has been fixed?

hjdivad · 2017-01-11T23:59:02Z

@runspired @bmac gonna try to get to it this weekend

@runspired i'll likely need to sync up with you re your recent changes in this area

runspired · 2017-01-12T01:08:16Z

@hjdivad I may have to come also crash at your place :P

@trumb1mj I don't think you are hitting this issue exactly but a different retention issue that's related.

That said, our intention is to make it so that internal models ARE kept if a relationship still has access to them, while your need is explicitly for them to be unloaded even when one does. David @igorT and myself will need to consider this case.

trumb1mj · 2017-01-12T02:15:04Z

@runspired is there any way to build my relationships to avoid this?

Also, I don't understand why deleting an object with a many-to-1 would cause any residual references to be left around--the other way makes a bit of sense but I still think we should be able to opt out and just whack things. Not disagreeing just not quite understanding.

igorT · 2017-01-15T21:50:50Z

addon/-private/system/model/internal-model.js

-    this.currentState = RootState.empty;
-    this.isReloading = false;
+    // TODO: see if we can clean this up
+    this._isDestroying = false;


Either way lets leave a comment on why/how it's used

igorT · 2017-01-15T21:50:59Z

addon/-private/system/model/internal-model.js

    this._isUpdatingRecordArrays = false;
+    this._isDematerializing = false;


igorT · 2017-01-15T22:48:00Z

addon/-private/system/model/internal-model.js

  getRecord() {
-    if (!this._record) {
+    if (!this._record && !this._isDematerializing) {


We should comment this

igorT · 2017-01-15T23:19:21Z

addon/-private/system/model/internal-model.js

+        if (internalModel._bfsId < bfsId) {
+          queue.push(internalModel);
+          internalModel._bfsId = bfsId;
+        }


would it be worth adding a dev assert if we ever see a case of internalModel._bfsId > bfsId ? That would mean bad things are happening

igorT · 2017-01-15T23:24:19Z

addon/-private/system/model/internal-model.js

+  }
+
+  _cleanupOrphanedInternalModels() {
+    this._isDematerializing = false;


Should comment on why we do this here

igorT · 2017-01-15T23:43:01Z

addon/-private/system/model/internal-model.js

+    assert("Cannot destroy an internalModel while its record materialized", !this.record || this.record.get('isDestroyed') || this.record.get('isDestroying'));
+
+    this.store._removeFromIdMap(this);
+    this._isDestroyed = true;


Should we set isDestroying to false?

If we still need it

igorT · 2017-01-15T23:43:45Z

addon/-private/system/model/model.js

-    this._internalModel.clearRelationships();
-    this._internalModel.recordObjectWillDestroy();
-    //TODO should we set internalModel to null here?
+    this._internalModel.resetRecord();


Seems odd to call this from the record into the internalModel. Can we move it somewhere else?

@igorT

`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. Finally - Add a lot of `toString`s to test classes, as a debugging convenience. - Minor test cleanup Includes work from @igorT, @sly7-7 and @pangratz [Fix emberjs#3296] [Supercede emberjs#3301]

Kerrick · 2017-05-05T22:20:02Z

Did this seem to leave anybody else with severe performance problems?

In previous versions of Ember Data it was relatively quick to store.unloadAll('my-model-type') when I had ~2500 of them loaded, but in Ember Data 2.12 it takes upwards of 25 seconds. (In case it's relevant, my model has 5 hasMany relationships, and 3 belongsTo relationships.)

stefanpenner · 2017-05-05T23:54:37Z

@Kerrick can you open a fresh issue and provide a reproduction. Sounds like something we should look into and that will make this actionable

tylerturdenpants · 2017-06-23T04:41:41Z

@Kerrick I'm also experiencing exactly the same issue. 2.12 was super fast. 2.13-2.15 is so slow at unloading the records that it seems to hang the browser. I've also noticed given certain relationships lead to a memory leak that quickly grows to the gigabyte range of memory in a minute or two and then crashes.

hjdivad force-pushed the better-unload branch from 78872d3 to 88c5287 Compare October 19, 2016 16:00

hjdivad commented Oct 19, 2016

View reviewed changes

hjdivad force-pushed the better-unload branch from 88c5287 to aad8d7a Compare October 19, 2016 16:09

stefanpenner reviewed Oct 19, 2016

View reviewed changes

igorT reviewed Oct 19, 2016

View reviewed changes

stefanpenner reviewed Oct 19, 2016

View reviewed changes

pangratz reviewed Oct 19, 2016

View reviewed changes

pangratz mentioned this pull request Oct 19, 2016

Revert "unloadRecord does not remove observers" #4594

Merged

hjdivad force-pushed the better-unload branch from 1ea007b to 2872536 Compare October 21, 2016 18:13

hjdivad force-pushed the better-unload branch from 2872536 to 53a22c2 Compare October 22, 2016 08:43

hjdivad force-pushed the better-unload branch 4 times, most recently from c2f3d8a to 7bd94eb Compare October 22, 2016 14:29

This was referenced Oct 22, 2016

Relationship data lost when a record is unloaded #3020

Closed

unloadRecord doesn't disconnect observers on the model #4126

Closed

pangratz reviewed Oct 24, 2016

View reviewed changes

stefanpenner reviewed Oct 25, 2016

View reviewed changes

hjdivad force-pushed the better-unload branch 3 times, most recently from c5f22fb to fd8d42b Compare January 15, 2017 21:49

igorT reviewed Jan 15, 2017

View reviewed changes

addon/-private/system/model/internal-model.js

this._isUpdatingRecordArrays = false;

this._isDematerializing = false;

Copy link

Member

igorT Jan 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

igorT reviewed Jan 15, 2017

View reviewed changes

hjdivad force-pushed the better-unload branch 3 times, most recently from fddb4cb to b387340 Compare January 16, 2017 06:19

hjdivad force-pushed the better-unload branch from b387340 to 2f18405 Compare January 16, 2017 06:26

igorT merged commit 6d96eda into emberjs:master Jan 16, 2017

hjdivad deleted the better-unload branch January 16, 2017 06:47

workmanw mentioned this pull request May 5, 2017

Unloading models can leave stale fragments w/ ember-data-2.13 adopted-ember-addons/ember-data-model-fragments#261

Closed

sudowork mentioned this pull request May 16, 2017

unloadRecord of child record causes hasMany relationship to be invalid using ember-data@2.13 #4986

Closed

workmanw mentioned this pull request Jun 21, 2017

FilteredRecordArray has null values after unloadRecord w/ 2.13 and newer. #5025

Closed

		});


		test('unloading a disconnected subgraph clears the relevant internal models', function(assert) {

		this.__implicitRelationships = null;

		// Used for coloring during BFS

		this._isUpdatingRecordArrays = false;
		this._isDematerializing = false;

Free up internalModels #4593

Free up internalModels #4593

Conversation

hjdivad commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjdivad commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wecc commented Oct 21, 2016

hjdivad commented Oct 22, 2016

hjdivad commented Oct 22, 2016

bmac commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjdivad commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmac commented Oct 28, 2016

bmac commented Nov 11, 2016

runspired commented Nov 18, 2016

trumb1mj commented Jan 11, 2017

hjdivad commented Jan 11, 2017

runspired commented Jan 12, 2017

trumb1mj commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kerrick commented May 5, 2017

stefanpenner commented May 5, 2017

Free up `internalModel`s #4593

Free up `internalModel`s #4593