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
51.09k stars 13.51k forks source link

Memory leak in state change #1096

Closed perrygovier closed 8 years ago

perrygovier commented 10 years ago

It appears that changing state increases memory usage with every transition. Here's a simple example. Open your browser's memory profiler and click "Get Clicky".

http://codepen.io/perrygovier/pen/FJgBa

Pages with more stuff on it tend to increase the amount of memory that gets eaten with each transition. Note the number of nodes and listeners never decreases.

screen shot 2014-04-09 at 5 31 45 pm

On my app that uses a few larger lists, the app can quickly get to 200mb after just a few mins of manual clicking back and forth. I have yet to crash the app due to memory pressure, but it seems more a matter of impatience than something that wouldn't happen with continued use.

I believe this is less an issue with Ionic and more to do with Angular's UI Router, but I thought I'd start here as it's where it popped up for me.

BountySource discussion that appears related

adamdbradley commented 10 years ago

https://github.com/angular-ui/ui-router/issues/545

perrygovier commented 10 years ago

There's a parallel discussion that lead to this ticket being opened. It appears other users are seeing what I am in where larger apps escalate quicker.

http://forum.ionicframework.com/t/memory-leak/2823/4

More interesting, Aritzg created an example in pure Angular, and while memory spiked, it was quickly garbage collected. I'll try to whip up a demo this weekend to confirm. Perhaps there's something Ionic's doing to prevent or limit that GC event?

perrygovier commented 10 years ago

I've done a bit more digging, but it's still a mystery what the root cause of the issue is. I ran some benchmarks against a plain UI Router example, matching the version Ionic is using. It does appear that the leak in ng-animate has been fixed.

This is code I used for these tests https://dl.dropboxusercontent.com/u/9006564/test.zip

First, without ng-animate screen shot 2014-04-13 at 11 31 29 am

Then, with ng-animate screen shot 2014-04-13 at 11 33 41 am

Note the immediate drop in memory after each page transition using ng-animate, that's to be expected. Both are reduced back to near their original size once the CG button is clicked at the end. The stepping in the first one was automatic GC before I was able to click the button.

It has been suggested on the UI Router github tickets, that the node and listener count perpetually increasing is actually a bug in web inspector, and can be ignored. Unfortunately, Safari and Firefox's inspector do not offer node or listener count to confirm this.

So UI Router may not be the sole cause of the leak. Taking a closer look at Ionic:

Here's a heap comparison (delta) going from the main page to another and back on the above UI Router demo, and the Ionic tabs demo.

Angular UI Router example screen shot 2014-04-13 at 1 19 49 pm Here's the Ionic tabs demo screen shot 2014-04-13 at 1 21 43 pm

These lists can be pretty intimidating to sift through. Worth noting however, is that the node list has a zero delta in plain UI Router, and has grown by 2240 elements in Ionic. Now, after the first dashboard -> friends -> dashboard cycle, this could be just caching. However, this is after the third cycle, so I think the node delta should be 0.

More interesting are the elements in the Detached DOM Tree arrays. In it, I find markup from the previous pages. These detached DOM tree elements explain why having larger pages, large graphics, or video exacerbate the leak. I believe this is the primary culprit for the memory leaks that have gotten to be in the hundreds of MBs.

screen shot 2014-04-13 at 2 47 55 pm

Upon selecting any element in this list, I believe things that show up in yellow in the window below, indicate a reference keeping them from being released. Many of these reference createEventHandler() of the jQLight library. As plain Angular uses this too, I'll have to think about next steps in tracking this down. Maybe set up a log for that method, and then cross reference the elements with the list of elements not being released.

The search continues.

Hope this was helpful.

TLDR: It looks like UI Router has fixed most of its problems, and the others are the inspector's fault. It appears that some event listeners aren't removed on state changes, keeping those old elements in memory even after they're removed from the DOM.

adamdbradley commented 10 years ago

This is great, thanks for digging into this. So you said UI Router has fixed most of its problems, so do you mean its fixed its problems in version 0.2.7, or 0.2.10? Right now Ionic is using 0.2.7 because at the time 0.2.8 had bugs and we've been holding back and upgrading.

aritzg commented 10 years ago

Hi. I have just updated to 1.0 beta.2 and replaced ui-router 0.2.7 with 0.2.10 but still the memory leak remains.

ajoslin commented 10 years ago

It currently is not leaking for me in Chrome 34.0.18 OSX, given the example in the OP and using ui-router 0.2.10 with the nightly build.

Here's a codepen with 0.2.10: http://codepen.io/ionic/pen/fa145096b86fc28c81006da84028e28e/

perrygovier commented 10 years ago

Still seeing a number of detached DOM trees, but only a single stray event listener, and the memory impact after initial caching and GC is negligible. Might be a good idea to test with more complicated templates, but it's looking good to me.

ajoslin commented 10 years ago

We should definitely get those stray event listeners then!

How can you tell with more clarity what is leaking? I don't really know how to use the memory profiler besides to look at the graph.

aritzg commented 10 years ago

I will try to reproduce my case in CodePen. Until now, the bigger the dom objects were (say images, item lists with images), the bigger the memory leak was.

perrygovier commented 10 years ago

I don't understand it completely either, but in profiles, you can take multiple heap snapshots and then change from the summary view to the comparison view, seeing the changes in memory between two points. As you dig down, I'm not sure how to interpret everything, but I know detached DOM trees are DOM elements that are no longer in the DOM but are either waiting for GC or are being held on to by stray listeners or something similar.

After clicking through each page and populating the cache, it's my understanding there shouldn't be any additional detached DOM trees. Still, this is substantially less of a problem than it was.

aritzg commented 10 years ago

I have just forked @ajoslin 's pen

http://codepen.io/aritzg/pen/beaFy

As you will notice, I added a fourth tab called 'list' which shows a 6 item list. each item has a name, a description and an image.

I you try the "get clicky" button and start a record (In Chrome ) Developer tools -> Timeline --> Memory , you will see used memory will grow. From time to time, GC will do its job but memory baseline will keep growing.

Regards!

FrancoAA commented 10 years ago

Any update about this issue? Because I tried upgrading router-ui but the memory leaking persists

ajoslin commented 10 years ago

Working on it.

ajoslin commented 10 years ago

I just fixed a memory leak: scrollView was adding listeners to the document and never removing them.

There might still be more leaks, though.

Could you guys investigate with the newest nightly (once it's up, about 30 minutes after this post)?

Thanks all.

robdmoore commented 10 years ago

Cool! @graemefoster and I suspected this today while profiling our app. Good to see a fix has been put in. I'll give it a whirl later in the week and see what impact it has.

aritzg commented 10 years ago

HI @ajoslin . First of all thanks for your efort.

I forked the codepen published before.

With 1.0.0 b3 http://codepen.io/aritzg/pen/oxdaf

With nightly http://codepen.io/aritzg/pen/ohFyq

I benchmarked both but unfortunately IMHO, the memory leak remains.

Here the results.

benchmark

Blue marks refer to the moment when I click on "get licky". Grean mark refer to the moment when I force GC manually

I am java/web developer and not very skilled with JS so, by now, I can not offer much more help than this.

Hope this helps.

Regards

ajoslin commented 10 years ago

Hi @aritzg,

Thanks for the benchmarks.

We'll continue to look at this.

thechrisroberts commented 10 years ago

I'm seeing a pretty nasty leak that I assume is related. I've got an app with a side menu and four tabs. First tab contains a Mapbox embed. Switch between tabs and memory grows and grows. This is particularly pronounced with the mapbox view.

I've thrown together a Codepen with a similar structure showing the effect. The mapbox view is pretty basic with no pins and the other tabs are minimal so the memory growth is not much (though any leak should be plugged) but in my actual app it is quite serious, leading the app to crash after a short period of usage, rendering it essentially unusable.

http://codepen.io/thechrisroberts/pen/IowtF

The image below shows snapshots after the initial load, then switching to tab 2 and tab 3, then back to tab 1. Memory use inches up every step.

screen shot 2014-06-10 at 9 33 06 am

The next image shows the app I'm working on: initial load, switch to a couple of tabs, and switch back to the tab with the map.

screen shot 2014-06-10 at 9 35 09 am

ajoslin commented 10 years ago

Hi @thechrisroberts,

Thanks for the information. Does the mapbox have a 'destroy' or 'cleanup' method you can use?

For example:

var map = /* ... */
$scope.$on('$destroy', function() {
  map.destroy();
});
thechrisroberts commented 10 years ago

It does, just updated the pen to use it. I experimented with that in my app and it didn't seem to make a difference but it looks like it does help in the pen. I'll look at it a bit more in my app.

thechrisroberts commented 10 years ago

Just did a little more experimenting with my app. In addition to Mapbox I'm using markerclusterer. Destroying the mapbox did not destroy the markers; I had to remove those before removing the map. Saves a nice chunk of memory, but I still have memory growing when I switch between my other tabs which use fairly straightforward lists: image, text, that's it.

aritzg commented 10 years ago

Hi @ajoslin

I benchmarked again Ionic 1.0 beta.3 VS Nightly (20140612)

image

As shown in the image above there is a great improvement regarding memory leak.

Now, after the "get Clicky" process,once I force the GC, the memory usage falls.

But after repeating the process several times, the memory baseline keeps increasing 4 to 5 MB in each iteration, wich is way better than it was in previous versions.

I think it is still not resolved completely but it is almost there.

Congratulations!

bappy004 commented 10 years ago

I still see around 1.1MB memory leak when using the tabs app and a map inside one of the tabs. Using 1.0.0 b7. Is there any update on this issue? Thanks.

ajoslin commented 10 years ago

We're still investigating, and haven't found anything reliable yet. It sounds like tabs is definitely the culprit, I will do an investigation of the tabs directive this week.

Werbelow commented 10 years ago

screenshot 2014-06-24 10 51 45 I am also having a lot of memory issues and think that it is causes by tabs. The longer I have the app open and the more times I click on the tabs, the higher the memory usage goes. It never seems to GC and just keeps increasing.

bappy004 commented 10 years ago

Any update on this? We're eagerly waiting for a solution to this problem.

Thanks.

Kind regards

Nizamul Morshed

On Wed, Jun 25, 2014 at 3:07 AM, Travis Werbelow notifications@github.com wrote:

[image: screenshot 2014-06-24 10 51 45] https://cloud.githubusercontent.com/assets/5963934/3374569/eb5c9f5e-fbbf-11e3-8251-22f6bf6c4ce7.png I am also having a lot of memory issues and think that it is causes by tabs. The longer I have the app open and the more times I click on the tabs, the higher the memory usage goes. It never seems to GC and just keeps increasing.

— Reply to this email directly or view it on GitHub https://github.com/driftyco/ionic/issues/1096#issuecomment-46999593.

cuipengfei commented 10 years ago

My app is experiencing leaking too. After switching between different lists(ng-grid), the memory usage goes to 900 plus MB, and the app becomes almost stuck.

kentongray commented 10 years ago

I'm not sure if this is anyones problem, but thought I'd throw it out there, because we spent a while tearing our hair out over a really large leak (1-2mb per transition). We were convinced it was ionic when it seems like in reality it was an issue with angular 1.3 beta https://github.com/angular/angular.js/issues/6794 & https://github.com/angular/angular.js/issues/8105

Upgrading to the latest beta solved the problem. Good luck to everyone else :rage4:

cuipengfei commented 10 years ago

My problem is solved, it was a bootstrap plugin that's holding on to the dom tree.

ravishivt commented 10 years ago

@cuipengfei Which bootstrap plugin?

cuipengfei commented 10 years ago

@ravishivt nya-select

dlongley commented 10 years ago

I left a comment on https://github.com/angular/angular.js/issues/4864#issuecomment-50540515 with a fix that might be related to your issue here. I noticed that when certain angular animations get canceled, detached DOM nodes weren't being reclaimed.

jbasinger commented 10 years ago

My team and I are also having a leak issue. Initially we thought it was ui-router, so I made this plunker to test it out http://embed.plnkr.co/rtIGJCG5ahlocHJ0gdmb/preview

There was no leak using just ui-router. Then I tried it with Ionic and it seems to be leaking when you go back and forth between the Main and Leak states using $state.go http://embed.plnkr.co/ZpeGhZzLywIIP2RMjYLX/preview

I measured by taking heap snapshots in Chrome Dev Tools and in doing so you can see the Detached DOM tree entries rising and an (array) entry rising considerably. You need a lot of nodes to make a large dent in the memory usage so the Leak state is pretty big. You can recreate what I did by taking a snapshot before going anywhere, then going to the Leak state and then back to the Main state and taking another snapshot and repeating the process a few times. The heap size never falls.

Here is a screenshot of the snapshots, I went back and forth a few times between snaps 3 and 4 to enunciate the leak http://i.imgur.com/0H6WCWf.png

I'm not too sure how to read heap comparisons, so any kind of help would be great.

dlongley commented 10 years ago

@jbasinger, have you tried to see if the change to animate.js that I proposed (https://github.com/angular/angular.js/issues/4864#issuecomment-50540515) helps at all?

Here's the diff:

           if(removeAnimations || !data.totalActive) {
+            data.active = data.last = data.node = null;
             element.removeClass(NG_ANIMATE_CLASS_NAME);
             element.removeData(NG_ANIMATE_STATE);
           }
perrygovier commented 10 years ago

I merged in @jbasinger's PR and I'm not seeing any more detached DOM nodes. Digging in, to the heap, what little difference there is seems to be JS internals, jQuery Cache, and new instances of items, with a zero size delta. Gonna call this one fixed. Let me know if you're still seeing anything. screen shot 2014-09-08 at 3 05 14 pm

kamleshkoringa commented 10 years ago

I am using latest nighty (1.0.0-beta.11-nightly-431) but still have memory leak issue. Please check heap snapshot. Please check image 2 it shows jqlite contains object from my previous page. Please let me advise if I have to take care in my code for this issue.

image

image

aritzg commented 10 years ago

@kamleshkoringa I tested it yesterday with nightly-432 (2014-09-08) and I think it was (nearly) OK reagarding memory leak issues.

This is my benchmark http://codepen.io/aritzg/pen/ohFyq

Could you reproduce the leak with nightly-432 o nightly-433?

Regards

zmacomber commented 10 years ago

@aritzg - I tested via http://embed.plnkr.co/e0toG4auJVYuDn558m2r/preview and saw the heap memory in Chrome's dev tools via their profiler increase roughly 2MB each snapshot (after toggling between the "Main" and "Get Leaky!" buttons). index.html points against the nightly-432 build.

Is that plunker set up right? Should I be pointing against a different build?

perrygovier commented 10 years ago

There is some caching that is about that size. Click through each tab before hitting the "Get Leaky!" link, and then hit the trash can to manually trigger GC. Are you still seeing it then?

zmacomber commented 10 years ago

@perrygovier - Still seeing it after clicking through tabs and manually triggering GC.

dlongley commented 10 years ago

@perrygovier, @aritzg: If I run http://codepen.io/anon/pen/yuLHw I see leaks via Chrome's dev tools. I changed the run time on the plunker to 5 seconds here as it doesn't really need to run that long to see the leaks.

I used the "Timeline" mode in Chrome's dev tools to look for a leak. This was the process I used:

  1. Click on "Scientific Facts" link and the "About", "Contacts", and "List" links to ensure that any long-lived detached DOM elements are cached for those pages before starting the test.
  2. Go to the "Timeline" tab in Chrome's dev tools and click the garbage collection icon to ensure the test is starting out cleanly.
  3. Click the record button and then click the "get clicky" button.
  4. Wait 5 seconds until the simulated clicking has stopped and then click the garbage collection button.
  5. Click the record button to stop the recording.
  6. Double click the timeline display to get a view of the whole 5 second recording and observe the memory graph.

The results before the clicking operation were:

JS Heap Size: 19013920 Nodes: 2630 Listeners: 206

And after:

JS Heap Size: 20262264 Nodes: 3252 Listeners: 244

You can repeat this same process and the numbers will continue to increase. As you can see, not everything is collected.

When solving these kinds of leaks, I tend to focus on listeners and nodes because it's easier to determine that those are actual leaks. I work to ensure that no listeners are leaking first. Finding extra listeners is often easier and doing so can solve the node and heap size at the same time. Of course, sometimes there's still something lingering around that needs to be inspected more closely.

In any case, there are still leaks here.

dlongley commented 10 years ago

I don't think http://embed.plnkr.co/e0toG4auJVYuDn558m2r/preview is a good test because it doesn't seem like you can manually create any long-lived detached dom elements before starting the test. That makes it more difficult to easily ensure that what gets created during the "Get Clicky" phase isn't something that should be hanging around in cache. A good test for a leak here would be to ensure that an environment is set up that has already created whatever nodes need to stay resident offscreen before the actual test phase so you know anything created during it must be collected (or else there is a leak). You also want to make sure that the test phase actually has a clear end (I didn't inspect the code for the plunker above ... but it seemed like it was just continuously generating events without end).

zmacomber commented 10 years ago

@dlongley - thanks for the info. I'm coming from issue #1993 which was closed and where we were directed to post here. The plunker I posted is roughly equivalent to the ones done on issue #1993.

zmacomber commented 10 years ago

@dlongley - I just ran a similar process as you did using http://codepen.io/anon/pen/yuLHw. The only thing I changed in that codepen was the references of "nightly" to "1.0.0-beta.12" in the link hrefs and script srcs in the head section. I clicked through "Scientific Facts" and the other tabs and then clicked on garbage collection on the timeline tab. I then clicked "get clicky" and after it settled down, I took a heap snapshot on the profiles tab. I took a total of 5 heap snapshots after repeating the above steps and found that the heap snapshot memory stayed right around 11MB for me each time.

Maybe after all "1.0.0-beta.12" has fixed these issues? I'm wondering if you see the same thing if you run those steps. If the heap memory stays about the same, does that show that the leakiness is gone?

dlongley commented 10 years ago

@zmacomber, I set the ionic-angular.js script to use "1.0.0-beta.12" and ran it in Timeline mode as described above. I was still seeing leaked listeners and nodes. IMO, it's a better idea to use that mode to get decent granularity and be more precise in ensuring there are no leaks.

jbasinger commented 10 years ago

I tried this again with nightly and it seems to be fixed. Previously I was constraining my heap snapshots to only 3-4. I let it go a couple more times and it dropped back down to appropriate levels.

memorydrop

Running a timeline shows the same results. I'm not sure what's being held onto until the GC comes around, but memory does seem to get up there. At least it comes back down pretty quickly. Thanks Drifty bros! Keep up the good work!

mica16 commented 9 years ago

Using the latest nightly build (908), seems that the memory leak still happens while switching between tabs. Even with simple navigation inside one specific tab.

Has anyone experienced it too?

perrygovier commented 9 years ago

Quick check, what do the tabs contain, and, are you cycling through each tab before taking your first snapshot. Also, be sure to take your snapshots in icognito mode, since plugins tend to be quite leaky as well. There are still opportunities for a leak to occur, but major leaks should now be a thing of the past.

perrygovier commented 9 years ago

I've run through our example apps, and the only growth outside of JS's internals (stuff in parens) is the array of view objects in the history. So it does grow, but only a tiny bit, and it's as expected, each view in the history array just has 6 or so string properties. Could you shed any more light on how to reproduce it? Specifically, do you have large arrays or objects in a given page's $scope?

mica16 commented 9 years ago

Hi, Perry.

I've just found the reason. Actually, I was attempting to remove the ionicModals (as recommended in the Ionic doc) in the destroy event of the scope. However, it will never reach for some other reasons. Explaining a memory leak, since while transitioning between views, previous modals were not eligible to GC and new ones were "stacked".

Therefore, I refactored my code to effectively call this remove() method. I've got now an average of consumed memory of 70mo, instead of 130Mo I could reach... I was afraid that the GC would have some issue.. but it was totally my fault. The whole on Android and iOS works really great with the 908 nightly build.

Thanks for your patience!

Michael