Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Memory leak with nested ng-repeats #2624

Closed
ashaffer opened this issue May 10, 2013 · 10 comments
Closed

Memory leak with nested ng-repeats #2624

ashaffer opened this issue May 10, 2013 · 10 comments

Comments

@ashaffer
Copy link

This bug is a little strange, and i'm not sure whether angular is to blame or V8. I think ultimately its V8's issue, but there may or may not be things angular can do to mitigate it.

I've created a plunkr to demonstrate:

http://plnkr.co/edit/0xtIMLnl08h5BjGVDNug?p=preview

Let it run through its changes (should take only a few seconds), and then take a heap snapshot. Search for the 'Child' constructor and you should find that there are substantially more objects hanging around than there should be.

If you expand the Child objects list you should see most of them highlighted in yellow and some not highlighted at all - the un-highlighted ones are the ones that have been $destroy'd but are for some reason still resident in memory. You can check this by hovering over one and noting that $$destroyed is set to true on all of them. Note in their retention trees the reference to 'prototype' inside of $new. I can eliminate all of the references in their retention trees without modifications to angular except this one.

I have written a quasi-fix for this that is commented out currently in the plunkr. If you comment back in my replacement for $new (at the top of the file) and let it run again, then you should see that something similar happens, but hopefully with somewhat fewer extraneous scopes hanging around. However, for the few that are left, if you examine their retention trees they will be empty, which leads me to believe that this is a bug in V8's garbage collector.

This is as far as i've been able to get with it on my own. I'm not sure if there is some hack to force V8 to garbage collect these or if we'll just have to wait for them to fix it.

I'm happy to send a pull request with my fix, but I don't really feel confident that its right or that it fully addresses the issue, so I thought it would be better to submit this as an issue first to see what you guys think.

Edit: I should point out that I have seen this happen on a single un-nested ng-repeat, but I wasn't able to create a toy example to reproduce that.

@ashaffer
Copy link
Author

Update: After discovering the 'show hidden properties' option in the profiler, I think i've traced the issue back to dangling references retained by angular's jqCache object. I'm not sure exactly why this is occurring though.

@ashaffer
Copy link
Author

Upon even further investigation, the problem disappears (with or without the code I added to $new) when chrome is run with

--js-flags="--nocrankshaft --nouse-ic"

So it seems its definitely a bug with v8's maps being retained by optimized code.

It looks like this patch in V8 may address the issue: https://code.google.com/p/v8/source/detail?r=13666
but it's hard to say.

ashaffer added a commit to ashaffer/angular.js that referenced this issue May 13, 2013
…extent that it can be without a patch to v8)
@IgorMinar
Copy link
Contributor

@btford btford closed this as completed Aug 24, 2013
@btford
Copy link
Contributor

btford commented Aug 24, 2013

As part of our effort to clean out old issues, this issue is being automatically closed since it has been inactivite for over two months.

Please try the newest versions of Angular (1.0.8 and 1.2.0-rc.1), and if the issue persists, comment below so we can discuss it.

Thanks!

@maliarov
Copy link

Still exist in 1.0.8 and 1.2.0-rc.2... nested "ng-repeat" cause dramatic memory grow (DOM elements are not releasing). My app eats 70 megs per view change :( I got "ng-repeat" of directive with "ng-repeat" inside (list of products with their variations).

tested on:
Stable: Chrome 29.0.1547.66 m
Canary: 31.0.1626.3 canary Aura SyzyASan

IE, Safary, FF - ok

@btford btford reopened this Sep 11, 2013
@ashaffer
Copy link
Author

I also discovered today that this was seriously impacting my end to end tests. I have been using the hack in my PR in my copy of angular, but I hadn't applied it in angular-scenario, which has its own instance of the scope code.

Some of my larger e2e tests allocated upwards of 100mb that never got released, and was having serious negative impacts on performance. Applying the fix in my PR resolves this. I know its a hack, but since it doesn't seem to be a priority for the v8 team to fix this it seems like it might be worthwhile, as the negative impact is pretty significant in some cases.

@sunra
Copy link

sunra commented Oct 18, 2013

angular 1.2.0-rc.3 - problem is present. How to use Angular in production if he crashed chrome constantly...

@BankRen
Copy link

BankRen commented Jan 2, 2014

@ashaffer @btford
In scope.$destroy(), it just clear the relationship from parentScope. It worked well at Chrome, but it not enough at IE. We should recursion invok childScope.$destroy() and clear all $$watchers to release the object reference at IE.

  $destroy: function() {
    // we can't destroy the root scope or a scope that has been already destroyed
    if ($rootScope == this || this.$$destroyed) return;
    var parent = this.$parent;       

    //clean children 
    var child = this.$$childHead;
    var curentChild;
    if(child){
        do{          
            curentChild = child;
            child = child.$$nextSibling;
            if(curentChild){
                curentChild.$destroy();
            }
        }while(child);
        curentChild = null;
    }

    this.$broadcast('$destroy');
    ......
    //after set "this.$$childTail = null;", need clean watchers
    if(this.$$watchers){
        this.$$watchers.length = 0;
    }        
  }

@IgorMinar
Copy link
Contributor

@BankRen, @sunra can you please open a new issue with reduction that reproduces the memory leak?

@IgorMinar
Copy link
Contributor

@ashaffer the v8 bug that was causing this leak is now fixed. I'm going to close this issue. if you still see mem leaks, please open a new issue with a link to plunker/jsfiddle app that reproduces the leak.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants