[BUGFIX release] Faster attrs-proxy

The attrs-proxy exists to make component attrs available directly on the component instance, for compatibility with the pre-Glimmer world. This change makes the attrs-proxy significantly faster by implementing the proxy using Glimmer's lifecycle hooks instead of creating observers. I also moved the didInitAttrs hook inside component `init`, so that changes made there do not trigger observers. I condensed two distinct tests into one, because the subtle difference between the two scenarios has disappeared. The behavior change seems acceptable because this case is already covered under the double-render warning, so people will be told they're doing something with potentially undefined behavior. Using [complex list benchmark](https://github.com/ef4/initial-render-perf) that has been ported to idiomatic modern Ember, I'm measuring 56% faster rendering when compared to master. This takes us from 270% slower than 1.12 to 21% slower than 1.12. And much remains to still be optimized.
emberjs · Aug 2, 2015 · eb2e127 · eb2e127
1 parent bc2a93f
commit eb2e127
Show file tree

Hide file tree

Showing 8 changed files with 29 additions and 126 deletions.
diff --git a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js
@@ -300,16 +300,15 @@ ComponentNodeManager.prototype.destroy = function() {
 
 export function createComponent(_component, isAngleBracket, _props, renderNode, env, attrs = {}, proto = _component.proto()) {
   let props = assign({}, _props);
-  let attrsSnapshot;
 
   if (!isAngleBracket) {
     let hasSuppliedController = 'controller' in attrs; // 2.0TODO remove
     Ember.deprecate('controller= is deprecated', !hasSuppliedController, { id: 'ember-htmlbars.create-component', until: '3.0.0' });
 
-    attrsSnapshot = takeSnapshot(attrs);
-    props.attrs = attrsSnapshot;
+    let snapshot = takeSnapshot(attrs);
+    props.attrs = snapshot;
 
-    mergeBindings(props, shadowedAttrs(proto, attrsSnapshot));
+    mergeBindings(props, shadowedAttrs(proto, snapshot));
   } else {
     props._isAngleBracket = true;
   }
@@ -319,8 +318,6 @@ export function createComponent(_component, isAngleBracket, _props, renderNode,
 
   let component = _component.create(props);
 
-  env.renderer.componentInitAttrs(component, attrsSnapshot);
-
   // for the fallback case
   component.container = component.container || env.container;
 

diff --git a/packages/ember-htmlbars/lib/node-managers/view-node-manager.js b/packages/ember-htmlbars/lib/node-managers/view-node-manager.js
@@ -95,8 +95,6 @@ ViewNodeManager.prototype.render = function(env, attrs, visitor) {
     }
 
     if (component) {
-      var snapshot = takeSnapshot(attrs);
-      env.renderer.setAttrs(this.component, snapshot);
       env.renderer.willRender(component);
       env.renderedViews.push(component.elementId);
     }
@@ -130,7 +128,7 @@ ViewNodeManager.prototype.rerender = function(env, attrs, visitor) {
       env.renderer.willUpdate(component, snapshot);
 
       if (component._renderNode.shouldReceiveAttrs) {
-        env.renderer.updateAttrs(component, snapshot);
+        env.renderer.componentUpdateAttrs(component, snapshot);
         setProperties(component, mergeBindings({}, shadowedAttrs(component, snapshot)));
         component._renderNode.shouldReceiveAttrs = false;
       }
@@ -187,6 +185,7 @@ export function createOrUpdateComponent(component, options, createOptions, rende
 
     component = component.create(props);
   } else {
+    env.renderer.componentUpdateAttrs(component, snapshot);
     mergeBindings(props, shadowedAttrs(component, snapshot));
     setProperties(component, props);
   }

diff --git a/packages/ember-htmlbars/tests/integration/component_invocation_test.js b/packages/ember-htmlbars/tests/integration/component_invocation_test.js
@@ -810,55 +810,6 @@ QUnit.test('implementing `render` allows pushing into a string buffer', function
   equal(view.$('#zomg').text(), 'Whoop!');
 });
 
-QUnit.test('comopnent should rerender when a property is changed during children\'s rendering', function() {
-  expectDeprecation(/twice in a single render/);
-
-  var outer, middle;
-
-  registry.register('component:x-outer', Component.extend({
-    value: 1,
-    grabReference: Ember.on('init', function() {
-      outer = this;
-    })
-  }));
-
-  registry.register('component:x-middle', Component.extend({
-    grabReference: Ember.on('init', function() {
-      middle = this;
-    })
-  }));
-
-  registry.register('component:x-inner', Component.extend({
-    pushDataUp: Ember.observer('value', function() {
-      middle.set('value', this.get('value'));
-    })
-  }));
-
-  registry.register('template:components/x-outer', compile('{{#x-middle}}{{x-inner value=value}}{{/x-middle}}'));
-  registry.register('template:components/x-middle', compile('<div id="middle-value">{{value}}</div>{{yield}}'));
-  registry.register('template:components/x-inner', compile('<div id="inner-value">{{value}}</div>'));
-
-
-  view = EmberView.extend({
-    template: compile('{{x-outer}}'),
-    container: container
-  }).create();
-
-  runAppend(view);
-
-  equal(view.$('#inner-value').text(), '1', 'initial render of inner');
-  equal(view.$('#middle-value').text(), '1', 'initial render of middle');
-
-  run(() => outer.set('value', 2));
-
-  equal(view.$('#inner-value').text(), '2', 'second render of inner');
-  equal(view.$('#middle-value').text(), '2', 'second render of middle');
-
-  run(() => outer.set('value', 3));
-
-  equal(view.$('#inner-value').text(), '3', 'third render of inner');
-  equal(view.$('#middle-value').text(), '3', 'third render of middle');
-});
 
 QUnit.test('components in template of a yielding component should have the proper parentView', function() {
   var outer, innerTemplate, innerLayout;
@@ -970,7 +921,7 @@ QUnit.test('components should receive the viewRegistry from the parent view', fu
   equal(outer._viewRegistry, viewRegistry);
 });
 
-QUnit.test('comopnent should rerender when a property (with a default) is changed during children\'s rendering', function() {
+QUnit.test('comopnent should rerender when a property is changed during children\'s rendering', function() {
   expectDeprecation(/modified value twice in a single render/);
 
   var outer, middle;

diff --git a/packages/ember-views/lib/compat/attrs-proxy.js b/packages/ember-views/lib/compat/attrs-proxy.js
@@ -1,15 +1,7 @@
-import { get } from 'ember-metal/property_get';
 import { Mixin } from 'ember-metal/mixin';
-import { on } from 'ember-metal/events';
 import { symbol } from 'ember-metal/utils';
 import { PROPERTY_DID_CHANGE } from 'ember-metal/property_events';
-
-import {
-  addObserver,
-  removeObserver,
-  _addBeforeObserver,
-  _removeBeforeObserver
-} from 'ember-metal/observer';
+import { on } from 'ember-metal/events';
 
 export function deprecation(key) {
   return `You tried to look up an attribute directly on the component. This is deprecated. Use attrs.${key} instead.`;
@@ -21,16 +13,6 @@ function isCell(val) {
   return val && val[MUTABLE_CELL];
 }
 
-function attrsWillChange(view, attrsKey) {
-  let key = attrsKey.slice(6);
-  view.currentState.legacyAttrWillChange(view, key);
-}
-
-function attrsDidChange(view, attrsKey) {
-  let key = attrsKey.slice(6);
-  view.currentState.legacyAttrDidChange(view, key);
-}
-
 let AttrsProxyMixin = {
   attrs: null,
 
@@ -56,46 +38,39 @@ let AttrsProxyMixin = {
     val.update(value);
   },
 
-  willWatchProperty(key) {
-    if (this._isAngleBracket || key === 'attrs') { return; }
-
-    let attrsKey = `attrs.${key}`;
-    _addBeforeObserver(this, attrsKey, null, attrsWillChange);
-    addObserver(this, attrsKey, null, attrsDidChange);
+  _propagateAttrsToThis() {
+    let attrs = this.attrs;
+    let values = {};
+    for (let prop in attrs) {
+      if (prop !== 'attrs') {
+        values[prop] = this.getAttr(prop);
+      }
+    }
+    this.setProperties(values);
   },
 
-  didUnwatchProperty(key) {
-    if (this._isAngleBracket || key === 'attrs') { return; }
+  initializeShape: on('init', function() {
+    this._isDispatchingAttrs = false;
+  }),
 
-    let attrsKey = `attrs.${key}`;
-    _removeBeforeObserver(this, attrsKey, null, attrsWillChange);
-    removeObserver(this, attrsKey, null, attrsDidChange);
+  didReceiveAttrs() {
+    this._super();
+    this._isDispatchingAttrs = true;
+    this._propagateAttrsToThis();
+    this._isDispatchingAttrs = false;
   },
 
-  legacyDidReceiveAttrs: on('didReceiveAttrs', function() {
-    if (this._isAngleBracket) { return; }
-
-    var keys = Object.keys(this.attrs);
-
-    for (var i = 0, l = keys.length; i < l; i++) {
-      // Only issue the deprecation if it wasn't already issued when
-      // setting attributes initially.
-      if (!(keys[i] in this)) {
-        this.notifyPropertyChange(keys[i]);
-      }
-    }
-  }),
 
   unknownProperty(key) {
     if (this._isAngleBracket) { return; }
 
-    var attrs = get(this, 'attrs');
+    var attrs = this.attrs;
 
     if (attrs && key in attrs) {
       // do not deprecate accessing `this[key]` at this time.
       // add this back when we have a proper migration path
       // Ember.deprecate(deprecation(key), { id: 'ember-views.', until: '3.0.0' });
-      let possibleCell = get(attrs, key);
+      let possibleCell = attrs.key;
 
       if (possibleCell && possibleCell[MUTABLE_CELL]) {
         return possibleCell.value;
@@ -112,6 +87,7 @@ let AttrsProxyMixin = {
 
 AttrsProxyMixin[PROPERTY_DID_CHANGE] = function(key) {
   if (this._isAngleBracket) { return; }
+  if (this._isDispatchingAttrs) { return; }
 
   if (this.currentState) {
     this.currentState.legacyPropertyDidChange(this, key);

diff --git a/packages/ember-views/lib/views/states/default.js b/packages/ember-views/lib/views/states/default.js
@@ -1,11 +1,5 @@
 import EmberError from 'ember-metal/error';
 import { get } from 'ember-metal/property_get';
-
-import {
-  propertyWillChange,
-  propertyDidChange
-} from 'ember-metal/property_events';
-
 import { MUTABLE_CELL } from 'ember-views/compat/attrs-proxy';
 
 /**
@@ -26,21 +20,8 @@ export default {
     return null;
   },
 
-  legacyAttrWillChange(view, key) {
-    if (key in view.attrs && !(key in view)) {
-      propertyWillChange(view, key);
-    }
-  },
-
-  legacyAttrDidChange(view, key) {
-    if (key in view.attrs && !(key in view)) {
-      propertyDidChange(view, key);
-    }
-  },
-
   legacyPropertyDidChange(view, key) {
     let attrs = view.attrs;
-
     if (attrs && key in attrs) {
       let possibleCell = attrs[key];
 

diff --git a/packages/ember-views/lib/views/states/pre_render.js b/packages/ember-views/lib/views/states/pre_render.js
@@ -9,8 +9,6 @@ import merge from 'ember-metal/merge';
 let preRender = Object.create(_default);
 
 merge(preRender, {
-  legacyAttrWillChange(view, key) {},
-  legacyAttrDidChange(view, key) {},
   legacyPropertyDidChange(view, key) {}
 });
 

diff --git a/packages/ember-views/lib/views/view.js b/packages/ember-views/lib/views/view.js
@@ -1322,6 +1322,8 @@ var View = CoreView.extend(
     if (!this._viewRegistry) {
       this._viewRegistry = View.views;
     }
+
+    this.renderer.componentInitAttrs(this, this.attrs || {});
   },
 
   __defineNonEnumerable(property) {

diff --git a/packages/ember-views/tests/compat/attrs_proxy_test.js b/packages/ember-views/tests/compat/attrs_proxy_test.js
@@ -83,7 +83,6 @@ QUnit.test('an observer on an attribute in the root of the component is fired wh
 
     barObserver: on('init', observer('bar', function() {
       var count = get(this, 'observerFiredCount');
-
       set(this, 'observerFiredCount', count + 1);
     })),