ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.96k stars 13.51k forks source link

[v4 beta-13]: perf: Rendering / performance regression compared to v3 rendering #15953

Closed gitdude1458 closed 5 years ago

gitdude1458 commented 5 years ago

Bug Report

Ionic Info

Ionic:
   ionic (Ionic CLI)             : 4.2.1 
   Ionic Framework               : @ionic/angular 4.0.0-beta.13
   @angular-devkit/build-angular : 0.7.5
   @angular-devkit/schematics    : 0.7.5
   @angular/cli                  : 6.1.5
   @ionic/angular-toolkit        : 1.0.0

System:

   NodeJS : v10.11.0 (/usr/bin/node)
   npm    : 6.4.1
   OS     : Linux 

Describe the Bug

Ionic v4 seems to suffer from huge performance degradation and visual artifacts when re-rendering template compared to V3.

The issue is visible even on very small form with segment button conditionally rendering a form with ion-list with just 6 elements.

Ionic V3: The whole screen changes at once, providing good user experience.

Ionic V4: The rendering is slower, doesn't happen at once, instead visual artifacts are rendered (first elements are not rendered at all, then some of them are rendered without CSS styles, then CSS is applied).

Those artifacts are very annoying in our production app where the issue is much worse than here and it was noticed by all beta users / developers. We had to stop our migration process before this issue is resolved.

They're mostly okay on fast mobiles, but for example on iPhone 5S the repainting / reflowing is clearly visible.

Steps to Reproduce Steps to reproduce the behavior:

  1. Open the sample app (or any Ionic app that uses *ngIf to conditionally render Ionic components)
  2. Switch to Chrome Dev Tools / Performance, enable CPU 6x slowdown and run profiling, enable screenshots
  3. Click on the segment buttons, stop profiling
  4. Check the screenshots in Chrome
  5. Ionic v3 renders fine (switches from state A to state B), v4 has visible visual artifacts

Related Code https://github.com/gitdude1458/ionic-v4-rendering-slow

Video: V3: https://raw.githubusercontent.com/gitdude1458/ionic-v4-rendering-slow/master/v3.webm

Video: V4: https://raw.githubusercontent.com/gitdude1458/ionic-v4-rendering-slow/master/v4.webm

It's better to use Chrome screenshots to see the artifacts, video has low FPS.

Expected Behavior Ionic V4 should render at least as well as V3.

Additional Context Problems are much more visible in our production app, esp. on iPhone 5S and slower Android phones.

alan-albuquerque commented 5 years ago

I realized that version 4 has a huge performance problem. I use a Samsung Galaxy Note 4 to test the app on Android, and my old app (Ionic v3) was running smoother than the app I started creating with version 4 (mostly animations). I know it's beta, but I think that's a major problem. I expected that version 4 was more performatic, but I think I'll give a change to the RN. =(

@gitdude1458 it's look like your videos are broken to me.

jase88 commented 5 years ago

Videos are not working for me either.

Example is realized with ionic4 Angular, right? Might be interesting to compare your results with Ionic4/StencilJS components (without Angular or Vue) to get a indicator what is the reason for the poor performance.

gitdude1458 commented 5 years ago

Strange, the videos worked for me on FF, but I have reencoded them anyway, so hope they work now. Anyway I have been looking around the source code to find clues about this huge performance regression and noticed this:

https://github.com/ionic-team/stencil/blob/master/src/client/queue-client.ts

It seems that Stencil is placing all DOM modifications / reads in queue (not bad), but seems to flush the queue just enough to hit 22 FPS, so when there are more modifications, they are scheduled by requestAnimationFrame for next rendering.

This seems to align pretty nicely with what I have seen so far in performance tab - multiple frames being rendered, but partially, thus leading to artifacts.

While this queue probably makes huge sense in lot of scenarios, I think it may not be the case in others where it is simply preferable to just do the rendering as quickly as possible.

I'll try rebuilding Ionic with modified Stencil to see how much it improves the situation if I force it to drain the whole queue.

gitdude1458 commented 5 years ago

Looks like my suspicion was right. I tried following things:

  1. Increase the timeout in queue-client.ts - helped - you could still see some visual artifacts - but at least not partially styled / updated components - the text would still scroll though

  2. Disable use of requestAnimationFrame in Ionic - this actually helped a lot, now the whole UI was rendered at once (while lowering FPS, but that is not the issue).

Ionic V4 is still slower than V3 when it comes to rendering, but at least those visual artifacts / visible partial renderings are gone. By no means I believe disable requestAnimationFrame is proper solution at all, it's ugly hack. I think Ionic team needs to implement some sensible frame batching to properly update the content of screen. It seems to work fairly well for page switching, but very poorly when individual template changes (which I believe is totally legitimate thing to do and a must for lot of apps) and/or is complex.

But anyway in case anybody is facing similar issue, it is possible to workaround the rendering artifacts by completely disabling requestAnimationFrame by placing this code in your app init:

(<any>window).Ionic.raf = function(cb: any) { cb(); };

It's dirty, ugly hack, it doesn't make the rendering faster (it's still slower than V3), but at least it doesn't show partially rendered layout, which was huge issue on slower phones in our app

alan-albuquerque commented 5 years ago

@gitdude1458

screenshot from 2018-10-16 09-45-09

MarkChrisLevy commented 5 years ago

@gitdude1458 Thanks for this issue - I was about to report the same thing, especially when it comes to partial rendering.

mhartington commented 5 years ago

I think what's happening here is they async rendering of the component. Since the components render async, you're getting the "waterflow" effect as they each render and become hydrated. It's not technically a performance issue, though it is pretty jarring. Looking into a fix

jordanpurinton commented 5 years ago

Same thing is happening on my end. Specifically when rendering forms asynchronously.

godenji commented 5 years ago

Looking into a fix

@mhartington any idea when potential fix will land in v4 beta? This is the single most noticable bug in v4 that all users of Ionic v4 apps see clear as day. Certainly business owners are less likely to signoff off on migration to v4 when animations/transitions in v3 are virtually flawless.

ptitjes commented 5 years ago

@gitdude1458 I had the same conclusions as you: https://github.com/ionic-team/stencil/issues/1107 There you'll find screenshots of the rendering performance, captured by Chrome DevTools, and StackBlitz demos that exhibit that rendering a 200 items list takes 100ms on Ionic 3 compared to 1500ms on Ionic 4!

@mhartington, as said by @gitdude1458, the problem comes from forcing rendering to happen during requestAnimationFrame:

When I did force the flush of the queue in one shot, by removing that "less than timeout" here in Stencil, then it had almost (still 2 times slower) the same performance as Ionic v3.

While working DOM only during requestAnimationFrame makes sense when you are doing few changes while having animations, it does not when you have lots of changes at a time. I think it is a very big drawback of the way Stencil currently works.

I really expect that you guys provide a way to toggle this behavior before the final release. In its current state performance-wise, it would be unprofessional of me to deliver an application based on Ionic 4.

Anyway, that is not very reassuring even if it is a beta!

manucorporat commented 5 years ago

We are well aware of this issues and we have already made further changes in ionic beta.15 to improve the situation.

We also have an on going research to reduce drastically the cost of initialisation of web components with stencil, check out some WIP here: https://github.com/ionic-team/stencil/pull/1192

gitdude1458 commented 5 years ago

The changes in v15 helped, but it's still worse than disabling the requestAnimationFrame completely. For example when using ion-select:

I agree with @ptitjes - this is so far the single most problematic issue with Ionic v4 - delivering app with such visible performance issues would be unprofessional to say at least, so I hope it gets solved.

gitdude1458 commented 5 years ago

Looking into the source code it's pretty obvious why: In

 const async = App.asyncQueue !== false;
...
    const timeout = async
      ? now() + (7 * Math.ceil(congestion * (1.0 / 22.0)))
      : Infinity;
...

    if (!rafPending) {
      rafPending = true;
      App.raf(flush);
}

In other words, flush is run and flushes the whole queue. The problem is that the flush is ran during requestAnimationFrame - but if the rendering takes multiple frames (each operation is queued individually), visual artifacts are still displayed, although, less likely.

I think Ionic needs to detect page update / busy cycles and queue those updates in one animation frame (even if takes more time) to avoid visual artifacts and/or stop using requestAnimationFrames for anything besides animation (it doesn't really makes much sense when redrawing a page completely to use it )

amitairos commented 5 years ago

@gitdude1458 Can you explain this hack please? How do I do it?

gitdude1458 commented 5 years ago

@amitairos See my comment above: https://github.com/ionic-team/ionic/issues/15953#issuecomment-430167830

If interested, place that code in your app component's constructor. No guarantees, it's dirty hack :-)

amitairos commented 5 years ago

@gitdude1458 Thanks. Maybe helps a little, but after playing a bit with the app and showing animations multiple times etc., it starts lagging, particularly while doing animations.

ionitron-bot[bot] commented 5 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.