Fix memory leak in incremental resolver

pkg/analyzer/lib/src/generated/incremental_resolver.dart

Index: pkg/analyzer/lib/src/generated/incremental_resolver.dart
diff --git a/pkg/analyzer/lib/src/generated/incremental_resolver.dart b/pkg/analyzer/lib/src/generated/incremental_resolver.dart
index 542e72193eba091ea0b32bb04b699ae2c06e3d7c..f869228ed281874ee95078848302594a3103e6ec 100644
--- a/pkg/analyzer/lib/src/generated/incremental_resolver.dart
+++ b/pkg/analyzer/lib/src/generated/incremental_resolver.dart
@@ -952,42 +952,44 @@ class IncrementalResolver {
   /**
    * The context the compilation unit being resolved in.
    */
-  AnalysisContext _context;
+  final AnalysisContext _context;
 
   /**
    * The object used to access the types from the core library.
    */
-  TypeProvider _typeProvider;
+  final TypeProvider _typeProvider;
 
   /**
    * The type system primitives.
    */
-  TypeSystem _typeSystem;
+  final TypeSystem _typeSystem;
 
   /**
    * The element for the library containing the compilation unit being resolved.
    */
-  LibraryElementImpl _definingLibrary;
+  final LibraryElementImpl _definingLibrary;
+
+  final AnalysisCache _cache;
 
   /**
    * The [CacheEntry] corresponding to the source being resolved.
    */
-  CacheEntry newSourceEntry;
+  final CacheEntry newSourceEntry;
 
   /**
    * The [CacheEntry] corresponding to the [LibrarySpecificUnit] being resolved.
    */
-  CacheEntry newUnitEntry;
+  final CacheEntry newUnitEntry;
 
   /**
    * The source representing the compilation unit being visited.
    */
-  Source _source;
+  final Source _source;
 
   /**
    * The source representing the library of the compilation unit being visited.
    */
-  Source _librarySource;
+  final Source _librarySource;
 
   /**
    * The offset of the changed contents.
@@ -1007,14 +1009,14 @@ class IncrementalResolver {
   /**
    * The delta between [_updateEndNew] and [_updateEndOld].
    */
-  int _updateDelta;
+  final int _updateDelta;
 
   /**
    * The set of [AnalysisError]s that have been already shifted.
    */
-  Set<AnalysisError> _alreadyShiftedErrors = new HashSet.identity();
+  final Set<AnalysisError> _alreadyShiftedErrors = new HashSet.identity();
 
-  RecordingErrorListener errorListener = new RecordingErrorListener();
+  final RecordingErrorListener errorListener = new RecordingErrorListener();
   ResolutionContext _resolutionContext;
 
   List<AnalysisError> _resolveErrors = AnalysisError.NO_ERRORS;
@@ -1025,20 +1027,23 @@ class IncrementalResolver {
    * given source in the given library.
    */
   IncrementalResolver(
+      this._cache,
       this.newSourceEntry,
       this.newUnitEntry,
-      this._definingUnit,
+      CompilationUnitElementImpl definingUnit,
       this._updateOffset,
-      this._updateEndOld,
-      this._updateEndNew) {
-    _updateDelta = _updateEndNew - _updateEndOld;
-    _definingLibrary = _definingUnit.library;
-    _librarySource = _definingLibrary.source;
-    _source = _definingUnit.source;
-    _context = _definingUnit.context;
-    _typeProvider = _context.typeProvider;
-    _typeSystem = _context.typeSystem;
-  }
+      int updateEndOld,
+      int updateEndNew)
+      : _definingUnit = definingUnit,
+        _context = definingUnit.context,
+        _typeProvider = definingUnit.context.typeProvider,
+        _typeSystem = definingUnit.context.typeSystem,

[Brian Wilkerson, 2016/02/01 14:58:16]

No need to duplicate "definingUnit.context" in either this line or the line
above.

[skybrian, 2016/02/02 01:16:54]

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.

[Brian Wilkerson, 2016/02/02 16:32:06]

Ah, right. I'd forgotten that.
 
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.

[skybrian, 2016/02/02 22:42:11]

Okay, assuming by "leave it as is" you didn't mean to revert the change?

[Brian Wilkerson, 2016/02/02 22:51:14]

Correct. I meant "leave the code the way I saw it in the CL"  :-)

+        _definingLibrary = definingUnit.library,
+        _source = definingUnit.source,
+        _librarySource = definingUnit.library.source,
+        _updateEndOld = updateEndOld,
+        _updateEndNew = updateEndNew,
+        _updateDelta = updateEndNew - updateEndOld;
 
   /**
    * Resolve [node], reporting any errors or warnings to the given listener.
@@ -1279,8 +1284,8 @@ class IncrementalResolver {
   void _updateElementNameOffsets() {
     LoggingTimer timer = logger.startTimer();
     try {
-      _definingUnit
-          .accept(new _ElementOffsetUpdater(_updateOffset, _updateDelta));
+      _definingUnit.accept(
+          new _ElementOffsetUpdater(_updateOffset, _updateDelta, _cache));
       _definingUnit.afterIncrementalResolution();
     } finally {
       timer.stop('update element offsets');
@@ -1358,16 +1363,17 @@ class IncrementalResolver {
 class PoorMansIncrementalResolver {
   final TypeProvider _typeProvider;
   final Source _unitSource;
+  final AnalysisCache _cache;
 
   /**
    * The [CacheEntry] corresponding to the source being resolved.
    */
-  CacheEntry _sourceEntry;
+  final CacheEntry _sourceEntry;
 
   /**
    * The [CacheEntry] corresponding to the [LibrarySpecificUnit] being resolved.
    */
-  CacheEntry _unitEntry;
+  final CacheEntry _unitEntry;
 
   final CompilationUnit _oldUnit;
   CompilationUnitElement _unitElement;
@@ -1384,6 +1390,7 @@ class PoorMansIncrementalResolver {
   PoorMansIncrementalResolver(
       this._typeProvider,
       this._unitSource,
+      this._cache,
       this._sourceEntry,
       this._unitEntry,
       this._oldUnit,
@@ -1444,6 +1451,7 @@ class PoorMansIncrementalResolver {
             _shiftTokens(firstPair.oldToken);
             {
               IncrementalResolver incrementalResolver = new IncrementalResolver(
+                  _cache,
                   _sourceEntry,
                   _unitEntry,
                   _unitElement,
@@ -1570,6 +1578,7 @@ class PoorMansIncrementalResolver {
         }
         // perform incremental resolution
         IncrementalResolver incrementalResolver = new IncrementalResolver(
+            _cache,
             _sourceEntry,
             _unitEntry,
             _unitElement,
@@ -1648,6 +1657,7 @@ class PoorMansIncrementalResolver {
     NodeReplacer.replace(oldComment, newComment);
     // update elements
     IncrementalResolver incrementalResolver = new IncrementalResolver(
+        _cache,
         _sourceEntry,
         _unitEntry,
         _unitElement,
@@ -2036,17 +2046,26 @@ class ResolutionContextBuilder {
  */
 class _DeclarationMismatchException {}
 
+/**
+ * Adjusts the location of each Element that moved.
+ *
+ * Since operator== and hashCode of an Element are based
+ * on the element location, we also need to remove each
+ * moved element from the cache to avoid a memory leak.
+ */
 class _ElementOffsetUpdater extends GeneralizingElementVisitor {
   final int updateOffset;
   final int updateDelta;
+  final AnalysisCache cache;
 
-  _ElementOffsetUpdater(this.updateOffset, this.updateDelta);
+  _ElementOffsetUpdater(this.updateOffset, this.updateDelta, this.cache);
 
   @override
   visitElement(Element element) {
     // name offset
     int nameOffset = element.nameOffset;
     if (nameOffset > updateOffset) {
+      cache.remove(element);

[Brian Wilkerson, 2016/02/01 14:58:16]

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?

[skybrian, 2016/02/01 22:26:32]

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?

[Brian Wilkerson, 2016/02/02 16:32:06]

As long as we only remove elements when their hashCode is about to change, no,
it shouldn't be a regression.
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.

       (element as ElementImpl).nameOffset = nameOffset + updateDelta;
     }
     // visible range
@@ -2056,10 +2075,13 @@ class _ElementOffsetUpdater extends GeneralizingElementVisitor {
         int newOffset = visibleRange.offset + updateDelta;
         int length = visibleRange.length;
         if (element is FunctionElementImpl) {
+          cache.remove(element);
           element.setVisibleRange(newOffset, length);
         } else if (element is LocalVariableElementImpl) {
+          cache.remove(element);
           element.setVisibleRange(newOffset, length);
         } else if (element is ParameterElementImpl) {
+          cache.remove(element);
           element.setVisibleRange(newOffset, length);
         }
       }