Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component creation performance is slower in 1.13 than in 1.12 #11502

Closed
GavinJoyce opened this issue Jun 18, 2015 · 65 comments

Comments

Projects
None yet
@GavinJoyce
Copy link
Member

commented Jun 18, 2015

The performance of the initial component render times seems to be significantly slower in Ember 1.13 than in Ember 1.12.

Ember 1.13: http://emberjs.jsbin.com/zijuye

image

Ember 1.12: http://emberjs.jsbin.com/layoso

image

Ember 1.11: http://emberjs.jsbin.com/kunaco

Ember 1.10: http://emberjs.jsbin.com/xivavi

@GavinJoyce GavinJoyce changed the title Component creation performance is worse in 1.13 Component creation performance is slower in 1.13 than in 1.12 Jun 18, 2015

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2015

Possibly a dupe of #10261 which I've just come across.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

i would like to see the numbers without the ember inspector open. The inspector itself does negatively affect performance as it instrumentation overhead isn't non-trivial, it is possible that merely the instrumentation has changed to favor the former. Even if not, it non-inspector #'s would provide a more accurate perspective.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

Quick stab at removing the need for measuring with the inspector:

1.13.0 - 112.159ms
1.12.0 - 47ms


Also, please note that 1.13.0 and 1.13.1 production builds still included all deprecations (because of an issue with the build pipeline which was fixed in 1.13.2).

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

1.13.2 drops to: 81.203ms

This may also be do to the slightly different babel transpiled output, although I hope to a lesser degree.

@OrKoN

This comment has been minimized.

Copy link

commented Jul 2, 2015

@rwjblue @stefanpenner I have found an old benchmark which compares performance of major frameworks using a todo app. The benchmark was using an old version of Ember so I migrated it to Ember 1.13.2. Surprisingly to me, Ember turns out to be 4-10 times slower. Maybe you could take a look at the app as you're experts? I hope I am doing something terribly wrong in the app or in the test.

selection_104

Git: https://github.com/OrKoN/todomvc-perf-comparison
Live: http://blog.alex-rudenko.com/todomvc-perf-comparison/

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

This test is a-bit quirky, although there are likely some issues with initial render. So please don't assume I am being dismissive of a potential problem.

Let me explain, ember makes the following assumption:

1 click producing a 100 changes, is more realistic then 100 sequential clicks each producing 1 change.

So we batch work, and do optimizations per click. The overhead for these optimizations means the 100 sequential clicks case is poor, but benefit being, the realistic scenario should benefit from the optimizations.

A quick adjustment, to instead perform the same optimization but in what i consider to be the unrealistic scenario results in:

before:

Ember : Adding100Items : Sync: 2253.7569999694824 ms
Ember : Adding100Items : Async: 32.8890000237152 ms
Ember : CompletingAllItems : Sync: 357.1809999994002 ms
Ember : CompletingAllItems : Async: 16.21599995996803 ms
Ember : DeletingItems : Sync: 1841.7600000393577 ms
Ember : DeletingItems : Async: 10.8780000009574 ms
Ember : 4512.680999992881 ms
Run 1/1 - Total : 4512.680999992881 ms

after:

Ember : Adding100Items : Sync: 534.5300000044517 ms
Ember : Adding100Items : Async: 33.12600002391264 ms
Ember : DeletingItems : Sync: 612.1939999866299 ms
Ember : DeletingItems : Async: 14.044000010471791 ms
Ember : 1193.894000025466 ms
Run 1/1 - Total : 1193.894000025466 ms

It's worth noting, this still isn't really realistic, As simulating 100 sequential clicks isn't how an app would normally delete 100 items. It's even highly unlikely that a user could issue 100 clicks at 60fps, which would result in 1600ms of idle time.

I suspect if the test was adjusted to have a single actions that added 100 and a single actions that removed 100, and another action that merely mutated 100. We would be able to see something useful.

Now thats not saying that improvements here couldn't provide some benefit, but they run the risk of focusing on the wrong things, and maybe even making the important case worse.

@sandstrom

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

I've created a simple example app on initial rendering performance, basically a bunch of small components. Perhaps it's useful for investigations.

@OrKoN

This comment has been minimized.

Copy link

commented Jul 2, 2015

I think I get the idea but it seems that it's not quite working at the moment. Although the tests support the idea (see below). The app is faster when 100 items are inserted at once. But still slower than others.

I have modified the angular and ember apps to insert 100 items at once (with update & removal as previously):

image

only insertion of 100 items at once w/o removal and update:

image

insertion 1 by 1 w/o removal and update:

image

Then I thought that the Ember app may be slower due to ember-data and the locale storage adapter that it uses. So I changed the app to use just Ember.A. This is a batch insert of 100 items at once:

image

In my real app, I have got a problem with a dropdown. ~200 items take 600ms for initial rendering (gets worse on mobiles). So it's pretty much like this screen:

image

or with backbone included:

image

Also I want to say that most of other apps seem low-level to me and harder to understand than the Ember app. But performance is important too.

@mixonic

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

A complete aside, but FWIW I constantly reference the benchmarks in https://github.com/eviltrout/ember-performance to understand how Ember's performance changes over time. Please contribute benchmarks to that repo, and improve what is there. A one-off repo may be helpful for this immediate discussion, but will not have a long-term impact.

That is all, thanks :-)

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2015

Thanks @mixonic, I'll make some conributions to ember-performance.

The Renders Complex List test captures the difference between 1.12.0 and 1.13.3:

image

image

@marcemira

This comment has been minimized.

Copy link

commented Jul 7, 2015

I have a huge and heavily loaded of components page... Since I switched from 1.12.1 to 1.13.3 it is now WAY slower.. T__T . Transitions with liquid-fire freezes and glitches for 2 o 3 seconds :/ - Without liquid fire, takes around the same amount of time but instead, it renders the loading template (not the case with liquid-outled for some odd reason)

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

Ya lots of the compat layer stuff seems to be borking performance. Hopefully someone dives in

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2015

@stefanpenner do you have any pointers on where to begin looking?

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

that question feels like a trap. Start with the profiler, grab a coffee, put on some good trance music and hang on & hope for the best.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

My ember-conf talk is also related

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2015

:-) thanks

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

Rather then sleeping (now i have resorted to coffee), I did some checking and noticed a large number of small issues. Some of these issues have the potential to cause pretty hefty work amplification. Although fixing each of them will only result in a small win, we are currently in a death by a thousand papercuts scenario.

My plan is to slowly (as time permits) go through each issue in detail

  • inspection
  • root cause analysis
  • proposed solution
  • outcome etc.

Anyways the first finding, it appears far to many change related events are firing during initial render.
This is obviously not needed, since initial render should have sufficient state for initial render, and not rely on change events to complete its render ( at-least in this situation)

For reference, I just added some extra debugging code to sendEvent to see what was going on. I did this since it was near the top of the profile, but on initial render it should really be nearly unused.

The resulting instrumentation output was:

// fine
@array:before: 3
@array:change: 3
@enumerable:before: 3
@enumerable:change: 3
activate: 2
arrangedContent:before: 1
arrangedContent:change: 1

// unexpected on initial render.
attrs.context:before: 2203
attrs.context:change: 2203
attrs.layoutName:before: 2203
attrs.layoutName:change: 2203
attrs.templateName:before: 2203
attrs.templateName:change: 2203
attrs:before: 2203
attrs:change: 2203

// fine
content:before: 1
content:change: 1

// unexpected
controller:before: 2201
controller:change: 2201

// expected (these are the life-cycle hooks)
didInitAttrs: 2200
didInsertElement: 2202
didReceiveAttrs: 2200
didRender: 2202

// routing
didTransition: 1

// expected
init: 2216

// strange but to low to care about
length:before: 3
length:change: 3
model:before: 1
model:change: 1

// very might unexpected
parentViewDidChange: 2201

// expected
willInsertElement: 2202
willRender: 2200

// expected
willTransition: 1

Well, apparently we have 2200 components/views, so some life-cycle events should be happening but clearly lots of these really should NOT be happening. Not only are the dispatches wasteful but potential consumers may considerably amplify the work.

Anyways, after reviewing the instrumentation two discrete problematic areas appear:

  1. parentView <-> view is invalidating on initial render – this results in parentViewDidChange, controller:before, controller:after events.
  2. this.attrs* are invalidating on initial render

parentView <-> view

Is the smaller of the two, but also the easiest to fix quickly. Based on the assumption that we render top -> bottom (which turns out to be true), there is no reason to create a child before the parent, and as such we should be able to give the parent to the child at creation. This ensures the parentView reference is part of the initial state of the childView, and costing us 0 changeEvents.

When looking at the componentCreation code, we clearly already setup the parent -> child relationship at child creation time: ...

The offending code is actually linkChild which explicitly calls set and instance.trigger. Set correctly detects that nothing changes, and doesn't emit a observer change event, but the trigger isn't ware of this, and fires regardless. Causing some of the unexpected work.

proposed fix:

linkChild(instance) {
instance.container = this.container;
set(instance, 'parentView', this);
instance.trigger('parentViewDidChange');
instance.ownerView = this.ownerView;
},
(need to fixup some things, and write tests)

The result was a marginally improved experience (sendEvent dropped by 10-15%). Clearly more to do.

Also note: the container and ownerView should not change post initial componentCreation. ownerView is a bit dubious still so i left it for now, but i also removed container set.

this.attrs invalidations

This second one is more involved, but we can also break it down into two chunks.

  1. this.attrs itself
  2. this.attrs.*

The first this.attrs is being reset during render, I believe this is largely to do with how positionalArgs are provided:

I have left an idea for discussion:

The second this.attrs.* currently always broadcasts changes: https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/compat/attrs-proxy.js#L75-L87 Again, even if the state is the initial state.

I haven't opened a targeted issue for this one yet, but hope to after ^^

Thats all for now, ^^ will likely keep me busy for some time. During my session i did notice many other offenders, some future iteration will try to explore them further.

@marcemira

This comment has been minimized.

Copy link

commented Jul 8, 2015

👍 👍

@sandstrom

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2015

Awesome, your wizard eyes on this are hugely appreciated! 💫

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2015

I'm seeing a small improvement in 1.13.4 (using this ember-performance PR)

image

image

image

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 14, 2015

@GavinJoyce I also noticed mandatory setter was mistakenly left enabled in 1.13.x which is a mega slow down. @rwjblue has a quick fix and I suspect another release is imminent.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jul 14, 2015

I updated the 1.13.4 builds (instead of immediately releasing a 1.13.5). The builds @GavinJoyce used in ember-performance (see PR over there) did not include the mandatory-setter mistake.

@marcemira

This comment has been minimized.

Copy link

commented Jul 14, 2015

@rwjblue thanks! 👍

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2015

Ember 1.13.5 (pr) shows no significant change from 1.13.4:

image

Ember 1.12.0:

image

@devlo

This comment has been minimized.

Copy link

commented Jul 20, 2015

I am too seeing degrade in rendering performance in components... huge degrade between 1.12 and 1.13.4, it's so big that it's noticeable for user. I need loading spinners in 1.13.4 on content that i didn't use them before in 1.12, because waiting times are such higher.
In one of you blog posts you wrote:

Initial render of component-heavy pages shows some improvement in most of our tests

while using glimmer engine.
I have different components on different routes, some of them complex, some nested etc.
I do not have any depreciation's in code, i am no expert either, but i am not seeing any improvement, only slower rendering times.
I've checked some of big ember implementations, some of random sites where people use ember etc and in only about 10%-20% of them i could really see glimmer shine, in the rest of them current glimmer version would only degrade render performance without any benefit. Please correct me if i am wrong. It's not like everyone have use case with list of companies from stock market in which 100+ elements are updated every 500 ms...
I really love ember, i really love developing with ember and most of its concepts. While 1.12 version performance was only acceptable taking into consideration all the benefits of the framework, 1.13+ in this state is not..
As i said before i am no expert, so maybe some expert could answer my question.. is it possible that glimmer will be in future at least as fast as engine from 1.12 version ?

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

is it possible that glimmer will be in future at least as fast as engine from 1.12 version ?

This isn't glimmer related at all. Code above the glimmer layer is causing the issues, and large due to compatibility. My comment above begins the investigation -> #11502 (comment) and the just got +1 on fixing another piece of the puzzle #11686

GavinJoyce

Ya, this is expected. I don't believe any performance tweaks would have landed for that release. But atleast it did not regress further.

If you have time, adding more comprehensive tests to ember-performance would be fantastic.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

@rwjblue nice

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

The performance of the initial component render times seems to be significantly slower in Ember 1.13 than in Ember 1.12.

just updated the jsbin, it appears the initial render performance of legacy components (as apposed to angle brackets – which perform better) is near parity again http://jsbin.com/qecubegace/edit?html,js,output

50ms on my machine on latast release
42ms on my machine for 1.12

Although we are approaching parity, going forward:

  • we still have some more work todo on the legacy style components, getting them into a better place
  • the focus of performance will likely continue but focused more on the angle bracket components, as they exist to shed some of the hand-cuffs we have with the legacy components
  • HTMLBars is blazingly fast, our goal is to continue to make the Ember portions as close to zero-cost abstractions as possible.

Thanks for your patiences on this, and @rwjblue / @ef4 for helping drive the patches :)

@peec

This comment has been minimized.

Copy link

commented Aug 9, 2015

This is super awesome, thanks a lot for the work going on the performance issues 👍

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

This is super awesome, thanks a lot for the work going on the performance issues

more to do, @sandstrom example is still not near parity, although the original jsbin is.

His example involves more nested and likely some other factors.

@discorick discorick referenced this issue Aug 12, 2015

Merged

Discorick/additional susbcribers #80

3 of 3 tasks complete
@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2015

Complex list rendering performance continues to improve (🍻):

image

image

image

image

@rwjblue

This comment has been minimized.

Copy link
Member

commented Sep 5, 2015

A few more PR's to do less work on initial render:

  • #12297 -- Removes double looping over this.attrs.
  • #12299 -- Removes Ember.get(component, 'layout') in most cases.
@rwjblue

This comment has been minimized.

Copy link
Member

commented Sep 5, 2015

Also to note: the ember-performance suite used in some of the numbers above suffers from an issue that causes all of ember-metal to be a debug build for Ember 1.13 and above. I created an issue to track that over in eviltrout/ember-performance#49. This makes comparisons using ember-performance with Ember versions above 1.12 a bit off (since it includes Ember.deprecate, Ember.assert, etc statements).

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2015

I have a PR to update the ember-performance tests to work with 2.0 and have gathered some raw (and with a caveat) data here:

image

Spreadsheet: https://docs.google.com/spreadsheets/d/15MPfzCncD_PvM-nmfdrA-_UsUzNpAVV9pFkt3wU5rBo/edit?usp=sharing

@GavinJoyce GavinJoyce referenced this issue Sep 16, 2015

Open

Add more performance tests #53

0 of 1 task complete
@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2015

Here are some quick numbers for the Render Complex List performance test now that eviltrout/ember-performance#49 has been closed:

image

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2015

I've added the ability to run performance tests against multiple versions of ember and to show graphs, here are the latest numbers for the complex list benchmark:

image

@rwjblue

This comment has been minimized.

Copy link
Member

commented Oct 11, 2015

@GavinJoyce - Thank you for all your hard work on ember-performance!

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2015

2.2.0 seems to bring a nice initial render speed improvement:

image

@OrKoN

This comment has been minimized.

Copy link

commented Nov 17, 2015

👍 Thank you!

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

Is there a status update on this? Does Ember team recommend to use 1.12.x or 1.13.x if we want the best performance?

@ef4

This comment has been minimized.

Copy link

commented Feb 25, 2016

Is there a status update on this? Does Ember team recommend to use 1.12.x or 1.13.x if we want the best performance?

It depends on which dimension of performance matters most to your app. Re-rendering is dramatically faster in 2.x, but initial render is still somewhat slower.

But there's no performance reason to pick 1.13 over 2.2. The best choices are 2.2 if initial render is fast enough for your use case, or 1.12 if it's not. Waiting around at 1.12 until the 2.x series can be fully optimized is a reasonable choice. In that case it may be worth getting your test suite running under the latest 2.x so you'll know you're compatible.

@vanthome

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

What @ef4 writes is exactly my experience with Ember >= 2.2.0.
Initial render perf (especially in deeply nested component structures) is a subtle problem for me. I searched for some issue regarding this, but could not find one. Does anyone know whether this issue is tracked somewhere?

@sandstrom

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

@vanthome The new incarnation of Glimmer is expected to improve initial rendering. You can track its progress here: #12907 (and in its source repo)

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2016

For what it's worth, we've found the v2.2 performance on Intercom to about the same as 1.11. We didn't upgrade to 1.13, 2.0, 2.1 due to the initial render performance regression but we're going to upgrade to v2.2 on production next week. We're also keenly awaiting Glimmer2 (and helping where we can) as it's shaping up to bring significant performance improvements

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

@ef4 and all thanks for the color on this. Looking forward to Glimmer2 👍

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

One more question, I vaguely recall seeing this in another issue or blog post but will there be a version of of 1.13.x which improves performance to 2.2 levels? I'd really like to underscore the importance of an LTS release which can get performance patches, we're currently on 1.11 and it's quite a daunting prospect to upgrade to 2.2 just to get better performance.

@vanthome

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

Many thanks for the info guys! Wasn't aware of Glimmer 2

@ef4

This comment has been minimized.

Copy link

commented Feb 25, 2016

@johnnyshields A bunch of performance fixes have already landed in 1.13.x, and the latest versions there are dramatically faster than 1.13.0. I wouldn't expect to see more major work there.

But don't be daunted about the upgrade work between 1.13 and 2.2. Once you get running cleanly on 1.13 the rest of the 2.x upgrades have been a breeze, as there are no breaking changes.

Also, the first LTS release will be 2.4.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

Is Glimmer2 slated for 2.4 or 2.5?

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

We upgraded Intercom to Ember 2.2 this week and it's about 10% faster than 1.11 for initial render.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

The primary purpose of this issue was to track the performance regression for initial rendering which occurred in Ember 1.13. There are absolutely still many things to be worked on, but as mentioned by @GavinJoyce in his last comment we have generally reached parity with pre-1.13 initial rendering speed (and are definitely much faster for rerendering).

I am going to close this larger meta issue now. We will absolutely keep an eye on things as we progress towards glimmer 2....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.