This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
fix(Scope): aggressively clean up scope on $destroy to minimize leaks #6856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. The theory is that the V8 leaks are due to inline caches which are caches built on the fly to speed up property access for javascript objects. By cleaning the scope object and removing all properties, we clean up ICs as well and so no leaks occur. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se?p=preview Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes angular#6794
|
Given the nature of the issue, it is hard to know if the patch works. Now, if it does, I think this patch is better than the other proposed solutions as it keeps all the hacks in one place |
|
LGTM! Will merge. |
IgorMinar
added a commit
that referenced
this pull request
Mar 28, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. The theory is that the V8 leaks are due to inline caches which are caches built on the fly to speed up property access for javascript objects. By cleaning the scope object and removing all properties, we clean up ICs as well and so no leaks occur. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se?p=preview Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes #6794 Closes #6856
troch
pushed a commit
to troch/angular.js
that referenced
this pull request
Mar 30, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. The theory is that the V8 leaks are due to inline caches which are caches built on the fly to speed up property access for javascript objects. By cleaning the scope object and removing all properties, we clean up ICs as well and so no leaks occur. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se?p=preview Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes angular#6794 Closes angular#6856
IgorMinar
added a commit
to IgorMinar/angular.js
that referenced
this pull request
Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. V8 leaks are due to strong references from optimized code (fixed in M34) and inline caches (fix in works). Inline caches are caches that the virtual machine builds on the fly to speed up property access for javascript objects. These caches contain strong references to objects so under certain conditions this can create a leak. The reason why these leaks are extra bad for Scope instances is that scopes hold on to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak. This change removes references to objects that might be holding other big objects. This means that even if the destroyed scope leaks, the child scopes should not leak because we are not explicitly holding onto them. Additionally in theory we should also help make the current scope eligible for GC by changing properties of the current Scope object. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. This is a second attempt to enhance the cleanup, the first one failed and was reverted because it was too aggressive and caused problems for existing apps. See: angular#6897 [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes angular#6794 Closes angular#6856
IgorMinar
added a commit
to IgorMinar/angular.js
that referenced
this pull request
Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. V8 leaks are due to strong references from optimized code (fixed in M34) and inline caches (fix in works). Inline caches are caches that the virtual machine builds on the fly to speed up property access for javascript objects. These caches contain strong references to objects so under certain conditions this can create a leak. The reason why these leaks are extra bad for Scope instances is that scopes hold on to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak. This change removes references to objects that might be holding other big objects. This means that even if the destroyed scope leaks, the child scopes should not leak because we are not explicitly holding onto them. Additionally in theory we should also help make the current scope eligible for GC by changing properties of the current Scope object. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. This is a second attempt to enhance the cleanup, the first one failed and was reverted because it was too aggressive and caused problems for existing apps. See: angular#6897 [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes angular#6794 Closes angular#6856
IgorMinar
added a commit
that referenced
this pull request
Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. V8 leaks are due to strong references from optimized code (fixed in M34) and inline caches (fix in works). Inline caches are caches that the virtual machine builds on the fly to speed up property access for javascript objects. These caches contain strong references to objects so under certain conditions this can create a leak. The reason why these leaks are extra bad for Scope instances is that scopes hold on to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak. This change removes references to objects that might be holding other big objects. This means that even if the destroyed scope leaks, the child scopes should not leak because we are not explicitly holding onto them. Additionally in theory we should also help make the current scope eligible for GC by changing properties of the current Scope object. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. This is a second attempt to enhance the cleanup, the first one failed and was reverted because it was too aggressive and caused problems for existing apps. See: #6897 [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes #6794 Closes #6856 Closes #6968
IgorMinar
added a commit
that referenced
this pull request
Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier for GC to collect this scope object. V8 leaks are due to strong references from optimized code (fixed in M34) and inline caches (fix in works). Inline caches are caches that the virtual machine builds on the fly to speed up property access for javascript objects. These caches contain strong references to objects so under certain conditions this can create a leak. The reason why these leaks are extra bad for Scope instances is that scopes hold on to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak. This change removes references to objects that might be holding other big objects. This means that even if the destroyed scope leaks, the child scopes should not leak because we are not explicitly holding onto them. Additionally in theory we should also help make the current scope eligible for GC by changing properties of the current Scope object. I was able to manually verify that this fixes the problem for the following example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se Given the nature of the problem I'm not 100% sure that this will work around the V8 problem in scenarios common for Angular apps, but I guess it's better than nothing. This is a second attempt to enhance the cleanup, the first one failed and was reverted because it was too aggressive and caused problems for existing apps. See: #6897 [1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073 Closes #6794 Closes #6856 Closes #6968
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.
The theory is that the V8 leaks are due to inline caches which are caches
built on the fly to speed up property access for javascript objects.
By cleaning the scope object and removing all properties, we clean up ICs
as well and so no leaks occur.
I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se?p=preview
Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.
Closes #6794