Skip to content

Commit

Permalink
Merge pull request #5449 from runspired/revert-has-many-perf
Browse files Browse the repository at this point in the history 
Revert "Optimise has many notifications (#4850)"
  • Loading branch information
@runspired
runspired authored Apr 25, 2018
2 parents e9e97a1 + a0884bd commit e544a31
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 139 deletions.
1 change: 0 additions & 1 deletion addon/-private/system/diff-array.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
@namespace
@method diffArray
@private
@for DS
@param {Array} oldArray the old array
@param {Array} newArray the new array
@return {hash} {
Expand Down
85 changes: 12 additions & 73 deletions addon/-private/system/relationships/state/has-many.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ 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 @@ -21,7 +19,6 @@ 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 @@ -206,11 +203,6 @@ 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 @@ -274,75 +266,22 @@ export default class ManyRelationship extends Relationship {
}

computeChanges(internalModels = []) {
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));
let members = this.canonicalMembers;
let internalModelsToRemove = [];
let internalModelSet = setForArray(internalModels);

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

internalModelsToRemove.push(member);
});

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;
}
}
}
this.removeCanonicalInternalModels(internalModelsToRemove);

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

Expand Down
118 changes: 67 additions & 51 deletions tests/integration/records/relationship-changes-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ 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 @@ -532,26 +533,32 @@ test('Calling push with relationship triggers willChange and didChange with deta
}
};

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

});
});


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

siblings.addArrayObserver(observer);

run(() => {
Expand Down Expand Up @@ -582,26 +589,31 @@ 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;
let person = run(() => store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling1Ref, sibling2Ref]

run(() => {
store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling1Ref, sibling2Ref]
}
}
}
},
included: [
sibling1, sibling2
]
}));
},
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 @@ -646,24 +658,28 @@ 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;
let person = run(() => store.push({
data: {
type: 'person',
id: 'wat',
attributes: {
firstName: 'Yehuda',
lastName: 'Katz'
},
relationships: {
siblings: {
data: [sibling2Ref]

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

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

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

module('unit/diff-array Diff Array tests', {
Expand Down
27 changes: 13 additions & 14 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('returning new hasMany relationship info from a delete maintains a local addition', function(assert) {
test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship info from a delete clears local state', function(assert) {
assert.expect(4);

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

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

assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work');
assert.deepEqual(pets.map(p => get(p, 'id')), ['will-destroy-1', 'server-known-2'], 'precond - relationship has the correct pets to start');
assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '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')), ['will-destroy-1', 'server-known-2', 'local-3'], 'precond2 - relationship now has the correct three pets');
assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2', '3'], 'precond2 - relationship now has the correct three pets');

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

// 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');
// 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');
});
});
});
Expand Down

0 comments on commit e544a31

Please sign in to comment.