[BUGFIX beta] Avoid extra work for Ember.Component#layout.
#12299
Conversation
rwjblue
force-pushed
the
make-layout-simple-prop
branch
from
4e51311 to
a520e4c
Compare
| if (this.layoutName && this.container) { | ||
| let layoutName = get(this, 'layoutName'); | ||
|
|
||
| set(this, 'layout', this.templateForName(layoutName)); |
There was a problem hiding this comment.
are these observerd, if not we can go straight to native property set, skipping Ember.set
There was a problem hiding this comment.
Kk, will update (and nothing is observable in init AFAIK).
There was a problem hiding this comment.
To answer the actual question though: we only check for them once after initial creation, and they are not observed for changes.
There was a problem hiding this comment.
Updated to remove Ember.set and just use property assignment.
👍 It would also be nice to see some micro/macro #'s as it aids my mental model (but only do if easy) |
|
Ya, I will try to run numbers this evening. Either way I think that less work (that is easier to reason about) should be better for us long term. |
--- Prior to this change, `layout` was a CP that did a few things: * `get(this, 'layoutName')` (and if set `this.container.lookup`) * `get(this, 'defaultLayout')` After this change, `layout` is a simple property that avoids that extra work. --- Prior to this change, `defaultLayout` would actually override a layout that came from the container, this was incorrect (literally the opposite of what we wanted). This corrects that logic, and ensures that we only attempt to use the `layout` property if the layout was not found during the container lookup (which properly mimics the behavior in 1.0 - 1.12 where `layout` was injected into the component if a layout was found in the container).
rwjblue
force-pushed
the
make-layout-simple-prop
branch
from
a520e4c to
bceed2f
Compare
yup |
|
The average (of 10 runs) with this change is around 2% faster than the prior commit on canary against http://rwjblue.jsbin.com/moquja. I believe it is likely that within the margin of error for this kind of thing though, but seems like win/win either way... |
[BUGFIX beta] Avoid extra work for `Ember.Component#layout`.
There are a few changes in this PR, listed below:
Prior to this change,
layoutwas a CP that did a few things:get(this, 'layoutName')(and if setthis.container.lookup)get(this, 'defaultLayout')After this change,
layoutis a simple property that avoids that extra work.Prior to this change,
defaultLayoutwould actually override a layout that came from the container, this was incorrect (literally the opposite of what we wanted).This corrects that logic, and ensures that we only attempt to use the
layoutproperty if the layout was not found during the container lookup (which properly mimics the behavior in 1.0 - 1.12 wherelayoutwas injected into the component if a layout was found in the container).The basic gist of these changes is less
Ember.get's in the normal cases, but full support for existing semantics (aka PAYGO).