Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1650873002:
Fix memory leak in incremental resolver (Closed)
|
Created:
4 years, 10 months ago by skybrian Modified:
4 years, 10 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix memory leak in incremental resolver
Moving an element that's in the AnalysisCache causes
it to leak because operator== and hashCode are based
on its location.
To make sure we detect this, added a "frozen" flag to
Element. It's set when the element is used as a key
in the AnalysisCache.
Cleanup: in IncrementalResolver, make all fields
final that are never changed.
BUG=
R=brianwilkerson@google.com
Committed: https://github.com/dart-lang/sdk/commit/54ee3eefd1a0e3ce12ba6d5fd03a4213dabccdb2
Patch Set 1 #
Total comments: 18
Patch Set 2 : add unit test #Patch Set 3 : revert frozen flag #Patch Set 4 : simplify and improve tests #
Total comments: 5
Messages
Total messages: 16 (2 generated)
skybrian@google.com changed reviewers: + brianwilkerson@google.com, scheglov@google.com
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1713: bool frozen = false; This would be better implemented as a Modifier because then it wouldn't increase the memory usage. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:2031: //print("$this at $location is frozen and can't be mutated"); Remove debugging code. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:1040: _typeSystem = definingUnit.context.typeSystem, No need to duplicate "definingUnit.context" in either this line or the line above. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:2068: cache.remove(element); This causes us to loose all analysis information associated with the element, which is unfortunate. Konstantin and I discussed an alternate implementation in which AnalysisPartition would implement a method like: modifyUsing(element, function) where the semantics of the method would be to remove the entry for the element, invoke the function, then restore the entry for the element to the partition. That would allow us to preserve analysis information when making an incremental change. (It would also negate the need for passing the AnalysisContext along because every CacheEntry knows the partition that owns it. It would also remove the need for marking elements as "frozen", though I suppose that might be a useful additional check.) What do you think?
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1847: _checkNotFrozen(); The location of an Element consists of identifiers (names) of all enclosing elements. So, in order to guarantee that the location does not change we should freeze not only this, but also all the enclosing elements.
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1847: _checkNotFrozen(); > So, in order to guarantee that the location does not change we should freeze not > only this, but also all the enclosing elements. Good point! If we went with the method on CachePartition we would need to remove and restore all children of the element being modified. If we stick with the approach of freezing elements, then we'll probably have to go with a reference counting scheme so that if multiple children of an element are both placed in the cache we won't unfreeze the parent until all of the children are removed. (Which makes me even more concerned about freezing elements.)
On 2016/02/01 16:11:58, Brian Wilkerson wrote: > https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... > File pkg/analyzer/lib/src/dart/element/element.dart (right): > > https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... > pkg/analyzer/lib/src/dart/element/element.dart:1847: _checkNotFrozen(); > > So, in order to guarantee that the location does not change we should freeze > not > > only this, but also all the enclosing elements. > > Good point! > > If we went with the method on CachePartition we would need to remove and restore > all children of the element being modified. > > If we stick with the approach of freezing elements, then we'll probably have to > go with a reference counting scheme so that if multiple children of an element > are both placed in the cache we won't unfreeze the parent until all of the > children are removed. (Which makes me even more concerned about freezing > elements.) The way the frozen flag works is consistent with how hashCode and location caching also work in ElementImpl. Everywhere we clear _cachedHashCode and _cachedLocation, we should probably visit all descendants of the ElementImpl and clear their caches as well. However, since we're not doing that yet, changing the location of the parent doesn't affect operator== and hashCode for the children, so there isn't an AnalysisCache leak for the children. Instead the hashCode and location of each child is out of date. I wonder what the consequences are? If we stick with the current design and continue to cache the hashCode and location in each ElementImpl, it seems like we could also continue to use the frozen flag. Whenever the parent changes, we need to crawl all the descendents anyway and it's easy to check for the frozen flag at the same time.
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:2068: cache.remove(element); On 2016/02/01 14:58:16, Brian Wilkerson wrote: > This causes us to loose all analysis information associated with the element, > which is unfortunate. > > Konstantin and I discussed an alternate implementation in which > AnalysisPartition would implement a method like: modifyUsing(element, function) > where the semantics of the method would be to remove the entry for the element, > invoke the function, then restore the entry for the element to the partition. > That would allow us to preserve analysis information when making an incremental > change. (It would also negate the need for passing the AnalysisContext along > because every CacheEntry knows the partition that owns it. It would also remove > the need for marking elements as "frozen", though I suppose that might be a > useful additional check.) What do you think? It's unfortunate, but I'm not sure it's a performance regression? By changing the offset, we change operator== and hashCode causing a memory leak. The next time we call get() on the AnalysisCache with this AnalysisTarget, it seems like it will return null, causing the map value to be rebuilt with a "different" key. It would be good to fix this, but since it already happens today, I think we should fix the memory leak first and then look into improving performance in a separate patch?
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:2031: //print("$this at $location is frozen and can't be mutated"); On 2016/02/01 14:58:16, Brian Wilkerson wrote: > Remove debugging code. I've seen other commented out debugging code checked in. Should I remove it when I see it? https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:1040: _typeSystem = definingUnit.context.typeSystem, On 2016/02/01 14:58:16, Brian Wilkerson wrote: > No need to duplicate "definingUnit.context" in either this line or the line > above. If I change it to _context I get this error: Only static members can be accessed in initializers I believe what this actually means is that field names aren't in scope within initializers. So, we could write it as definingUnit.context, or to pass in context as a separate parameter, or write a factory constructor.
Added a test reproducing the cache entry leak. I could take out the frozen flag if you'd rather not do that now. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1713: bool frozen = false; On 2016/02/01 14:58:16, Brian Wilkerson wrote: > This would be better implemented as a Modifier because then it wouldn't increase > the memory usage. Hmm. That apparently makes it part of the public API? https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:2031: //print("$this at $location is frozen and can't be mutated"); On 2016/02/02 01:16:54, skybrian wrote: > On 2016/02/01 14:58:16, Brian Wilkerson wrote: > > Remove debugging code. > > I've seen other commented out debugging code checked in. > Should I remove it when I see it? Done.
https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... File pkg/analyzer/lib/src/dart/element/element.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1713: bool frozen = false; > That apparently makes it part of the public API? I'm not sure why. The class Modifier isn't part of the public API. Anyway, we need to resolve the bigger issues before worrying about this one. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:1847: _checkNotFrozen(); > The way the frozen flag works is consistent with how hashCode and location > caching also work in ElementImpl. > > Everywhere we clear _cachedHashCode and _cachedLocation, we should > probably visit all descendants of the ElementImpl and clear their caches as > well. However, since we're not doing that yet, changing the location of the > parent doesn't affect operator== and hashCode for the children, so there isn't > an AnalysisCache leak for the children. Instead the hashCode and location of > each child is out of date. I wonder what the consequences are? It seems to me that the consequences are that we're postponing the problem until something forces the cached data to be flushed. It appears that the only time we're flushing the cached hash code is when something changes that would have demonstrated the problem anyway, so it might not be an issue. > If we stick with the current design and continue to cache the hashCode and > location in each ElementImpl, it seems like we could also continue to use the > frozen flag. Whenever the parent changes, we need to crawl all the descendents > anyway and it's easy to check for the frozen flag at the same time. I'm not totally convinced that it's easy :-). Also, I'm starting to get confused by all of the possibilities, so I'm not sure what changes we're actually looking at making at this point. Could you update the CL to reflect your current understanding of what we should do to the code? Then we can move forward from there. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/dart/e... pkg/analyzer/lib/src/dart/element/element.dart:2031: //print("$this at $location is frozen and can't be mutated"); > I've seen other commented out debugging code checked in. > Should I remove it when I see it? It's a judgement call. Commented out code tends to get stale quickly, so if it gets left in it should be really useful code (and even then we should consider whether it can be re-written so that it can be left uncommented. In this case, we're already throwing an exception with the same message, so the print statement would be easy to re-create. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:1040: _typeSystem = definingUnit.context.typeSystem, > I believe what this actually means is that field names aren't in scope > within initializers. Ah, right. I'd forgotten that. > So, we could write it as definingUnit.context, or to pass in context as > a separate parameter, or write a factory constructor. Or put it in the body of the method (which would mean it couldn't be final), or leave it as is. I'll vote for the latter. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:2068: cache.remove(element); > It's unfortunate, but I'm not sure it's a performance regression? As long as we only remove elements when their hashCode is about to change, no, it shouldn't be a regression. > It would be good to fix this, but since it already happens today, I think we > should fix the memory leak first and then look into improving performance in a > separate patch? That would be fine. It just looked to me like it would be easier in this case to solve both problems at the same time, but it's your call.
Okay, here is a simplified version that I'd like to commit. https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:1040: _typeSystem = definingUnit.context.typeSystem, On 2016/02/02 16:32:06, Brian Wilkerson wrote: > > I believe what this actually means is that field names aren't in scope > > within initializers. > > Ah, right. I'd forgotten that. > > > So, we could write it as definingUnit.context, or to pass in context as > > a separate parameter, or write a factory constructor. > > Or put it in the body of the method (which would mean it couldn't be final), or > leave it as is. I'll vote for the latter. Okay, assuming by "leave it as is" you didn't mean to revert the change?
LGTM https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right): https://codereview.chromium.org/1650873002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/incremental_resolver.dart:1040: _typeSystem = definingUnit.context.typeSystem, > Okay, assuming by "leave it as is" you didn't mean to revert the change? Correct. I meant "leave the code the way I saw it in the CL" :-) https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... File pkg/analyzer/test/generated/incremental_resolver_test.dart (right): https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/incremental_resolver_test.dart:128: if (cache.get(key) == null) { Would the test cache.get(key) != it.value be better? It probably doesn't matter for any of the tests that are using this, but if we add a test that makes a change then reverts it, I think we could end up getting a non-null but still wrong value. Not positive, though. https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/incremental_resolver_test.dart:131: if (seen.contains(key)) { nit: just use "!seen.add(key)" here and remove line 134
https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... File pkg/analyzer/test/generated/incremental_resolver_test.dart (right): https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/incremental_resolver_test.dart:128: if (cache.get(key) == null) { On 2016/02/02 22:51:14, Brian Wilkerson wrote: > Would the test > > cache.get(key) != it.value > > be better? It probably doesn't matter for any of the tests that are using this, > but if we add a test that makes a change then reverts it, I think we could end > up getting a non-null but still wrong value. Not positive, though. `it.value` works mostly as `cache.get(key)`. So, in case of a damaged Element location both will return `null` (almost always). So, we might want to have both.
https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... File pkg/analyzer/test/generated/incremental_resolver_test.dart (right): https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/incremental_resolver_test.dart:128: if (cache.get(key) == null) { On 2016/02/02 23:04:25, scheglov wrote: > On 2016/02/02 22:51:14, Brian Wilkerson wrote: > > Would the test > > > > cache.get(key) != it.value > > > > be better? It probably doesn't matter for any of the tests that are using > this, > > but if we add a test that makes a change then reverts it, I think we could end > > up getting a non-null but still wrong value. Not positive, though. > > `it.value` works mostly as `cache.get(key)`. > So, in case of a damaged Element location both will return `null` (almost > always). > So, we might want to have both. Yes, the old value is (usually) no longer accessible with the mutated key, because the hash has changed and we're looking in a different slot which has no entries that compare equal. I think the only way to retrieve the old value would be to create a key that compares equal with the mutated key, but returns the old hashCode. But checking for nulls and duplicate keys seems like enough. https://codereview.chromium.org/1650873002/diff/60001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/incremental_resolver_test.dart:131: if (seen.contains(key)) { On 2016/02/02 22:51:14, Brian Wilkerson wrote: > nit: just use "!seen.add(key)" here and remove line 134 Done.
Description was changed from ========== Fix memory leak in incremental resolver Moving an element that's in the AnalysisCache causes it to leak because operator== and hashCode are based on its location. To make sure we detect this, added a "frozen" flag to Element. It's set when the element is used as a key in the AnalysisCache. Cleanup: in IncrementalResolver, make all fields final that are never changed. BUG= ========== to ========== Fix memory leak in incremental resolver Moving an element that's in the AnalysisCache causes it to leak because operator== and hashCode are based on its location. To make sure we detect this, added a "frozen" flag to Element. It's set when the element is used as a key in the AnalysisCache. Cleanup: in IncrementalResolver, make all fields final that are never changed. BUG= R=brianwilkerson@google.com Committed: https://github.com/dart-lang/sdk/commit/54ee3eefd1a0e3ce12ba6d5fd03a4213dabccdb2 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 54ee3eefd1a0e3ce12ba6d5fd03a4213dabccdb2 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
