Fix memory leak in incremental resolver

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

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.generated.incremental_resolver;
 
 import 'dart:collection';
 import 'dart:math' as math;
 
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 934 matching lines @@
  */
 class IncrementalResolver {
   /**
    * The element of the compilation unit being resolved.
    */
   final CompilationUnitElementImpl _definingUnit;
 
   /**
    * 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.
    */
   final int _updateOffset;
 
   /**
    * The end of the changed contents in the old unit.
    */
   final int _updateEndOld;
 
   /**
    * The end of the changed contents in the new unit.
    */
   final int _updateEndNew;
 
   /**
    * 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;
   List<AnalysisError> _verifyErrors = AnalysisError.NO_ERRORS;
 
   /**
    * Initialize a newly created incremental resolver to resolve a node in the
    * 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.
    *
    * [node] - the root of the AST structure to be resolved.
    *
    * Returns `true` if resolution was successful.
    */
   bool resolve(AstNode node) {
     logger.enter('resolve: $_definingUnit');
@@ skipping 220 matching lines @@
                 _updateEndOld, _updateEndNew, _updateDelta));
       } finally {
         timer.stop('invalidate cache with delta');
       }
     }
   }
 
   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');
     }
   }
 
   void _updateEntry() {
     _updateErrors_NEW(RESOLVE_TYPE_NAMES_ERRORS, []);
     _updateErrors_NEW(RESOLVE_UNIT_ERRORS, _resolveErrors);
     _updateErrors_NEW(VARIABLE_REFERENCE_ERRORS, []);
@@ skipping 57 matching lines @@
       _verifyErrors = errorListener.getErrorsForSource(_source);
     } finally {
       timer.stop('verify');
     }
   }
 }
 
 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;
 
   int _updateOffset;
   int _updateDelta;
   int _updateEndOld;
   int _updateEndNew;
 
   LineInfo _newLineInfo;
   List<AnalysisError> _newScanErrors = <AnalysisError>[];
   List<AnalysisError> _newParseErrors = <AnalysisError>[];
 
   PoorMansIncrementalResolver(
       this._typeProvider,
       this._unitSource,
+      this._cache,
       this._sourceEntry,
       this._unitEntry,
       this._oldUnit,
       bool resolveApiChanges) {
     _resolveApiChanges = resolveApiChanges;
   }
 
   /**
    * Attempts to update [_oldUnit] to the state corresponding to [newCode].
    * Returns `true` if success, or `false` otherwise.
@@ skipping 40 matching lines @@
             bool success = _resolveCommentDoc(newUnit, firstPair);
             logger.log('Documentation comment resolved: $success');
             return success;
           }
           // A pure whitespace change.
           if (firstPair.kind == _TokenDifferenceKind.OFFSET) {
             logger.log('Whitespace change.');
             _shiftTokens(firstPair.oldToken);
             {
               IncrementalResolver incrementalResolver = new IncrementalResolver(
+                  _cache,
                   _sourceEntry,
                   _unitEntry,
                   _unitElement,
                   _updateOffset,
                   _updateEndOld,
                   _updateEndNew);
               incrementalResolver._updateCache();
               incrementalResolver._updateElementNameOffsets();
               incrementalResolver._shiftEntryErrors();
             }
@@ skipping 106 matching lines @@
           if (oldBeginToken.previous.type == TokenType.EOF) {
             _oldUnit.beginToken = newBeginToken;
           } else {
             oldBeginToken.previous.setNext(newBeginToken);
           }
           newNode.endToken.setNext(oldNode.endToken.next);
           _shiftTokens(oldNode.endToken.next);
         }
         // perform incremental resolution
         IncrementalResolver incrementalResolver = new IncrementalResolver(
+            _cache,
             _sourceEntry,
             _unitEntry,
             _unitElement,
             _updateOffset,
             _updateEndOld,
             _updateEndNew);
         bool success = incrementalResolver.resolve(newNode);
         // check if success
         if (!success) {
           logger.log('Failure: element model changed.');
@@ skipping 58 matching lines @@
     logger.log('oldComment.beginToken: ${oldComment.beginToken}');
     logger.log('newComment.beginToken: ${newComment.beginToken}');
     _updateOffset = oldToken.offset - 1;
     // update token references
     _shiftTokens(firstPair.oldToken);
     _setPrecedingComments(oldToken, newComment.tokens.first);
     // replace node
     NodeReplacer.replace(oldComment, newComment);
     // update elements
     IncrementalResolver incrementalResolver = new IncrementalResolver(
+        _cache,
         _sourceEntry,
         _unitEntry,
         _unitElement,
         _updateOffset,
         _updateEndOld,
         _updateEndNew);
     incrementalResolver._updateCache();
     incrementalResolver._updateElementNameOffsets();
     incrementalResolver._shiftEntryErrors();
     _updateEntry();
@@ skipping 368 matching lines @@
   }
 }
 
 /**
  * Instances of the class [_DeclarationMismatchException] represent an exception
  * that is thrown when the element model defined by a given AST structure does
  * not match an existing element model.
  */
 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
     if (element is LocalElement) {
       SourceRange visibleRange = element.visibleRange;
       if (visibleRange != null && visibleRange.offset > updateOffset) {
         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);
         }
       }
     }
     super.visitElement(element);
   }
 }
 
 class _ElementsGatherer extends GeneralizingElementVisitor {
   final DeclarationMatcher matcher;
@@ skipping 65 matching lines @@
   @override
   String toString() => name;
 }
 
 class _TokenPair {
   final _TokenDifferenceKind kind;
   final Token oldToken;
   final Token newToken;
   _TokenPair(this.kind, this.oldToken, this.newToken);
 }