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

BryanCrotaz · 2016-10-17T23:51:53Z

diff the old and new array and optimise the notifications accordingly.

Particularly important when polling server for changes with a repeating model.reload() or relationship.update()

BryanCrotaz · 2016-10-18T09:10:52Z

tested in our app where we refresh a large relationship every second and have 10 or so filters chained off this. CPU usage has gone from 100% of one core to a trickle.

stefanpenner · 2016-10-18T18:03:59Z

Cool, i was hoping someone would work on this! I will try to find some time to review this.

BryanCrotaz · 2016-10-18T18:06:10Z

Whaddayathink?

stefanpenner · 2016-10-21T08:48:11Z

.gitignore

@@ -24,3 +24,4 @@ node_modules/
 bower_components/
 .metadata_never_index
 npm-debug.log
+.vscodeignore


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

please remove

stefanpenner · 2016-10-21T08:49:18Z

tests/integration/records/relationship-changes-test.js

+import DS from 'ember-data';
+
+var env, store;
+const attr = DS.attr;


const { attr, hasMany} = DS

stefanpenner · 2016-10-21T08:49:21Z

tests/integration/records/relationship-changes-test.js

+
+import DS from 'ember-data';
+
+var env, store;


stefanpenner · 2016-10-21T08:49:31Z

tests/integration/records/relationship-changes-test.js

+var env, store;
+const attr = DS.attr;
+const hasMany = DS.hasMany;
+const run = Ember.run;


const { run } = Ember

stefanpenner · 2016-10-21T08:50:49Z

addon/-private/system/many-array.js

+        this.set('length', toSet.length);
+      }
+      this.currentState = toSet;
+      this.arrayContentDidChange(firstChangeIndex, added, removed);


as we are notifiying array change events, tests should likely test that these are accurate.

but found a bug in array change event mechanism

stefanpenner · 2016-10-21T08:52:31Z

addon/-private/system/many-array.js

+    var firstChangeIndex = -1; // -1 signifies no changes
+    // find the first change
+    var currentArray = this.currentState;
+    for (var i=0; i<shortestLength; i++) {


we should likely skip this if within init, but that isn't needed i think until #4600 is merged, so either this one or that one need to take this into account.

not sure what you mean

stefanpenner · 2016-10-21T08:53:05Z

addon/-private/system/many-array.js

@@ -87,15 +87,39 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
    );
    toSet = toSet.concat(newRecords);
    var 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)) {


if the object is dead, we should exit at the top of the function

stefanpenner · 2016-10-21T08:54:33Z

addon/-private/system/many-array.js

+    }
+    if (firstChangeIndex !== -1) {
+      // we found a change
+      var added = newLength - firstChangeIndex;


what happens if we have non-contiguous changes, e.g. the collection was replaced and some members of the previous set and the current set are the same.

[model#1, model#2, model#3, model#4, model#10]

turns into:

[model#1, model#5, model#10]

Then we'd put out

firstChange: 1 removed: 4 added: 2

... currently

Not sure what the spec is on removed and added - would it be more correct to output

firstChange: 1 removed: 2 added: 1

... could count back from end to find the last change...

the following example may be clearer as to what the problem may be.

[model#1, model#2, model#3, model#4, model#10]

becomes:

[model#1, model#10, model#3, model#11, model#10]

optimised in latest commit

stefanpenner · 2016-10-21T08:54:49Z

addon/-private/system/many-array.js

+      var removed = oldLength - firstChangeIndex;
+      this.arrayContentWillChange(firstChangeIndex, added, removed);
+      // It’s possible the parent side of the relationship may have been unloaded by this point
+      if (_objectIsAlive(this)) {


we should have already exited above if the object is dead.

stefanpenner · 2016-10-21T11:17:36Z

cc @hjdivad your thoughts on diffing/batching would be handy here.

BryanCrotaz · 2016-10-22T16:40:15Z

@stefanpenner Implemented a better change finder, but now the internals of arrayContentWillChange are failing and I can't see how - seems like the target of the call is the array, not the observer. Have a feeling I'll cause more damage than I fix if I try and work on this one.

runspired · 2016-11-18T08:04:06Z

@BryanCrotaz I'd love to land this, think you'd have a little time to pair / cleanup either this Sunday or the week after the Thanksgiving break?

BryanCrotaz · 2016-11-18T08:43:03Z

@runspired as I said in my comment above I think I need some help - the latest problem is unrelated to what I've been doing and I don't have any knowledge

BryanCrotaz · 2016-11-18T08:44:29Z

@runspired sorry just reread your comment and realised what you're suggesting yes Sunday would be fine. I'm on GMT so morning your time?

runspired · 2016-11-18T08:46:18Z

@BryanCrotaz morning my time is good :)

BryanCrotaz · 2016-11-18T09:02:54Z

Invite me to a hangout when you're ready

runspired · 2016-12-21T18:00:28Z

I ended up working on this problem last night by accident :D

BryanCrotaz · 2016-12-21T18:38:40Z

@runspired :) Did you get anywhere? Still happy to pair...

Conflicts: addon/-private/system/many-array.js tests/integration/records/relationship-changes-test.js

BryanCrotaz · 2017-03-02T00:34:57Z

Odd - many-array flushCanonical is not being called any more during store.push so none of my optimisations are kicking in.

What's the new pattern?

stefanpenner · 2017-03-08T08:09:01Z

addon/-private/system/many-array.js

+      }
+      const added = newLength - unchangedEndBlockLength - firstChangeIndex;
+      const removed = oldLength - unchangedEndBlockLength - firstChangeIndex;
+      this.arrayContentWillChange(firstChangeIndex, removed, added);


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

stefanpenner · 2017-03-08T08:09:37Z

addon/-private/system/many-array.js

-    // 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;


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

stefanpenner · 2017-03-08T08:10:23Z

address feedback
tests green
rebase

BryanCrotaz · 2017-03-08T11:25:57Z

@stefanpenner rest of tests are failing because of a change on master.

relationship.push(data) used to call manyArray.flushCanonical but doesn't any more. I'm not sure why this would be the case. What's the correct behaviour here?

BryanCrotaz · 2017-03-08T13:00:22Z

Very strange.

I have a test that pushes a siblings hasMany into a person using store.push.

This observer fires ok

    person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() {
      observerCount++;
    });

This observer does not

    person.addObserver('siblings.[]', function() {
      observerCount++;
    });

sly7-7 · 2017-03-08T13:06:22Z

@BryanCrotaz Maybe because siblings.[] is not really the same as the manyarray ?

BryanCrotaz · 2017-03-08T13:15:20Z

@sly7-7 This test passed back in October. If I'm observing siblings.[] on a person I'd expect that observer to trigger when the relationship is changed via a store.push.

…o observe siblings.[]?

sly7-7 · 2017-03-08T14:11:28Z

@BryanCrotaz I fully agree with that. Are the tests failing because of your work ? Or do they not pass with the actual code ?

BryanCrotaz · 2017-03-08T14:53:26Z

I think they're not passing because of changes on master

stefanpenner · 2017-03-08T15:40:16Z

addon/-private/system/many-array.js

@@ -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() {


isInitialized no longer needed?

stefanpenner · 2017-03-08T15:40:59Z

addon/-private/system/many-array.js

      this.relationship.notifyHasManyChanged();
    }
+    this.record.updateRecordArrays();


if there is no change found, why invoke updateRecordArrays?

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

stefanpenner · 2017-03-08T15:41:52Z

addon/-private/system/many-array.js

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


when is diff.firstChangeIndex falsey and not 0 ?

Ah - that's left over from some testing

stefanpenner · 2017-03-08T15:42:02Z

addon/-private/system/many-array.js

+      this.arrayContentWillChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount);
+      set(this, 'length', toSet.length);
+      this.currentState = toSet;
+      this.arrayContentDidChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount);


stefanpenner · 2017-03-08T15:42:25Z

addon/-private/system/many-array.js

@@ -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);


this change seems unneeded

stefanpenner · 2017-03-08T16:15:50Z

tests/integration/records/relationship-changes-test.js

+  run(function() {
+    assert.equal(willChangeCount, 1, 'willChange observer should be triggered once');
+    assert.equal(didChangeCount, 1, 'didChange observer should be triggered once');
+    done();


this test does not actually appear async, the done can be removed.

stefanpenner · 2017-03-08T16:15:58Z

tests/integration/records/relationship-changes-test.js

+  run(function() {
+    assert.equal(willChangeCount, 1, 'willChange observer should be triggered once');
+    assert.equal(didChangeCount, 1, 'didChange observer should be triggered once');
+    done();


this test does not actually appear async, the done can be removed.

stefanpenner · 2017-03-08T16:16:04Z

tests/integration/records/relationship-changes-test.js

+  run(function() {
+    assert.equal(willChangeCount, 1, 'willChange observer should be triggered once');
+    assert.equal(didChangeCount, 1, 'didChange observer should be triggered once');
+    done();


this test does not actually appear async, the done can be removed.

stefanpenner · 2017-03-08T16:16:10Z

tests/integration/records/relationship-changes-test.js

+  run(function() {
+    assert.equal(willChangeCount, 1, 'willChange observer should be triggered once');
+    assert.equal(didChangeCount, 1, 'didChange observer should be triggered once');
+    done();


this test does not actually appear async, the done can be removed.

stefanpenner · 2017-03-08T16:16:15Z

tests/integration/records/relationship-changes-test.js

+  run(function() {
+    assert.equal(willChangeCount, 1, 'willChange observer should be triggered once');
+    assert.equal(didChangeCount, 1, 'didChange observer should be triggered once');
+    done();


this test does not actually appear async, the done can be removed.

stefanpenner · 2017-03-08T18:22:28Z

@BryanCrotaz I played with this approach in your example app, it very much improved performance of the pathological cases. I believe it is very much worth exploring further.

I am spending some time with @igorT tomorrow, we will be talking about this and how to move it forward.

I have left some feedback, that being addressed will help move this forward.

BryanCrotaz · 2017-03-08T18:31:21Z

We have lists with 100 entries where we're filtering on a model attribute. We now only check new array entries and with these changes we've see 50-fold changes in cpu usage.

stefanpenner · 2017-03-08T18:51:42Z

We have lists with 100 entries where we're filtering on a model attribute. We now only check new array entries and with these changes we've see 50-fold changes in cpu usage.

Yup, the downstream side-affects of overchatty arrays can be massive.

BryanCrotaz · 2017-03-08T23:50:55Z

@stefanpenner I think all your points are dealt with

stefanpenner · 2017-03-09T00:10:38Z

@BryanCrotaz very cool, will review thoroughly in the AM \w @igorT

BryanCrotaz · 2017-03-09T00:51:10Z

Unfortunately... it's so efficient that CPs hung off a peekAll don't work any more!

BryanCrotaz · 2017-03-09T01:35:24Z

@stefanpenner I need some help on this one.

I've added a test for the following code not working.

test('Calling push with relationship triggers observer of array if the relationship was empty and is added to', function(assert) {
// ...
    person.addObserver('siblings.[]', function() {
      observerCount++;
    });
// ...

The observer doesn't trigger. However the array observer on the many array in other tests does trigger.

How does an observer of 'array.[]' get triggered?

EDIT: Odd - this test doesn't pass on master either. Can't see anything wrong with it though. Thoughts?

stefanpenner · 2017-03-09T03:58:12Z

@BryanCrotaz let me take a look

stefanpenner · 2017-03-09T04:19:20Z

@BryanCrotaz it appears you CP is never accessed, so the observer has nothing to watch.

The following addresses the issue, but the test fails because relationships do silly things still and we mutate the array twice not once (we will have to address in the future)

commit 7b8ad1a47bd2ae8e999b95e26b15a4cecffa378f
Author: Stefan Penner <[email protected]>
Date:   Wed Mar 8 20:12:51 2017 -0800

    ensure the CP is accessed at-least once, so that the observer has something to observe

diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js
index b2595315..ed99e5a5 100644
--- a/tests/integration/records/relationship-changes-test.js
+++ b/tests/integration/records/relationship-changes-test.js
@@ -276,7 +276,6 @@ test('Calling push with relationship recalculates computed alias property to fir
 
 test('Calling push with relationship triggers observer of array if the relationship was empty and is added to', function(assert) {
   assert.expect(1);
-  let person = null;
   let observerCount = 0;
 
   run(() => {
@@ -297,12 +296,15 @@ test('Calling push with relationship triggers observer of array if the relations
     });
   });
 
-  person = store.peekRecord('person', 'wat');
+  let person = store.peekRecord('person', 'wat');
 
   run(() => {
-    person.addObserver('siblings.[]', function() {
-      observerCount++;
-    });
+    // materialize the CP, so that the later siblings observer enages (observers
+    // dont have side affects, so they cause a cp, in this case a relationship, to
+    // be accessed themsleves).
+    person.get('siblings');
+
+    person.addObserver('siblings.[]', () => observerCount++);
   });
 
   run(() => {
@@ -324,9 +326,7 @@ test('Calling push with relationship triggers observer of array if the relations
     });
   });
 
-  run(() => {
-    assert.equal(observerCount, 1, 'siblings observer should be triggered once');
-  });
+  assert.equal(observerCount, 1, 'siblings observer should be triggered once');
 });
 
 test('Calling push with relationship triggers observers once if the relationship was not empty and was added to', function(assert) {

BryanCrotaz · 2017-03-09T11:06:55Z

@stefanpenner doh! that old chestnut.

I've found another inefficiency that I don't know whether to fix. manyArray fires property change events during destruction. Does it need to? Is that going to break something else?

EDIT: appears it's needed so that CP caches are cleared when relationships are unloaded

BryanCrotaz · 2017-03-09T11:23:13Z

I think this PR is complete now.

BryanCrotaz · 2017-03-09T11:29:58Z

The diff is a bit of a mess so I'll resubmit as a new PR with it all flattened.

BryanCrotaz · 2017-03-09T11:47:52Z

replaced by PR #4850

optimise notifications when has-many array is changed

0eeff74

This was referenced Oct 18, 2016

querying existing record triggers a change in its belongsTo relationships (twiddle included) #4579

Closed

HasMany addArrayObserver arrayWillChange/arrayDidChange unexpected behavior #2981

Closed

BryanCrotaz changed the title ~~optimise notifications when has-many array is changed~~ [PERF] optimise notifications when has-many array is changed Oct 21, 2016

move test models into Module for speed and ES6-ify

4be1c97

stefanpenner reviewed Oct 21, 2016

View reviewed changes

BryanCrotaz added 2 commits October 21, 2016 18:04

add test for enumerable observer and ES6-ify

fd60d3e

implement subset array diff in has-many to optimise notifications

2625282

correct order of params in array notifications

3674dca

Merge branch 'master' into optimise-has-many

9577e58

Conflicts: addon/-private/system/many-array.js tests/integration/records/relationship-changes-test.js

stefanpenner suggested changes Mar 8, 2017

View reviewed changes

BryanCrotaz added 2 commits March 8, 2017 11:08

refactor diff-array out as function for ease of testing

6b71d77

fix most tests

cdbaf87

BryanCrotaz added 2 commits March 8, 2017 13:19

fix array observer tests - not sure how these tests used to pass

27e2f11

observe the many arrays themselves in tests - should surely be able t…

b199170

…o observe siblings.[]?

stefanpenner suggested changes Mar 8, 2017

View reviewed changes

BryanCrotaz added 4 commits March 8, 2017 19:13

address feedback and bring many-array into line with master

8fe3411

apply feedback

993dd60

remove jsconfig.json

df30a5f

bring in line with master

6e67c3d

add tests for CP hooked to many-array

e8c2c86

fix observer tests

90dbc65

BryanCrotaz closed this Mar 9, 2017

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

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

Conversation

BryanCrotaz commented Oct 17, 2016

BryanCrotaz commented Oct 18, 2016

stefanpenner commented Oct 18, 2016 • edited Loading

BryanCrotaz commented Oct 18, 2016

stefanpenner Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCrotaz Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Oct 21, 2016

BryanCrotaz commented Oct 22, 2016

runspired commented Nov 18, 2016

BryanCrotaz commented Nov 18, 2016

BryanCrotaz commented Nov 18, 2016

runspired commented Nov 18, 2016

BryanCrotaz commented Nov 18, 2016

runspired commented Dec 21, 2016

BryanCrotaz commented Dec 21, 2016

BryanCrotaz commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

sly7-7 commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

sly7-7 commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

stefanpenner commented Mar 8, 2017

BryanCrotaz commented Mar 8, 2017

stefanpenner commented Mar 9, 2017

BryanCrotaz commented Mar 9, 2017

BryanCrotaz commented Mar 9, 2017

stefanpenner commented Mar 9, 2017

stefanpenner commented Mar 9, 2017

BryanCrotaz commented Mar 9, 2017 • edited Loading

BryanCrotaz commented Mar 9, 2017

BryanCrotaz commented Mar 9, 2017

BryanCrotaz commented Mar 9, 2017

stefanpenner commented Oct 18, 2016 •

edited

Loading

stefanpenner Oct 21, 2016 •

edited

Loading

BryanCrotaz Oct 21, 2016 •

edited

Loading

BryanCrotaz commented Mar 9, 2017 •

edited

Loading