Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free up internalModels #4593

Merged
merged 1 commit into from
Jan 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 166 additions & 17 deletions addon/-private/system/model/internal-model.js
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 @@ -111,28 +122,38 @@ export default class InternalModel {
this.id = id;
this._internalId = InternalModelReferenceId++;
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;

// During dematerialization we don't want to rematerialize the record. The
// reason this might happen is that dematerialization removes records from
// record arrays, and Ember arrays will always `objectAt(0)` and
// `objectAt(len - 1)` to test whether or not `firstObject` or `lastObject`
// have changed.
this._isDematerializing = false;
Copy link
Member

Choose a reason for hiding this comment

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

Same here


this.resetRecord();

if (data) {
this.__data = data;
}

// 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 +166,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 @@ -201,6 +222,17 @@ export default class InternalModel {
this.__inFlightAttributes = v;
}

get _data() {
if (this.__data === null) {
this.__data = new EmptyObject();
}
return this.__data;
}

set _data(v) {
this.__data = v;
}

/*
implicit relationships are relationship which have not been declared but the inverse side exists on
another record somewhere
Expand Down Expand Up @@ -276,7 +308,7 @@ export default class InternalModel {
}

getRecord() {
if (!this._record) {
if (!this._record && !this._isDematerializing) {
Copy link
Member

Choose a reason for hiding this comment

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

We should comment this

heimdall.increment(materializeRecord);
let token = heimdall.start('InternalModel.getRecord');

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

recordObjectWillDestroy() {
resetRecord() {
this._record = null;
this.dataHasInitialized = false;
this.isReloading = false;
this.error = null;
this.currentState = RootState.empty;
this.__attributes = null;
this.__inFlightAttributes = null;
this._data = null;
}

dematerializeRecord() {
if (this.record) {
Copy link
Member

Choose a reason for hiding this comment

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

when do we call dematerializeRecord and there is no record?

Copy link
Member Author

Choose a reason for hiding this comment

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

store.peekRecord('person', 1).unloadRecord(); // internal model stays around b/c of relationship

store.unloadAll('person'); // store.unloadAll(type) is suspect but it is public API

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

this._isDematerializing = true;
this.record.destroy();
this.destroyRelationships();
this.updateRecordArrays();
this.resetRecord();
}
}

deleteRecord() {
Expand Down Expand Up @@ -356,14 +405,109 @@ 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) => {
Copy link
Member

Choose a reason for hiding this comment

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

we may want to use eachRelationshipByName cp here:

relationshipsByName: relationshipsByNameDescriptor,
because it is cached (eachRelBy Name can be costly), and seems like its provides what we want here.

Copy link
Member

Choose a reason for hiding this comment

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

we should benchmark

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];
assert('Internal Error: seen a future bfs iteration', internalModel._bfsId <= bfsId);
if (internalModel._bfsId < bfsId) {
queue.push(internalModel);
internalModel._bfsId = bfsId;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
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;
Copy link
Member

Choose a reason for hiding this comment

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

Should comment on why we do this here

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;
Copy link
Member

Choose a reason for hiding this comment

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

Should we set isDestroying to false?

Copy link
Member

Choose a reason for hiding this comment

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

If we still need it

}

eachAttribute(callback, binding) {
return this.modelClass.eachAttribute(callback, binding);
}
Expand Down Expand Up @@ -402,13 +546,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 +855,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
8 changes: 0 additions & 8 deletions addon/-private/system/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -807,14 +807,6 @@ const Model = Ember.Object.extend(Ember.Evented, {
this._super(...arguments);
},

willDestroy() {
//TODO Move!
this._super(...arguments);
this._internalModel.clearRelationships();
this._internalModel.recordObjectWillDestroy();
//TODO should we set internalModel to null here?
},

// This is a temporary solution until we refactor DS.Model to not
// rely on the data property.
willMergeMixin(props) {
Expand Down
10 changes: 0 additions & 10 deletions addon/-private/system/model/states.js
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
11 changes: 9 additions & 2 deletions addon/-private/system/record-array-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,15 @@ 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') {
// During dematerialization we don't want to rematerialize the record.
// recordWasDeleted can cause other records to rematerialize because it
// removes the internal model from the array and Ember arrays will always
// `objectAt(0)` and `objectAt(len -1)` to check whether `firstObject` or
// `lastObject` have changed. When this happens we don't want those
// models to rematerialize their records.
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
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
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
Loading