[PERF] use micro-queue #4668

runspired · 2016-11-18T22:55:02Z

@stefanpenner and I were discussing how it's better to use a more specialized micro-queue than to push everything into backburner a few days ago. I was spelunking through a profile and noticed that pushing a bunch of records of one time wasn't batched even though I thought it should be, turns out the run.join + the excess calls to schedule were really hurting us. In a few limited runs this seemed to help by as much as 30% on the time to push a bunch of records of the same type into the store.

EDIT

So upon request, I separated this out into two separate PRs (see #4698 for first change and numbers associated with it). Here are the numbers for making "just this change" to current master:

Numbers are measuring store._push for 238 records: 34 primary records and 204 related records (6 per primary record). Numbers are in milliseconds and from 35 runs comparing current master to this PR.

current master

{
  "min": "17.30",
  "q1": "23.76",
  "q2": "24.86",
  "q3": "27.17",
  "max": "40.92"
}

PR

{
  "min": "17.58",
  "q1": "24.07",
  "q2": "26.19",
  "q3": "27.41",
  "max": "41.63"
}

Obviously this doesn't seem like much of a win at first, if at all, other than a simpler data flow story (we time/batch all the relationship updates together).

However, much like how we didn't see much of an improvement in #4698 either, here are the numbers for once you combine these two techniques.

{
  "min": "16.67",
  "q1": "20.22",
  "q2": "22.41",
  "q3": "24.95",
  "max": "35.48"
}

runspired · 2016-11-18T23:00:25Z

addon/-private/system/store.js

@@ -1872,7 +1872,8 @@ Store = Service.extend({
    }
    if (data) {
      // normalize relationship IDs into records
-      this._backburner.schedule('normalizeRelationships', this, this._setupRelationships, internalModel, data);
+      this._pushedInternalModels.push(internalModel, data);
+      this._backburner.schedule('normalizeRelationships', this, this._setupRelationships);


this should also check for an existing flush

stefanpenner · 2016-11-20T17:15:58Z

The idea is, lets use backburner for "the flush"

runspired · 2016-11-20T17:17:26Z

There's issues with this PR (beyond the wrong check for !== null) that I haven't finished debugging. This change leads to adapterRun being called many more times.

stefanpenner · 2016-11-20T17:17:46Z

addon/-private/system/store.js

-      this._backburner.schedule('normalizeRelationships', this, this._setupRelationships, internalModel, data);
+      this._pushedInternalModels.push(internalModel, data);
+      if (this._relationshipFlush === null) {
+        this._relationshipFlush = this._backburner.schedule('normalizeRelationships', this, this._setupRelationships);


rather then forcing the caller to notice this. I would make _setupRelationships do this, that way we don't need to leak the scheduling to the caller.

runspired · 2016-11-30T06:24:11Z

So officially I'm seeing about 5% improvement overall with this change on my usual production setup, but it also seems to have significantly smoothed out the spikes and the cpu profile and timelines are much cleaner.

rwjblue · 2016-11-30T15:56:59Z

addon/-private/system/store.js

        operation = 'deleteRecord';
      } else {
        operation = 'updateRecord';
      }

      resolver.resolve(_commit(adapter, this, operation, snapshot));
+    }
+
+    pending.forEach((pendingItem) => {


Seems odd here, is this just leftover?

it is, will remove

rwjblue · 2016-11-30T15:58:52Z

addon/-private/system/store.js

@@ -2265,48 +2272,52 @@ Store = Service.extend({

    @method _push
    @private
-    @param {Object} data
+    @param {Object} apiDoc


I don't immediately grok what apiDoc means here, is this a common term? Maybe you could add a short description after the name to make it clearer to folks?

Maybe jsonApiDoc?

^ jsonApiDoc

rwjblue · 2016-11-30T15:59:53Z

addon/-private/system/store.js

  },

  _hasModelFor(modelName) {
    return !!getOwner(this)._lookupFactory(`model:${modelName}`);
  },

+  _pushedInternalModels: null,
+  _relationshipFlush: null,


This should be set to null in the constructor (init) above.

pangratz · 2016-11-30T18:56:25Z

tests/integration/adapter/build-url-mixin-test.js

@@ -252,7 +252,7 @@ test('buildURL - buildURL takes the records from findMany', function(assert) {
        }
      }
    });
-    post.get('comments').then(assert.wait(function(post) {
+    post.get('comments').then(assert.wait(function(rel) {


creeped in from a test failure i was investigating, will scrub

pangratz · 2016-11-30T18:58:05Z

addon/-private/system/store.js

@@ -2672,7 +2699,7 @@ function _commit(adapter, store, operation, snapshot) {
      if (adapterPayload) {
        payload = normalizeResponseHelper(serializer, store, modelClass, adapterPayload, snapshot.id, operation);
        if (payload.included) {
-          store.push({ data: payload.included });
+          store._push({ data: null, included: payload.included });


👍 good one

what is the difference between store.push and store._push here?

@bcardarella the main difference is that it doesn't materialize the records, however it also will likely become the basis for a new public API that's currently going through the RFC process.

pangratz · 2016-11-30T18:59:18Z

addon/-private/system/store.js

@@ -2265,48 +2272,52 @@ Store = Service.extend({

    @method _push
    @private
-    @param {Object} data
+    @param {Object} apiDoc


Maybe jsonApiDoc?

runspired · 2016-12-06T06:29:17Z

Following Meeting feedback, I'll be separating this into two PRs. I'll leave this one for the micro-queue and pull the move of run.join to _push into a separate PR.

runspired · 2016-12-09T00:47:13Z

addon/-private/system/store.js

@@ -2632,7 +2657,7 @@ Store = Service.extend({
      return;
    }

-    assert(`A ${relationship.parentType} record was pushed into the store with the value of ${relationship.key} being '${inspect(resourceIdentifiers)}', but ${relationship.key} is a hasMany relationship so the value must be an array. You should probably check your data payload or serializer.`, Array.isArray(resourceIdentifiers));
+    assert(`A ${relationship.record.modelName} record was pushed into the store with the value of ${relationship.key} being '${inspect(resourceIdentifiers)}', but ${relationship.key} is a hasMany relationship so the value must be an array. You should probably check your data payload or serializer.`, Array.isArray(resourceIdentifiers));


same as above

runspired · 2016-12-09T00:47:40Z

addon/-private/system/store.js

@@ -2621,7 +2646,7 @@ Store = Service.extend({
      return;
    }

-    assert(`A ${relationship.parentType} record was pushed into the store with the value of ${relationship.key} being ${inspect(resourceIdentifier)}, but ${relationship.key} is a belongsTo relationship so the value must not be an array. You should probably check your data payload or serializer.`, !Array.isArray(resourceIdentifier));
+    assert(`A ${relationship.record.modelName} record was pushed into the store with the value of ${relationship.key} being ${inspect(resourceIdentifier)}, but ${relationship.key} is a belongsTo relationship so the value must not be an array. You should probably check your data payload or serializer.`, !Array.isArray(resourceIdentifier));


made this change because relationship.parentType is not a real thing which led to this always reading undefined.

bmac · 2016-12-12T15:31:10Z

Thanks @runspired.

Ember Data 2.12 introduced a bug which prevents destroys and saves happening within the same run loop. We had originally updated Ember to 2.13 and then found this issue when testing repeaters which routinely destroy and save within the same run loop, thus exposing the problem. Since this bug is subtle and may be present in unknown places elsewhere in the codebase, it was determined that downgrading to 2.11.* makes the most sense as a temporary fix until the project can eventually be updated to the latest version of Ember Data. We originally attempted upgrading to several NEWER version of Ember Data (instead of downgrading), but there were many other test failures that showed up and not enough time to fix the root causes, so this is a sensible temporary fix. bug introduced = emberjs/data#4668 bug reported = emberjs/data#4993 bug fixed = emberjs/data#4994

runspired commented Nov 18, 2016

View reviewed changes

stefanpenner reviewed Nov 20, 2016

View reviewed changes

runspired force-pushed the store-push-perf branch from 494acb3 to 0132941 Compare November 30, 2016 06:11

runspired force-pushed the store-push-perf branch from 4fd32d8 to 43abee8 Compare November 30, 2016 06:31

rwjblue reviewed Nov 30, 2016

View reviewed changes

pangratz reviewed Nov 30, 2016

View reviewed changes

runspired mentioned this pull request Dec 6, 2016

[FIX backburner] Avoids spinning up unnecessary run loops via run.join #4698

Merged

runspired force-pushed the store-push-perf branch from 43abee8 to 1f36619 Compare December 6, 2016 07:13

runspired commented Dec 9, 2016

View reviewed changes

runspired force-pushed the store-push-perf branch from 1f36619 to 86be585 Compare December 9, 2016 01:30

[PERF] use micro-queue

063b3e5

runspired force-pushed the store-push-perf branch from 86be585 to 063b3e5 Compare December 9, 2016 02:40

stefanpenner approved these changes Dec 12, 2016

View reviewed changes

bmac merged commit fef5302 into emberjs:master Dec 12, 2016

simonihmig mentioned this pull request May 22, 2017

[BUGFIX] Fix flushing of pending saves, that include a deleted record #4994

Merged

surfacedamage mentioned this pull request Oct 31, 2017

APERTA-11767 Downgrade Ember-data from 2.12.2 to 2.11.3 Aperta-project/Aperta#3693

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PERF] use micro-queue #4668

[PERF] use micro-queue #4668

runspired commented Nov 18, 2016 •

edited

Loading

runspired Nov 18, 2016

stefanpenner commented Nov 20, 2016

runspired commented Nov 20, 2016

stefanpenner Nov 20, 2016

runspired Nov 30, 2016

runspired commented Nov 30, 2016

rwjblue Nov 30, 2016

runspired Nov 30, 2016

rwjblue Nov 30, 2016

pangratz Nov 30, 2016

runspired Nov 30, 2016

rwjblue Nov 30, 2016

pangratz Nov 30, 2016

runspired Nov 30, 2016

pangratz Nov 30, 2016

bcardarella Dec 6, 2016

runspired Dec 6, 2016

pangratz Nov 30, 2016

runspired commented Dec 6, 2016

runspired Dec 9, 2016

runspired Dec 9, 2016

bmac commented Dec 12, 2016

[PERF] use micro-queue #4668

[PERF] use micro-queue #4668

Conversation

runspired commented Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

stefanpenner commented Nov 20, 2016

runspired commented Nov 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Nov 30, 2016

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

runspired commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmac commented Dec 12, 2016

runspired commented Nov 18, 2016 •

edited

Loading