Skip to content

Commit

Permalink
Free up internalModels
Browse files Browse the repository at this point in the history 
`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]
  • Loading branch information
@hjdivad
hjdivad committed Jan 16, 2017
1 parent ae4aa84 commit eb71a9e
Show file tree
Hide file tree
Showing 21 changed files with 624 additions and 179 deletions.
162 changes: 146 additions & 16 deletions addon/-private/system/model/internal-model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ function extractPivotName(name) {
);
}

function areAllModelsUnloaded(internalModels) {
for (let i=0; i<internalModels.length; ++i) {
let record = internalModels[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 {
Expand Down Expand Up @@ -88,6 +98,7 @@ const {
);

let InternalModelReferenceId = 1;
let nextBfsId = 1;

/*
`InternalModel` is the Model class that we use internally inside Ember Data to represent models.
Expand All @@ -113,26 +124,27 @@ export default class InternalModel {
this.store = store;
this._data = data || new EmptyObject();
this.modelName = modelName;
this.dataHasInitialized = false;
this._loadingPromise = null;
this._record = null;
this.currentState = RootState.empty;
this.isReloading = false;
this._isDestroyed = false;
this.isError = false;
this.error = null;
this._isUpdatingRecordArrays = false;
this._isDematerializing = false;

this.resetRecord();

// caches for lazy getters
this._modelClass = null;
this.__deferredTriggers = null;
this.__recordArrays = null;
this._references = null;
this._recordReference = null;
this.__inFlightAttributes = null;
this.__relationships = null;
this.__attributes = null;
this.__implicitRelationships = null;

// Used during the mark phase of unloading to avoid checking the same internal
// model twice in the same scan
this._bfsId = 0;
}

get modelClass() {
Expand All @@ -145,7 +157,7 @@ export default class InternalModel {

get recordReference() {
if (this._recordReference === null) {
this._recordReference = new RecordReference(this.store, this)
this._recordReference = new RecordReference(this.store, this);
}
return this._recordReference;
}
Expand Down Expand Up @@ -275,8 +287,9 @@ export default class InternalModel {
return this._record;
}

// TODO: fix the wat of 'get record' and 'getRecord'
getRecord() {
if (!this._record) {
if (!this._record && !this._isDematerializing) {
heimdall.increment(materializeRecord);
let token = heimdall.start('InternalModel.getRecord');

Expand Down Expand Up @@ -307,8 +320,26 @@ export default class InternalModel {
return this._record;
}

recordObjectWillDestroy() {
resetRecord() {
// TODO: get laziness working again?
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._isDematerializing = true;
this.record.destroy();
this.destroyRelationships();
// TODO: notify other side to clear cp for belongs to
// TODO: test/investigate that things work in the hasMany case
this.updateRecordArrays();
}
}

deleteRecord() {
Expand Down Expand Up @@ -356,14 +387,108 @@ export default class InternalModel {
});
}

/**
Computes the set of internal models reachable from `this` across exactly one
relationship.
@return {Array} An array containing the internal models that `this` belongs
to or has many.
*/
_directlyRelatedInternalModels() {
let array = [];
this.type.eachRelationship((key, relationship) => {
if (this._relationships.has(key)) {
let relationship = this._relationships.get(key);
let localRelationships = relationship.members.toArray();
let serverRelationships = relationship.canonicalMembers.toArray();

array = array.concat(localRelationships, serverRelationships);
}
});
return array;
}


/**
Computes the set of internal models reachable from this internal model.
Reachability is determined over the relationship graph (ie a graph where
nodes are internal models and edges are belongs to or has many
relationships).
@return {Array} An array including `this` and all internal models reachable
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 the record to be
destroyed and freed up for garbage collection. It will also do a check
for cleaning up internal models.
This check is performed by first computing the set of related internal
models. If all records in this set are unloaded, then the entire set is
destroyed. Otherwise, nothing in the set is destroyed.
This means that this internal model will be freed up for garbage collection
once all models that refer to it via some relationship are also unloaded.
*/
unloadRecord() {
this.send('unloadRecord');
this.dematerializeRecord();
Ember.run.schedule('destroy', this, '_checkForOrphanedInternalModels');
}

_checkForOrphanedInternalModels() {
this._isDematerializing = false;
if (this.isDestroyed) { return; }

this._cleanupOrphanedInternalModels();
}

_cleanupOrphanedInternalModels() {
let relatedInternalModels = this._allRelatedInternalModels();
if (areAllModelsUnloaded(relatedInternalModels)) {
for (let i=0; i<relatedInternalModels.length; ++i) {
let internalModel = relatedInternalModels[i];
if (!internalModel.isDestroyed) {
internalModel.destroy();
}
}
}
}

eachRelationship(callback, binding) {
return this.modelClass.eachRelationship(callback, binding);
}

destroy() {
assert("Cannot destroy an internalModel while its record is materialized", !this.record || this.record.get('isDestroyed') || this.record.get('isDestroying'));

this.store._removeFromIdMap(this);
this._isDestroyed = true;
}

eachAttribute(callback, binding) {
return this.modelClass.eachAttribute(callback, binding);
}
Expand Down Expand Up @@ -402,13 +527,6 @@ export default class InternalModel {
return !!this._record;
}

destroy() {
this._isDestroyed = true;
if (this.hasRecord) {
return this.record.destroy();
}
}

/*
@method createSnapshot
@private
Expand Down Expand Up @@ -718,6 +836,18 @@ export default class InternalModel {
});
}

destroyRelationships() {
this.eachRelationship((name, relationship) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);
rel.destroy();
}
});
Object.keys(this._implicitRelationships).forEach((key) => {
this._implicitRelationships[key].destroy();
});
}

/*
When a find request is triggered on the store, the user can optionally pass in
attributes and relationships to be preloaded. These are meant to behave as if they
Expand Down
5 changes: 1 addition & 4 deletions addon/-private/system/model/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,11 +808,8 @@ const 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 is a temporary solution until we refactor DS.Model to not
Expand Down
10 changes: 0 additions & 10 deletions addon/-private/system/model/states.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() { },
Expand Down Expand Up @@ -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() {},
Expand Down Expand Up @@ -680,8 +672,6 @@ const RootState = {

setup(internalModel) {
internalModel.clearRelationships();
var store = internalModel.store;
store._dematerializeRecord(internalModel);
},

invokeLifecycleCallbacks(internalModel) {
Expand Down
7 changes: 5 additions & 2 deletions addon/-private/system/record-array-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,11 @@ export default class RecordArrayManager {
for (let i = 0, l = updated.length; i < l; i++) {
let internalModel = updated[i];

if (internalModel.isDestroyed ||
internalModel.currentState.stateName === 'root.deleted.saved') {
// TODO: leave a breadcrumb here explaining the pain caused by the
// firstObject, lastObject thing.
if (internalModel._isDematerializing ||
internalModel.isDestroyed ||
internalModel.currentState.stateName === 'root.deleted.saved') {
this._recordWasDeleted(internalModel);
} else {
this._recordWasChanged(internalModel);
Expand Down
1 change: 0 additions & 1 deletion addon/-private/system/record-map.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ export default class RecordMap {
for (let i = 0; i < records.length; i++) {
record = records[i];
record.unloadRecord();
record.destroy(); // maybe within unloadRecord
}
}

Expand Down
12 changes: 10 additions & 2 deletions addon/-private/system/relationships/state/belongs-to.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ export default class BelongsToRelationship extends Relationship {
super.addCanonicalRecord(newRecord);
}

inverseDidDematerialize() {
this.notifyBelongsToChanged();
}

flushCanonical() {
//temporary fix to not remove newly created records if server returned null.
//TODO remove once we have proper diffing
Expand All @@ -54,7 +58,7 @@ export default class BelongsToRelationship extends Relationship {
}
if (this.inverseRecord !== this.canonicalState) {
this.inverseRecord = this.canonicalState;
this.internalModel.notifyBelongsToChanged(this.key);
this.notifyBelongsToChanged();
}

super.flushCanonical();
Expand All @@ -71,7 +75,7 @@ export default class BelongsToRelationship extends Relationship {

this.inverseRecord = newRecord;
super.addRecord(newRecord);
this.internalModel.notifyBelongsToChanged(this.key);
this.notifyBelongsToChanged();
}

setRecordPromise(newPromise) {
Expand All @@ -84,6 +88,10 @@ export default class BelongsToRelationship extends Relationship {
if (!this.members.has(record)) { return;}
this.inverseRecord = null;
super.removeRecordFromOwn(record);
this.notifyBelongsToChanged();
}

notifyBelongsToChanged() {
this.internalModel.notifyBelongsToChanged(this.key);
}

Expand Down
10 changes: 10 additions & 0 deletions addon/-private/system/relationships/state/has-many.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ export default class ManyRelationship extends Relationship {
}

destroy() {
super.destroy();
if (this._manyArray) {
this._manyArray.destroy();
this._manyArray = null;
}
}

Expand All @@ -54,6 +56,14 @@ export default class ManyRelationship extends Relationship {
super.addCanonicalRecord(record, idx);
}

inverseDidDematerialize() {
if (this._manyArray) {
this._manyArray.destroy();
this._manyArray = null;
}
this.notifyHasManyChanged();
}

addRecord(record, idx) {
if (this.members.has(record)) {
return;
Expand Down
Loading

0 comments on commit eb71a9e

Please sign in to comment.