Improve the performance of the bulk fix processor

This improvement was found by Jens.

The bulk fix processor needs to be able to revert a partial set of edits
that can't be completed because of a conflicting pre-existing edit. In
order to do that efficiently it needs to know whether there are any
edits to be reverted, which it used to do by computing a hash from the
edits both before and after running a correction producer. Computing
the hashes is expensive.

This CL changes the detection to use a modification counter that's
incremented any time new edits are added, removing the computation of
the hash values.

I don't yet have a benchmark for the case that uncovered the issue,
but I do have code that generates a file containing 10,000 violations
of the prefer_single_quotes lint, requests bulk fix results 5 times
to warm up the VM, then requests fixes 5 more times and prints an
average (the times between runs are fairly close, so the standard
deviation is small). The code uses the legacy protocol to request the
fixes, but it doesn't execute any of the code in dartdev.)

    Before this change the average was 20670.6 ms.

    After this change the average ise 13979.0 ms.

This represents a 32% improvement.

Change-Id: I74c079bef8e095acf4ad36de6d0c0241934f79a5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425221
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Jens Johansen <[email protected]>

Author: bwilkerson

pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart

@@ -862,12 +862,11 @@ class BulkFixProcessor {
     CorrectionProducer producer,
     String codeName,
   ) async {
-    int computeChangeHash() => (builder as ChangeBuilderImpl).changeHash;
-
-    var oldHash = computeChangeHash();
+    var oldCount = (builder as ChangeBuilderImpl).modificationCount;
+    // Apply the producer, which might re-assign the `builder`.
     await _applyProducer(producer);
-    var newHash = computeChangeHash();
-    if (newHash != oldHash) {
+    var newCount = (builder as ChangeBuilderImpl).modificationCount;
+    if (newCount != oldCount) {
       changeMap.add(context.path, codeName.toLowerCase());
     }
   }

pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_core.dart

@@ -59,6 +59,18 @@ class ChangeBuilderImpl implements ChangeBuilder {
   /// A map of absolute normalized path to YAML file edit builders.
   final Map<String, YamlFileEditBuilderImpl> _yamlFileEditBuilders = {};
 
+  /// The number of times that any of the file edit builders in this change have
+  /// been modified.
+  ///
+  /// A file builder is considered to be modified when the list of edits is
+  /// modified in any way, such as by adding a new edit to the list. For Dart
+  /// file edit builders this includes changes to the list of libraries that
+  /// will be imported by creating edits at a later point.
+  ///
+  /// This can be used, for example, to determine whether any edits were added
+  /// by a given correction producer.
+  int modificationCount = 0;
+
   /// Initialize a newly created change builder. If the builder will be used to
   /// create changes for Dart files, then either a [session] or a [workspace]
   /// must be provided (but not both).
@@ -67,26 +79,6 @@ class ChangeBuilderImpl implements ChangeBuilder {
       : assert(session == null || workspace == null),
         workspace = workspace ?? _SingleSessionWorkspace(session!);
 
-  /// Return a hash value that will change when new edits have been added to
-  /// this builder.
-  int get changeHash {
-    // The hash value currently ignores edits to import directives because
-    // finalizing the builders needs to happen exactly once and this getter
-    // needs to be invoked repeatedly.
-    //
-    // In addition, we should consider implementing our own hash function for
-    // file edits because the `hashCode` defined for them might not be
-    // sufficient to detect all changes to the list of edits.
-    return Object.hashAll([
-      for (var builder in _genericFileEditBuilders.values)
-        if (builder.hasEdits) builder.fileEdit,
-      for (var builder in _dartFileEditBuilders.values)
-        if (builder.hasEdits) builder.fileEdit,
-      for (var builder in _yamlFileEditBuilders.values)
-        if (builder.hasEdits) builder.fileEdit,
-    ]);
-  }
-
   /// Return `true` if this builder has edits to be applied.
   bool get hasEdits {
     return _dartFileEditBuilders.isNotEmpty ||
@@ -270,6 +262,7 @@ class ChangeBuilderImpl implements ChangeBuilder {
     for (var entry in _yamlFileEditBuilders.entries) {
       copy._yamlFileEditBuilders[entry.key] = entry.value.copyWith(copy);
     }
+    copy.modificationCount = modificationCount;
     return copy;
   }
 
@@ -625,6 +618,7 @@ class FileEditBuilderImpl implements FileEditBuilder {
     var delta = _editDelta(edit);
     changeBuilder._updatePositions(edit.offset, delta);
     changeBuilder._lockedPositions.clear();
+    changeBuilder.modificationCount++;
   }
 
   /// Add the edit from the given [builder] to the edits associated with the

pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart

@@ -1228,8 +1228,8 @@ class DartEditBuilderImpl extends EditBuilderImpl implements DartEditBuilder {
     }
   }
 
-  void _writeSuperMemberInvocation(ExecutableElement element,
-      String memberName, List<FormalParameterElement> parameters) {
+  void _writeSuperMemberInvocation(ExecutableElement element, String memberName,
+      List<FormalParameterElement> parameters) {
     var isOperator = element is MethodElement && element.isOperator;
     write(isOperator ? ' ' : '.');
     write(memberName);
@@ -1728,9 +1728,7 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
 
     // Queued change.
     var importChange = (libraryChangeBuilder ?? this).librariesToImport[uri];
-    if (importChange != null) return true;
-
-    return false;
+    return importChange != null;
   }
 
   @override
@@ -2814,7 +2812,7 @@ class _LibraryImport implements Comparable<_LibraryImport> {
         '${allHiddenNames.isNotEmpty ? 'hide ${allHiddenNames.join(', ')}' : ''};';
   }
 
-  /// Ensures [name] is visible for this import.
+  /// Ensures that [name] is visible for this import.
   ///
   /// If the import already has a show combinator, this name will be added.
   /// If the import hides this name, it will be unhidden.