fix(rootScope): mitigate chrome gc leak, closes #2624 (to the extent tha... #2646
Conversation
…extent that it can be without a patch to v8)
|
@ashaffer - thanks for this PR. Can you provide a unit test that demonstrates the GC leak somehow? |
|
I would if I knew of a way to accomplish that. Is there some way I can detect memory leaks from within the code of a unit test? That'd be a very useful API to know about. EDIT: Answered my own question. For anyone else wondering, it's in chrome. |
|
On second thought, even using the performance.memory data will be relatively useless in a unit test. There is no way to ensure that a GC cycle has been run in the time between the $destroy call and resampling the memory consumption. The only way I can think of to do this would be to have the test runner execute chrome with the --expose-gc flag. But this would also require changes to karma, would it not? |
|
Can you identify exactly what is leaking? Scopes or elements? Does this occur only in Chrome? Is it a bug in Chrome? |
|
It's only scopes directly leaking, but the leaking of scopes causes other things to leak (because whatever else the scope retains a reference to is also retained). AFAIK it occurs only in Chrome and is a bug in V8. I reported it to them as well here: https://code.google.com/p/v8/issues/detail?id=2682 It is related to that commit in master in the sense that it is a similar bug, and may have the same root cause within V8 as the bug my patch addresses. |
|
Updated the patch to use delete instead of assigning to null in case someone has used defineGetter on the scope. |
|
I tried the patch and it does not consistently prevent the leak (it still leaks most of the time). So I don't think that we should merge this change in. I'm going to bug the V8 team, because this is definitely a V8 issue rather than Angular issue, see the screenshot for confirmation: |
|
Ya, I guess I should have been more clear. The patch does not prevent the leak, it just seems to leak substantially fewer scopes when its in place. And more importantly, at least for the application i'm developing, deleting everything on the scope at least allows those objects to be garbage collected so that not every piece of data ever rendered by the application has to hang around forever. It doesn't fix the leak, it just substantially reduces its impact. |
|
I've email the v8 guys. Last time we complained about this issue (and were promised a fix) they referenced this bug: https://code.google.com/p/v8/issues/detail?id=2073 (which doesn't look related, but I guess represents one symptom of a bigger issue). Let's way to see what they say before we merge or reject this PR. |
|
Ah yes I had found that bug and gotten excited that perhaps it was fixed, but the patch for it ( https://code.google.com/p/v8/source/detail?r=13666 ) was reverted before it made it into a real release, apparently because it was causing crashes or something. I think its related because the fix for that issue was to enable weak embedded maps, and the Child scopes are being retained by map descriptor objects which themselves are retained by optimized compiled code. So it seems probable to me that's the same issue, and that the fix is as simple as enabling that flag, it just looks like they're having trouble making the weak embedded maps stable. Hopefully they'll figure it out soon. |
|
The V8 team has made some progress and the current canary should be significantly less leaky. There are still some more fixes pending, but we should see an improvement soon. |
|
I'm going to close this PR as I'd rather avoid adding more hacks to angular when the proper fix can be done only in v8. Thanks for the effort and discussion! |
...t it can be without a patch to v8)