[PERF] make internal model lazier #4562

runspired · 2016-09-29T20:52:56Z

Branches off of heimdall-instrumentation, so that should be merged fist ;)

fivetanley · 2016-09-30T18:28:50Z

so excited 🙏

we'll have to make sure we don't break @BookingSync's and others' addons.

runspired · 2016-09-30T18:44:05Z

@fivetanley this provides identical private API's to what was present before, they are just lazy, with one notable exception. I've removed the use of get and set for currentState. It was being used inconsistently, half the places used dot notation, half used get/set. Looking into the code paths it appears there isn't a strong case for it needing to be get/set based, and obvs with it being inconsistent before if it did "need to be" it would have been broken.

fivetanley · 2016-09-30T18:45:02Z

yeah i am not really concerned about the api changing here, just a last step before merging to run it against addons that use internalModel heavily.

runspired · 2016-09-30T18:45:06Z

Also note, the tests on this pass locally but I need to rebase off master for them to pass here, If we could merge heimdall today I've got a follow up PR after this one that's also fun :)

runspired · 2016-10-17T11:01:58Z

@igor rebase complete

stefanpenner · 2016-10-17T21:59:44Z

addon/-private/system/model/internal-model.js

+
+    // these are here to prevent shape change later when Ember lookups add them
+    this.__ember_meta__ = null;
+    this[GUID_KEY] = guidFor(this);


i don't believe the left hand side here is required

stefanpenner · 2016-10-17T22:04:06Z

addon/-private/system/model/internal-model.js

          changedKeys.push(key);
        }
      }
    }

    return changedKeys;
-  },
+  }

  toString() {
    if (this.record) {
      return this.record.toString();


this branch seems un-needed

stefanpenner · 2016-10-17T22:04:37Z

addon/-private/system/model/states.js

@@ -584,6 +584,7 @@ var RootState = {
      },

      didCommit(internalModel) {
+        // TODO @runspired lastDirtyType doesn't seem to be a real thing
        internalModel.send('invokeLifecycleCallbacks', get(internalModel, 'lastDirtyType'));


^ todo + get, if its not used then this line may be a candidate for removal

stefanpenner · 2016-10-18T17:49:11Z

addon/-private/system/model/internal-model.js

+    let i;
+    let l;
+
+    if (TransitionChainMap[transitionMapId]) {


maybe let map = TransitionChainMap[transitionMapId] then reference map everywhere

stefanpenner · 2016-10-18T17:50:07Z

addon/-private/system/model/states.js

@@ -241,7 +244,7 @@ var DirtyState = {

    //TODO(Igor) reloading now triggers a
    //loadingData event, though it seems fine?
-    loadingData: Ember.K,
+    loadingData: K,


lets just do loadingData() { } rather then the single shared function of K

stefanpenner · 2016-10-18T17:50:47Z

addon/-private/system/model/states.js

@@ -4,7 +4,10 @@
 import Ember from 'ember';
 import { assert } from "ember-data/-private/debug";

-var get = Ember.get;
+const {
+  K


lets not use K regular concise functions are likely fine.

runspired · 2016-10-18T21:55:32Z

Eyeballing the production build benchmarks (no filtering of outliers but I am looking at a box plot), we moved from roughly 35ms spent in store.push for 238 records of a single type to a bit less than 28ms.

tchak · 2016-10-18T22:31:10Z

LGTM 👍

igorT · 2016-10-19T10:22:46Z

addon/-private/system/model/internal-model.js

+
+const assign = Ember.assign || Ember.merge;
+const setOwner = Ember.setOwner;
+const TransitionChainMap = new EmptyObject();


Can you put a comment here or in the method as to how the transitionchain map works

stefanpenner · 2016-10-19T11:09:36Z

addon/-private/system/model/model.js

@@ -305,6 +308,7 @@ var Model = Ember.Object.extend(Ember.Evented, {
    @private
    @type {Object}
  */
+  currentState: null,


should we define this on the prototype?

stefanpenner · 2016-10-19T11:09:59Z

addon/-private/system/model/states.js

@@ -430,7 +433,7 @@ createdState.uncommitted.pushedData = function(internalModel) {
  internalModel.triggerLater('didLoad');
 };

-createdState.uncommitted.propertyWasReset = Ember.K;
+createdState.uncommitted.propertyWasReset = K;


= function() { }

runspired · 2016-10-19T11:30:10Z

@igorT Travis is passing after making the changes from @stefanpenner's last review.

- allocations are now lazy - InternalModel is now an ES2015 class, the expensive classCallCheck is stripped via a babel plugin - TransitionMapCache has been added to make transitionTo function faster - get/set of currentState has been moved to dot notation access - overall this appears to be a 20% perf improvement, with more gains still realizable.

runspired force-pushed the feat/internal-model branch from 2b28f97 to 4fba4a9 Compare October 17, 2016 10:58

stefanpenner reviewed Oct 17, 2016

View reviewed changes

stefanpenner reviewed Oct 18, 2016

View reviewed changes

runspired force-pushed the feat/internal-model branch from 7baf19f to c0e567d Compare October 18, 2016 18:15

runspired mentioned this pull request Oct 18, 2016

Feat/improved cache #4589

Merged

igorT reviewed Oct 19, 2016

View reviewed changes

runspired force-pushed the feat/internal-model branch 3 times, most recently from 374ea89 to 475d9aa Compare October 19, 2016 10:38

stefanpenner reviewed Oct 19, 2016

View reviewed changes

runspired force-pushed the feat/internal-model branch from 475d9aa to 5afa263 Compare October 19, 2016 11:16

runspired force-pushed the feat/internal-model branch from 5afa263 to a28fb00 Compare October 19, 2016 20:17

igorT merged commit 41193f7 into emberjs:master Oct 21, 2016

pangratz mentioned this pull request Oct 23, 2016

complex InternalModel attributes may want to become lazy #3644

Closed

runspired mentioned this pull request Apr 18, 2018

Consider optimizing transitionTo #3142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PERF] make internal model lazier #4562

[PERF] make internal model lazier #4562

runspired commented Sep 29, 2016

fivetanley commented Sep 30, 2016

runspired commented Sep 30, 2016

fivetanley commented Sep 30, 2016

runspired commented Sep 30, 2016

runspired commented Oct 17, 2016

stefanpenner Oct 17, 2016

stefanpenner Oct 17, 2016

stefanpenner Oct 17, 2016

stefanpenner Oct 18, 2016

stefanpenner Oct 18, 2016

stefanpenner Oct 18, 2016

runspired commented Oct 18, 2016

tchak commented Oct 18, 2016

igorT Oct 19, 2016

stefanpenner Oct 19, 2016 •

edited

stefanpenner Oct 19, 2016

runspired commented Oct 19, 2016

[PERF] make internal model lazier #4562

[PERF] make internal model lazier #4562

Conversation

runspired commented Sep 29, 2016

fivetanley commented Sep 30, 2016

runspired commented Sep 30, 2016

fivetanley commented Sep 30, 2016

runspired commented Sep 30, 2016

runspired commented Oct 17, 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

runspired commented Oct 18, 2016

tchak commented Oct 18, 2016

Choose a reason for hiding this comment

stefanpenner Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Oct 19, 2016

stefanpenner Oct 19, 2016 •

edited