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 10 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

58 changes: 58 additions & 0 deletions addon/-private/system/diff-array.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
@namespace
@method diff-array
@for DS
@param {Array} oldArray the old array
@param {Array} newArray the new array
@return {hash} {
firstChangeIndex: <integer>, // null if no change
addedCount: <integer>, // 0 if no change
removedCount: <integer> // 0 if no change
}
*/
export default function (oldArray, newArray) {
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 give this method a name, so future travelers debugging can easily identify it in the call stack

const oldLength = oldArray.length;
const newLength = newArray.length;

const shortestLength = Math.min(oldLength, newLength);
let firstChangeIndex = null; // null signifies no changes

// find the first change
for (let i=0; i<shortestLength; i++) {
// compare each item in the array
if (oldArray[i] !== newArray[i]) {
firstChangeIndex = i;
break;
}
}

if (firstChangeIndex === null && newLength !== oldLength) {
// no change found in the overlapping block
// and array lengths differ,
// so change starts at end of overlap
firstChangeIndex = shortestLength;
}

let addedCount = 0;
let removedCount = 0;
if (firstChangeIndex !== null) {
// we found a change, find the end of the change
let unchangedEndBlockLength = shortestLength - firstChangeIndex;
// 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 (oldArray[oldLength-i] !== newArray[newLength-i]) {
unchangedEndBlockLength = i-1;
break;
}
}
addedCount = newLength - unchangedEndBlockLength - firstChangeIndex;
removedCount = oldLength - unchangedEndBlockLength - firstChangeIndex;
}

return {
firstChangeIndex,
addedCount,
removedCount
};
}
71 changes: 42 additions & 29 deletions addon/-private/system/many-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { assert } from "ember-data/-private/debug";
import { PromiseArray } from "ember-data/-private/system/promise-proxies";
import { _objectIsAlive } from "ember-data/-private/system/store/common";

import diffArray from './diff-array';

const { get, set } = Ember;

/**
Expand Down Expand Up @@ -52,9 +54,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 +150,36 @@ 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);
}
this.currentState = toSet;
this.arrayContentDidChange(0, oldLength, this.length);

if (isInitialized) {
//TODO Figure out to notify only on additions and maybe only if unloaded
// diff to find changes
const diff = diffArray(this.currentState, toSet);

if (diff.firstChangeIndex !== null) { // it's null if no change found
// we found a change
this.arrayContentWillChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount);
Copy link
Member

Choose a reason for hiding this comment

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

when is diff.firstChangeIndex falsey and not 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - that's left over from some testing

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(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount);
Copy link
Member

Choose a reason for hiding this comment

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

^

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

},

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.

internalReplace(idx, amt, objects) {
Expand All @@ -174,14 +188,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 +210,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 +287,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 +304,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
1 change: 1 addition & 0 deletions jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"compilerOptions":{"target":"es6","experimentalDecorators":true},"exclude":["node_modules","bower_components","tmp","vendor",".git","dist"]}
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 want to add this, this should be a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Loading