Skip to content

Commit

Permalink
Optimise has many notifications (emberjs#4850)
Browse files Browse the repository at this point in the history 
* optimise has-many notifications

* bring in line with master

* bring in line with master

* exit flushCanonical early if not alive

* add test for non contiguous change

* niggles

* jshint fix

optimise changes to hasMany 10x improvement with 5000 records

add comments

optimise fixing up relationships that have been edited since the last canonical flush

clear _removedRecords every time
  • Loading branch information
@BryanCrotaz @runspired
BryanCrotaz authored and runspired committed Apr 22, 2018
1 parent 87be9b7 commit 34aef62
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 93 deletions.
1 change: 1 addition & 0 deletions addon/-private/system/diff-array.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
@namespace
@method diffArray
@private
@for DS
@param {Array} oldArray the old array
@param {Array} newArray the new array
@return {hash} {
Expand Down
85 changes: 73 additions & 12 deletions addon/-private/system/relationships/state/has-many.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import Relationship from './relationship';
import OrderedSet from '../../ordered-set';
import ManyArray from '../../many-array';

import diffArray from '../../diff-array';

export default class ManyRelationship extends Relationship {
constructor(store, internalModel, inverseKey, relationshipMeta) {
super(store, internalModel, inverseKey, relationshipMeta);
Expand All @@ -19,6 +21,7 @@ export default class ManyRelationship extends Relationship {
this._loadingPromise = null;
this._willUpdateManyArray = false;
this._pendingManyArrayUpdates = null;
this._removedInternalModels = null;
}

_updateLoadingPromise(promise, content) {
Expand Down Expand Up @@ -203,6 +206,11 @@ export default class ManyRelationship extends Relationship {
if (!this.members.has(internalModel)) {
return;
}
// remember removals so we can deal with them in computeChanges
if (!this._removedInternalModels) {
this._removedInternalModels = new OrderedSet();
}
this._removedInternalModels.add(internalModel);
super.removeInternalModelFromOwn(internalModel, idx);
// note that ensuring the many array is created, via `this.manyArray`
// (instead of `this._manyArray`) is intentional.
Expand Down Expand Up @@ -266,22 +274,75 @@ export default class ManyRelationship extends Relationship {
}

computeChanges(internalModels = []) {
let members = this.canonicalMembers;
let internalModelsToRemove = [];
let internalModelSet = setForArray(internalModels);

members.forEach(member => {
if (internalModelSet.has(member)) { return; }
let members = this.canonicalMembers.toArray();
let removedSinceLastTime = this._removedInternalModels;
this._removedInternalModels = null; // clear it for next time

let diff = diffArray(members, internalModels);

if (diff.firstChangeIndex === null) {
// no changes found
if (removedSinceLastTime) {
// records that have been removed since last compute will need their inverses to be corrected
for (let i = 0, l = internalModels.length; i < l; i++) {
let record = internalModels[i];
if (removedSinceLastTime.has(record)) {
this.flushCanonicalLater();
break;
}
}
}
return;
}
let removedMembersSet = setForArray(members.slice(diff.firstChangeIndex, diff.firstChangeIndex + diff.removedCount));
let changeBlockSet = setForArray(internalModels.slice(diff.firstChangeIndex, diff.firstChangeIndex + diff.addedCount));

internalModelsToRemove.push(member);
// remove members that were removed but not re-added
removedMembersSet.forEach(member => {
if (!changeBlockSet.has(member)) {
this.removeCanonicalInternalModel(member);
}
});

this.removeCanonicalInternalModels(internalModelsToRemove);
let flushCanonicalLater = false;

// --- deal with records before the change block
if (removedSinceLastTime) {
// records that have been removed since last compute will need their inverses to be corrected
for (let i = 0; i < diff.firstChangeIndex; i++) {
let record = internalModels[i];
if (removedSinceLastTime.has(record)) {
flushCanonicalLater = true;
break;
}
}
}
// --- deal with records in the change block
for (let i = diff.firstChangeIndex, l = diff.firstChangeIndex + diff.addedCount; i < l; i++) {
let record = internalModels[i];
if (members[i] !== record) {
if (removedMembersSet.has(record)) {
// this is a reorder
this.removeCanonicalInternalModel(record);
}
// reorder or insert
this.addCanonicalInternalModel(record, i);
}
}
// --- deal with records after the change block
if (!flushCanonicalLater && removedSinceLastTime) {
// records that have been removed since last compute will need their inverses to be corrected
for (let i = diff.firstChangeIndex + diff.addedCount, l = internalModels.length; i < l; i++) {
let record = internalModels[i];
if (removedSinceLastTime.has(record)) {
flushCanonicalLater = true;
break;
}
}
}

for (let i = 0, l = internalModels.length; i < l; i++) {
let internalModel = internalModels[i];
this.removeCanonicalInternalModel(internalModel);
this.addCanonicalInternalModel(internalModel, i);
if (flushCanonicalLater) {
this.flushCanonicalLater();
}
}

Expand Down
118 changes: 51 additions & 67 deletions tests/integration/records/relationship-changes-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { alias } from '@ember/object/computed';
import { run } from '@ember/runloop';
import EmberObject, { set, get } from '@ember/object';
import setupStore from 'dummy/tests/helpers/store';

import DS from 'ember-data';
import { module, test } from 'qunit';

Expand Down Expand Up @@ -533,32 +532,26 @@ test('Calling push with relationship triggers willChange and didChange with deta
}
};

run(() => {
store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling1Ref]
}
}
let person = run(() => store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
included: [
sibling1
]

});
});

relationships: {
siblings: {
data: [sibling1Ref]
}
}
},
included: [
sibling1
]
}));

let person = store.peekRecord('person', 'wat');
let siblings = run(() => person.get('siblings'));

siblings.addArrayObserver(observer);

run(() => {
Expand Down Expand Up @@ -589,31 +582,26 @@ test('Calling push with relationship triggers willChange and didChange with deta
test('Calling push with relationship triggers willChange and didChange with detail when truncating', function(assert) {
let willChangeCount = 0;
let didChangeCount = 0;

run(() => {
store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling1Ref, sibling2Ref]
}
}
let person = run(() => store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
included: [
sibling1, sibling2
]
});
});
relationships: {
siblings: {
data: [sibling1Ref, sibling2Ref]
}
}
},
included: [
sibling1, sibling2
]
}));

let person = store.peekRecord('person', 'wat');
let siblings = run(() => person.get('siblings'));

let observer = {
arrayWillChange(array, start, removing, adding) {
willChangeCount++;
Expand Down Expand Up @@ -658,28 +646,24 @@ test('Calling push with relationship triggers willChange and didChange with deta
test('Calling push with relationship triggers willChange and didChange with detail when inserting at front', function(assert) {
let willChangeCount = 0;
let didChangeCount = 0;

run(() => {
store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling2Ref]
}
}
let person = run(() => store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
included: [
sibling2
]
});
});
let person = store.peekRecord('person', 'wat');
relationships: {
siblings: {
data: [sibling2Ref]
}
}
},
included: [
sibling2
]
}));

let observer = {
arrayWillChange(array, start, removing, adding) {
Expand Down
1 change: 0 additions & 1 deletion tests/unit/diff-array-test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { module, test } from 'qunit';

import { diffArray } from 'ember-data/-private';

module('unit/diff-array Diff Array tests', {
Expand Down
27 changes: 14 additions & 13 deletions tests/unit/model/relationships/has-many-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1611,7 +1611,7 @@ test('hasMany.firstObject.unloadRecord should not break that hasMany', function(
the parent record's hasMany is a situation in which this limitation will be encountered should other
local changes to the relationship still exist.
*/
test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship info from a delete clears local state', function(assert) {
test('returning new hasMany relationship info from a delete maintains a local addition', function(assert) {
assert.expect(4);

const Person = DS.Model.extend({
Expand Down Expand Up @@ -1642,7 +1642,7 @@ test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship
relationships: {
pets: {
data: [
{ type: 'pet', id: '2' }
{ type: 'pet', id: 'server-known-2' }
]
}
}
Expand All @@ -1664,30 +1664,30 @@ test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship
relationships: {
pets: {
data: [
{ type: 'pet', id: '1' },
{ type: 'pet', id: '2' }
{ type: 'pet', id: 'will-destroy-1' },
{ type: 'pet', id: 'server-known-2' }
]
}
}
},
included: [
{
type: 'pet',
id: '1',
id: 'will-destroy-1',
attributes: {
name: 'Shenanigans'
}
},
{
type: 'pet',
id: '2',
id: 'server-known-2',
attributes: {
name: 'Rambunctious'
}
},
{
type: 'pet',
id: '3',
id: 'local-3',
attributes: {
name: 'Rebel'
}
Expand All @@ -1699,25 +1699,26 @@ test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship
const person = store.peekRecord('person', '1');
const pets = run(() => person.get('pets'));

const shen = store.peekRecord('pet', '1');
const rebel = store.peekRecord('pet', '3');
const shen = store.peekRecord('pet', 'will-destroy-1');
const rebel = store.peekRecord('pet', 'local-3');

assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work');
assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2'], 'precond - relationship has the correct pets to start');
assert.deepEqual(pets.map(p => get(p, 'id')), ['will-destroy-1', 'server-known-2'], 'precond - relationship has the correct pets to start');

// add a new item locally
run(() => {
pets.pushObjects([rebel]);
});

assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2', '3'], 'precond2 - relationship now has the correct three pets');
assert.deepEqual(pets.map(p => get(p, 'id')), ['will-destroy-1', 'server-known-2', 'local-3'], 'precond2 - relationship now has the correct three pets');

return run(() => {
return shen.destroyRecord({})
.then(() => {
shen.unloadRecord();

// were ember-data to now preserve local edits during a relationship push, this would be '2'
assert.deepEqual(pets.map(p => get(p, 'id')), ['2'], 'relationship now has only one pet, we lost the local change');
// were ember-data to not preserve local edits during a relationship push, this would be 'server-known-2'
assert.deepEqual(pets.map(p => get(p, 'id')), ['server-known-2', 'local-3'], 'relationship now has only one pet, we kept the local change');
});
});
});
Expand Down

0 comments on commit 34aef62

Please sign in to comment.