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

[PERF] optimise notifications when has-many array is changed #4583

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ node_modules/
bower_components/
.metadata_never_index
npm-debug.log
.vscodeignore
Copy link
Member

@stefanpenner stefanpenner Oct 21, 2016

Choose a reason for hiding this comment

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

unsure if this should be added here, typically editor specific things are not included.

Copy link
Member

Choose a reason for hiding this comment

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

please remove

97 changes: 68 additions & 29 deletions addon/-private/system/many-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,16 @@ const { get, set } = Ember;
@uses Ember.MutableArray, Ember.Evented
*/
export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
record: null,

canonicalState: null,
currentState: null,

length: 0,

init() {
this._super(...arguments);

this.currentState = Ember.A([]);
Copy link
Member

Choose a reason for hiding this comment

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

why Ember.A ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we need array observer and app might not have [] mapped to Ember.A?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a change on master btw - I've merged it into my branch because my branch started so long ago

/**
The loading state of this array

Expand Down Expand Up @@ -141,31 +148,64 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
return this.currentState[index].getRecord();
},

flushCanonical(isInitialized = true) {
let toSet = this.canonicalState;
flushCanonical() {
Copy link
Member

Choose a reason for hiding this comment

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

isInitialized no longer needed?

// It’s possible the parent side of the relationship may have been unloaded by this point
if (!_objectIsAlive(this)) {
return;
}
//TODO make this smarter, currently its plenty stupid
let toSet = this.canonicalState.filter((internalModel) => !internalModel.isDeleted());

//a hack for not removing new records
//TODO remove once we have proper diffing
let newRecords = this.currentState.filter(
const newRecords = this.currentState.filter(
// only add new records which are not yet in the canonical state of this
// relationship (a new record can be in the canonical state if it has
// been 'acknowleged' to be in the relationship via a store.push)
(internalModel) => internalModel.isNew() && toSet.indexOf(internalModel) === -1
);
toSet = toSet.concat(newRecords);
let oldLength = this.length;
this.arrayContentWillChange(0, this.length, toSet.length);
// It’s possible the parent side of the relationship may have been unloaded by this point
if (_objectIsAlive(this)) {
this.set('length', toSet.length);
const oldLength = this.length;
Copy link
Member

Choose a reason for hiding this comment

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

what follows is relatively complicated, could it be extracted to its own "pure" function so it can be thoroughly unit tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const newLength = toSet.length;

const shortestLength = Math.min(oldLength, newLength);

let firstChangeIndex = -1; // -1 signifies no changes
// find the first change
const currentArray = this.currentState;
for (let i=0; i<shortestLength; i++) {
// compare each item in the array
if (currentArray[i] !== toSet[i]) {
firstChangeIndex = i;
break;
}
}
this.currentState = toSet;
this.arrayContentDidChange(0, oldLength, this.length);

if (isInitialized) {
//TODO Figure out to notify only on additions and maybe only if unloaded
if (firstChangeIndex === -1) {
// no change found in the matching part of the arrays
Copy link
Member

Choose a reason for hiding this comment

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

this change is not needed, the scoped let was appropriate.

if (newLength !== oldLength) {
firstChangeIndex = shortestLength;
}
}
if (firstChangeIndex !== -1) {
// we found a change, find the end of the change
let unchangedEndBlockLength = 0;
// walk back from the end of both arrays until we find a change
for (let i=1; i<shortestLength; i++) {
// compare each item in the array
if (currentArray[oldLength-i] !== toSet[newLength-i]) {
unchangedEndBlockLength = i-1;
break;
}
}
const added = newLength - unchangedEndBlockLength - firstChangeIndex;
const removed = oldLength - unchangedEndBlockLength - firstChangeIndex;
this.arrayContentWillChange(firstChangeIndex, removed, added);
Copy link
Member

Choose a reason for hiding this comment

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

if nothing changed, should be bail out here entirely? Seems like we don't want to emit any events in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

set(this, 'length', toSet.length);
Copy link
Member

Choose a reason for hiding this comment

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

this.set(this, 'length', toSet.length)

this.currentState = toSet;
this.arrayContentDidChange(firstChangeIndex, removed, added);
this.relationship.notifyHasManyChanged();
}
this.record.updateRecordArrays();
Copy link
Member

Choose a reason for hiding this comment

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

if there is no change found, why invoke updateRecordArrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that's where the canonical was updated. I'll go and check again

},

internalReplace(idx, amt, objects) {
Expand All @@ -174,14 +214,15 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
}
this.arrayContentWillChange(idx, amt, objects.length);
this.currentState.splice.apply(this.currentState, [idx, amt].concat(objects));
this.set('length', this.currentState.length);
set(this, 'length', this.currentState.length);
Copy link
Member

Choose a reason for hiding this comment

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

this change seems unneeded

this.arrayContentDidChange(idx, amt, objects.length);
},

//TODO(Igor) optimize
internalRemoveRecords(records) {
let index;
for (let i=0; i < records.length; i++) {
let index = this.currentState.indexOf(records[i]);
index = this.currentState.indexOf(records[i]);
this.internalReplace(index, 1);
}
},
Expand All @@ -195,13 +236,12 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
},

replace(idx, amt, objects) {
let records;
if (amt > 0) {
records = this.currentState.slice(idx, idx+amt);
this.get('relationship').removeRecords(records);
const records = this.currentState.slice(idx, idx+amt);
get(this, 'relationship').removeRecords(records);
Copy link
Member

Choose a reason for hiding this comment

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

this.get('relationship') is fine, no need to change.

}
if (objects) {
this.get('relationship').addRecords(objects.map(obj => obj._internalModel), idx);
get(this, 'relationship').addRecords(objects.map(obj => obj._internalModel), idx);
Copy link
Member

Choose a reason for hiding this comment

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

this.get('relationship') is fine, no need to change.

}
},

Expand Down Expand Up @@ -273,10 +313,10 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
@return {DS.PromiseArray} promise
*/
save() {
let manyArray = this;
let promiseLabel = 'DS: ManyArray#save ' + get(this, 'type');
let promise = Ember.RSVP.all(this.invoke("save"), promiseLabel).
then(() => manyArray, null, 'DS: ManyArray#save return ManyArray');
const manyArray = this;
const promiseLabel = `DS: ManyArray#save ${get(this, 'type')}`;
const promise = Ember.RSVP.all(this.invoke("save"), promiseLabel)
.then(() => manyArray, null, "DS: ManyArray#save return ManyArray");

return PromiseArray.create({ promise });
},
Expand All @@ -290,12 +330,11 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
@return {DS.Model} record
*/
createRecord(hash) {
let store = get(this, 'store');
let type = get(this, 'type');
let record;

const type = get(this, 'type');
assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !get(this, 'isPolymorphic'));
record = store.createRecord(type.modelName, hash);

const store = get(this, 'store');
const record = store.createRecord(type.modelName, hash);
this.pushObject(record);

return record;
Expand Down
Loading