[analysis server/analyzer] Improvements to completion

Observation 1: The legacy protocol answered completion requests via a
`server.resolveForCompletion` call, and the LSP protocol answered
completion requests via a `server.getResolvedUnit` call where the idea
is that resolving for completion requires less and is therefore faster.

Solution: Make the LSP protocol use `server.resolveForCompletion` too.

Observation 2: Completion requests often come right after change request
making timing important and the `await driver.applyPendingFileChanges()`
call done in the analysis server "pushed" the timing making the
`server.resolveForCompletion` actually finished after it had already
resolved the whole thing.

Solution: Don't do that - the driver adds it to the queue of work, the
work is done later in `performWork` called from
`AnalysisDriverScheduler._run` where `_applyPendingFileChanges` is
always called anyway (which is the call that completes the
`applyPendingFileChanges`call).

Observation 3: If there is no change yet to be processed, a
`resolveForCompletion` call is slower than a `getResolvedUnit` because
the resolved unit is cached (assuming it's a priority file) and the
`resolveForCompletion` call always parses the file again.

Solution: Respond to the `resolveForCompletion` call with the resolved
unit data if it's available in the cache. The cache is always cleared
when changes happen anyway.

Benchmarks on this stuff is a bit weird because it's timing related - so
while I'd say this is overall just better there are also runs where we
get "bad timing" and the runtimes are therefore not better. In an
attempt to clear it up I've run the benchmarks 25 times each, and
attempted to put the data in two different buckets as needed.

*lsp_type_in_big_file of size 16,000*

```
Fully done after last type (ms):
Difference at 95.0% confidence
        -1419.2 +/- 322.074
        -16.8866% +/- 3.83225%
        (Student's t, pooled s = 566.238)

Whole typing time (ms):
Difference at 95.0% confidence
        -1418.88 +/- 322.037
        -13.9661% +/- 3.16983%
        (Student's t, pooled s = 566.172)

Uncancelled completion response time (ms):
Difference at 95.0% confidence
        -1590.48 +/- 682.753
        -28.0194% +/- 12.028%
        (Student's t, pooled s = 1200.35)
```

The `Uncancelled completion response time` has a big "+/-" so attempting
to "good and bad bucketize" it I get:

good bucket:

```
Difference at 95.0% confidence
        -1929.8 +/- 347.712
        -35.9528% +/- 6.47797%
        (Student's t, pooled s = 543.261)
```

bad bucket (though truthfully there wasn't a clear cutoff before):

```
No difference proven at 95.0% confidence
```

which sort of makes sense: If the completion runs before a (new) change
starts processing we now `resolveForCompletion` instead which is faster,
but if completion runs after the change has started processing we
essentially - both before and after - do nothing (except wait for the
calculation to finish) because we just load the data from cache.

*lsp_type_in_big_file_ask_for_completion, 16,000*

```
Completion #1 (ms):
No difference proven at 95.0% confidence
```

Ehh. There's a clear cutoff in the now, so taking the 8 (how the cutoff
happens to be) fastest from each I get

```
Difference at 95.0% confidence
        -1034.12 +/- 75.3617
        -37.673% +/- 2.74542%
        (Student's t, pooled s = 70.2673)
```

Moving on.

```
Completion #2 (ms):
Difference at 95.0% confidence
        -1076.28 +/- 178.446
        -37.2601% +/- 6.17769%
        (Student's t, pooled s = 313.726)
```

here 2 in the "now" has bad timing, removing them from the statistics
gives

```
Difference at 95.0% confidence
        -1182.39 +/- 104.543
        -40.9334% +/- 3.6192%
        (Student's t, pooled s = 179.748)
```

Moving on.

```
Completion #3 (ms):
Difference at 95.0% confidence
        -735.6 +/- 292.553
        -25.6471% +/- 10.2%
        (Student's t, pooled s = 514.336)

and removing the 8 bad ones:

Difference at 95.0% confidence
        -1195.69 +/- 99.305
        -41.6884% +/- 3.46232%
        (Student's t, pooled s = 156.306)
```

Continuing like this:

```
Completion #4 (ms):
Difference at 95.0% confidence
        -948.4 +/- 265.704
        -32.9887% +/- 9.24214%
        (Student's t, pooled s = 467.134)

and removing the 5 bad ones:

Difference at 95.0% confidence
        -1252.87 +/- 86.5425
        -43.5793% +/- 3.01026%
        (Student's t, pooled s = 143.022)

Completion #5 (ms):

Difference at 95.0% confidence
        -1067.88 +/- 199.266
        -37.0011% +/- 6.90439%
        (Student's t, pooled s = 350.329)

and removing the 3 bad ones:

Difference at 95.0% confidence
        -1236.76 +/- 69.5776
        -42.8527% +/- 2.4108%
        (Student's t, pooled s = 118.18)
```

Moving on to the "Completion without change" I realize just now that the
benchmark for the first entry is broken - it doesn't wait until the
previous change has been processed, meaning that in the 3 cases where we
got bad timing in "Completion #5 (ms)" we see about the same result as
before, but in the 22 other cases we see bad results because it has to
wait until the previous change has been processed. For the remaining
(2-5) there is no virtually change which makes sense because both before
and now it just fetches the resolved unit from cache.

*legacy_type_in_big_file_ask_for_completion*

```
Completion #1 (ms):
Difference at 95.0% confidence
        -1817.08 +/- 60.6836
        -48.0638% +/- 1.60515%
        (Student's t, pooled s = 106.688)

Completion #2 (ms):
Difference at 95.0% confidence
        -2208.56 +/- 48.4844
        -55.4647% +/- 1.21761%
        (Student's t, pooled s = 85.2403)

Completion #3 (ms):
Difference at 95.0% confidence
        -2159.68 +/- 69.4145
        -53.0717% +/- 1.70578%
        (Student's t, pooled s = 122.037)

Completion #4 (ms):
Difference at 95.0% confidence
        -2264.44 +/- 73.2112
        -53.5455% +/- 1.73117%
        (Student's t, pooled s = 128.712)

Completion #5 (ms):
Difference at 95.0% confidence
        -2147.4 +/- 68.5023
        -50.9254% +/- 1.62453%
        (Student's t, pooled s = 120.434)
```

The first "Completion without change" suffers from the same as before
and I will skip it here.

```
Completion without change #2 (ms):
Difference at 95.0% confidence
        -416.28 +/- 28.2024
        -61.3512% +/- 4.15646%
        (Student's t, pooled s = 49.5826)

Completion without change #3 (ms):
Difference at 95.0% confidence
        -687.24 +/- 21.9848
        -92.6499% +/- 2.96387%
        (Student's t, pooled s = 38.6514)

Completion without change #4 (ms):
Difference at 95.0% confidence
        -637.32 +/- 26.482
        -95.2703% +/- 3.95868%
        (Student's t, pooled s = 46.5579)

Completion without change #5 (ms):
Difference at 95.0% confidence
        -702.32 +/- 18.7277
        -95.8301% +/- 2.55535%
        (Student's t, pooled s = 32.925)
```

I don't know why there doesn't appear to be any timing related issues
here (maybe sending and receiving the entire big file (in legacy vs in
lsp where a small 'diff' is send) taking more time pushes the timing,
but I'm guessing) - nor do I know why now "Completion without change #2"
is slower (~250 ms) than the subsequent ones (~30 ms).

Change-Id: I4c21d658efccbcf197eedb69f466b2942b78c4b9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/457364
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/analysis_server/lib/src/analysis_server.dart

@@ -1031,8 +1031,10 @@ abstract class AnalysisServer {
       return null;
     }
 
+    var result = _tryGetCachedResolvedUnitAsResolvedForCompletion(driver, path);
+    if (result != null) return result;
+
     try {
-      await driver.applyPendingFileChanges();
       return await driver.resolveForCompletion(
         path: path,
         offset: offset,
@@ -1044,6 +1046,37 @@ abstract class AnalysisServer {
     return null;
   }
 
+  Future<ResolvedForCompletionResultImpl?> resolveForCompletionWithLineColumn({
+    required String path,
+    required int line,
+    required int column,
+    required OperationPerformanceImpl performance,
+  }) async {
+    if (!file_paths.isDart(resourceProvider.pathContext, path)) {
+      return null;
+    }
+
+    var driver = getAnalysisDriver(path);
+    if (driver == null) {
+      return null;
+    }
+
+    var result = _tryGetCachedResolvedUnitAsResolvedForCompletion(driver, path);
+    if (result != null) return result;
+
+    try {
+      return await driver.resolveForCompletionWithLineColumn(
+        path: path,
+        line: line,
+        column: column,
+        performance: performance,
+      );
+    } catch (e, st) {
+      instrumentationService.logException(e, st);
+    }
+    return null;
+  }
+
   /// Sends an LSP notification to the client.
   ///
   /// The legacy server will wrap LSP notifications inside an
@@ -1097,6 +1130,34 @@ abstract class AnalysisServer {
     await contextManager.dispose();
     await analyticsManager.shutdown();
   }
+
+  ResolvedForCompletionResultImpl?
+  _tryGetCachedResolvedUnitAsResolvedForCompletion(
+    AnalysisDriver driver,
+    String path,
+  ) {
+    try {
+      ResolvedUnitResult? result = driver.getCachedResolvedUnit(path);
+      if (result != null) {
+        result as ResolvedUnitResultImpl;
+        return ResolvedForCompletionResultImpl(
+          analysisSession: result.session,
+          fileState: result.fileState,
+          path: path,
+          uri: result.uri,
+          exists: result.exists,
+          content: result.content,
+          lineInfo: result.lineInfo,
+          parsedUnit: result.unit,
+          unitElement: result.libraryFragment,
+          resolvedNodes: [result.unit],
+        );
+      }
+    } catch (e, st) {
+      instrumentationService.logException(e, st);
+    }
+    return null;
+  }
 }
 
 /// ContextManager callbacks that operate on the base server regardless

pkg/analysis_server/lib/src/cider/completion.dart

@@ -73,7 +73,6 @@ class CiderCompletionComputer {
       }
 
       var analysisSession = resolvedUnit.analysisSession;
-      var enclosingNode = resolvedUnit.parsedUnit;
 
       var lineInfo = resolvedUnit.lineInfo;
       var offset = lineInfo.getOffsetOfLine(line) + column;
@@ -84,7 +83,6 @@ class CiderCompletionComputer {
         filePath: resolvedUnit.path,
         fileContent: resolvedUnit.content,
         libraryFragment: resolvedUnit.unitElement,
-        enclosingNode: enclosingNode,
         offset: offset,
         unit: resolvedUnit.parsedUnit,
       );

pkg/analysis_server/lib/src/handler/legacy/completion_get_suggestions2.dart

@@ -166,15 +166,13 @@ class CompletionGetSuggestions2Handler extends CompletionHandler
       server.recentPerformance.completion.add(completionPerformance);
 
       var analysisSession = resolvedUnit.analysisSession;
-      var enclosingNode = resolvedUnit.parsedUnit;
 
       var completionRequest = DartCompletionRequest(
         analysisSession: analysisSession,
         fileState: resolvedUnit.fileState,
         filePath: resolvedUnit.path,
         fileContent: resolvedUnit.content,
         libraryFragment: resolvedUnit.unitElement,
-        enclosingNode: enclosingNode,
         offset: offset,
         unit: resolvedUnit.parsedUnit,
         dartdocDirectiveInfo: server.getDartdocDirectiveInfoForSession(

pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart

@@ -25,10 +25,10 @@ import 'package:analysis_server/src/services/snippets/dart_snippet_request.dart'
 import 'package:analysis_server/src/services/snippets/snippet_manager.dart';
 import 'package:analysis_server/src/utilities/element_location2.dart';
 import 'package:analysis_server/src/utilities/extensions/object.dart';
-import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/analysis/session.dart';
 import 'package:analyzer/dart/ast/ast.dart' as ast;
 import 'package:analyzer/source/line_info.dart';
+import 'package:analyzer/src/dart/analysis/results.dart';
 import 'package:analyzer/src/util/file_paths.dart' as file_paths;
 import 'package:analyzer/src/util/performance/operation_performance.dart';
 import 'package:analyzer/src/utilities/fuzzy_matcher.dart';
@@ -118,10 +118,28 @@ class CompletionHandler
     // To do this, tell the server to lock requests until we have a resolved
     // unit and LineInfo.
     late ErrorOr<LineInfo> lineInfo;
-    late ErrorOr<ResolvedUnitResult> unit;
+    late ErrorOr<ResolvedForCompletionResultImpl> unit;
     await server.pauseSchedulerWhile(() async {
-      unit = await path.mapResult(requireResolvedUnit);
-      lineInfo = await unit.map(
+      unit = await message.performance.runAsync('resolveForCompletion', (
+        performance,
+      ) async {
+        return await path.mapResult<ResolvedForCompletionResultImpl>((
+          path,
+        ) async {
+          var result = await server.resolveForCompletionWithLineColumn(
+            path: path,
+            line: pos.line,
+            column: pos.character,
+            performance: performance,
+          );
+          if (result != null) return success(result);
+          return server.isAnalyzed(path)
+              ? analysisFailedError(path)
+              : fileNotAnalyzedError(path);
+        });
+      });
+
+      lineInfo = unit.map(
         // If we don't have a unit, we can still try to obtain the line info from
         // the server (this could be because the file is non-Dart, such as YAML or
         // another handled by a plugin).
@@ -312,13 +330,17 @@ class CompletionHandler
 
   Future<Iterable<CompletionItem>> _getDartSnippetItems({
     required LspClientCapabilities clientCapabilities,
-    required ResolvedUnitResult unit,
+    required ResolvedForCompletionResultImpl unit,
     required int offset,
     required LineInfo lineInfo,
     required bool Function(String input) filter,
     CompletionItemDefaults? defaults,
   }) async {
-    var request = DartSnippetRequest(unit: unit, offset: offset);
+    var request = DartSnippetRequest.fromCompletionResult(
+      unit: unit,
+      offset: offset,
+      file: unit.fileState.source.file,
+    );
     var snippetManager = DartSnippetManager();
     var snippets = await snippetManager.computeSnippets(
       request,
@@ -340,7 +362,7 @@ class CompletionHandler
 
   Future<ErrorOr<_CompletionResults>> _getServerDartItems(
     LspClientCapabilities capabilities,
-    ResolvedUnitResult unit,
+    ResolvedForCompletionResultImpl unit,
     CompletionPerformance completionPerformance,
     OperationPerformanceImpl performance,
     int offset,
@@ -351,11 +373,19 @@ class CompletionHandler
     var useNotImportedCompletions =
         suggestFromUnimportedLibraries && capabilities.applyEdit;
 
-    var completionRequest = DartCompletionRequest.forResolvedUnit(
-      resolvedUnit: unit,
+    var analysisSession = unit.analysisSession;
+
+    var completionRequest = DartCompletionRequest(
+      analysisSession: analysisSession,
+      fileState: unit.fileState,
+      filePath: unit.path,
+      fileContent: unit.content,
+      libraryFragment: unit.unitElement,
       offset: offset,
-      dartdocDirectiveInfo: server.getDartdocDirectiveInfoFor(unit),
-      completionPreference: CompletionPreference.replace,
+      unit: unit.parsedUnit,
+      dartdocDirectiveInfo: server.getDartdocDirectiveInfoForSession(
+        analysisSession,
+      ),
     );
     var target = completionRequest.target;
     var targetPrefix = completionRequest.targetPrefix;
@@ -566,9 +596,8 @@ class CompletionHandler
           .enableSnippets;
       // We can only produce edits with edit builders for files inside
       // the root, so skip snippets entirely if not.
-      var isEditableFile = unit.session.analysisContext.contextRoot.isAnalyzed(
-        unit.path,
-      );
+      var isEditableFile = analysisSession.analysisContext.contextRoot
+          .isAnalyzed(unit.path);
       List<CompletionItem> unrankedResults;
       if (capabilities.completionSnippets &&
           snippetsEnabled &&

pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart

@@ -305,13 +305,12 @@ class DartCompletionRequest {
     required String filePath,
     required String fileContent,
     required LibraryFragment libraryFragment,
-    required AstNode enclosingNode,
     required int offset,
     required CompilationUnit unit,
     DartdocDirectiveInfo? dartdocDirectiveInfo,
     CompletionPreference completionPreference = CompletionPreference.insert,
   }) {
-    var target = CompletionTarget.forOffset(enclosingNode, offset);
+    var target = CompletionTarget.forOffset(unit, offset);
 
     var libraryElement = libraryFragment.element;
     var featureComputer = FeatureComputer(
@@ -364,7 +363,6 @@ class DartCompletionRequest {
       filePath: resolvedUnit.path,
       fileContent: resolvedUnit.content,
       libraryFragment: resolvedUnit.libraryFragment,
-      enclosingNode: resolvedUnit.unit,
       offset: offset,
       unit: resolvedUnit.unit,
       dartdocDirectiveInfo: dartdocDirectiveInfo,

pkg/analysis_server/lib/src/services/snippets/dart/main_function.dart

@@ -32,8 +32,7 @@ class MainFunction extends DartSnippetProducer {
       session: request.analysisSession,
       defaultEol: utils.endOfLine,
     );
-
-    var typeProvider = request.unit.typeProvider;
+    var typeProvider = request.typeProvider;
     var listString = typeProvider.listType(typeProvider.stringType);
 
     await builder.addDartFileEdit(request.filePath, (builder) {

pkg/analysis_server/lib/src/services/snippets/dart_snippet_request.dart

@@ -7,14 +7,32 @@ import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/analysis/session.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
+import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/dart/element/type_provider.dart';
 import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/source/source_range.dart';
+import 'package:analyzer/src/dart/analysis/results.dart';
 import 'package:analyzer_plugin/src/utilities/completion/completion_target.dart';
 
 /// The information about a request for a list of snippets within a Dart file.
 class DartSnippetRequest {
-  /// The resolved unit for the file that snippets are being requested for.
-  final ResolvedUnitResult unit;
+  /// The analysis session that produced the elements of the request.
+  final AnalysisSession analysisSession;
+
+  /// The type provider used when resolving the compilation [unit].
+  final TypeProvider typeProvider;
+
+  /// The file resource.
+  final File file;
+
+  /// The file resource.
+  final String content;
+
+  /// The parsed, unresolved compilation unit for the [content].
+  final CompilationUnit compilationUnit;
+
+  /// The element representing the library containing the compilation [unit].
+  final LibraryElement libraryElement;
 
   /// The path of the file snippets are being requested for.
   final String filePath;
@@ -30,15 +48,33 @@ class DartSnippetRequest {
   /// replaced if the snippet is selected.
   late final SourceRange replacementRange;
 
-  DartSnippetRequest({required this.unit, required this.offset})
-    : filePath = unit.path {
+  DartSnippetRequest({required ResolvedUnitResult unit, required this.offset})
+    : analysisSession = unit.session,
+      typeProvider = unit.typeProvider,
+      file = unit.file,
+      content = unit.content,
+      compilationUnit = unit.unit,
+      libraryElement = unit.libraryElement,
+      filePath = unit.path {
     var target = CompletionTarget.forOffset(unit.unit, offset);
     context = _getContext(target);
     replacementRange = target.computeReplacementRange(offset);
   }
 
-  /// The analysis session that produced the elements of the request.
-  AnalysisSession get analysisSession => unit.session;
+  DartSnippetRequest.fromCompletionResult({
+    required ResolvedForCompletionResultImpl unit,
+    required this.offset,
+    required this.file,
+  }) : analysisSession = unit.analysisSession,
+       typeProvider = unit.unitElement.element.typeProvider,
+       content = unit.content,
+       compilationUnit = unit.parsedUnit,
+       libraryElement = unit.unitElement.element,
+       filePath = unit.path {
+    var target = CompletionTarget.forOffset(unit.parsedUnit, offset);
+    context = _getContext(target);
+    replacementRange = target.computeReplacementRange(offset);
+  }
 
   /// The resource provider associated with this request.
   ResourceProvider get resourceProvider => analysisSession.resourceProvider;

pkg/analysis_server/lib/src/services/snippets/snippet_producer.dart

@@ -21,7 +21,6 @@ import 'package:meta/meta.dart';
 abstract class DartSnippetProducer extends SnippetProducer {
   final AnalysisSessionHelper sessionHelper;
   final CorrectionUtils utils;
-  final LibraryElement libraryElement;
   final bool useSuperParams;
 
   /// A cache of mappings from Elements to their public Library Elements.
@@ -34,18 +33,20 @@ abstract class DartSnippetProducer extends SnippetProducer {
     super.request, {
     required Map<Element, LibraryElement?> elementImportCache,
   }) : sessionHelper = AnalysisSessionHelper(request.analysisSession),
-       utils = CorrectionUtils(request.unit),
-       libraryElement = request.unit.libraryElement,
-       useSuperParams = request.unit.libraryElement.featureSet.isEnabled(
+       utils = CorrectionUtils.fromUnitAndContent(
+         request.compilationUnit,
+         request.content,
+       ),
+       useSuperParams = request.libraryElement.featureSet.isEnabled(
          Feature.super_parameters,
        ),
        _elementImportCache = elementImportCache;
 
   CodeStyleOptions get codeStyleOptions => sessionHelper.session.analysisContext
-      .getAnalysisOptionsForFile(request.unit.file)
+      .getAnalysisOptionsForFile(request.file)
       .codeStyleOptions;
 
-  bool get isInTestDirectory => request.unit.unit.inTestDir;
+  bool get isInTestDirectory => request.compilationUnit.inTestDir;
 }
 
 abstract class FlutterSnippetProducer extends DartSnippetProducer {

pkg/analysis_server_plugin/api.txt

@@ -16,6 +16,7 @@ package:analysis_server_plugin/edit/assist/dart_assist_context.dart:
     workspace (getter: ChangeWorkspace)
 package:analysis_server_plugin/edit/correction_utils.dart:
   CorrectionUtils (class extends Object):
+    fromUnitAndContent (constructor: CorrectionUtils Function(CompilationUnit, String))
     new (constructor: CorrectionUtils Function(ParsedUnitResult))
     endOfLine (getter: String)
     oneIndent (getter: String)
@@ -200,6 +201,7 @@ package:analyzer/source/source_range.dart:
 package:analyzer/src/dart/ast/ast.dart:
   AstNode (referenced)
   ClassDeclaration (referenced)
+  CompilationUnit (referenced)
   CompilationUnitMember (referenced)
   EnumDeclaration (referenced)
   Expression (referenced)

pkg/analysis_server_plugin/lib/edit/correction_utils.dart

@@ -34,6 +34,10 @@ final class CorrectionUtils {
     : _unit = result.unit,
       _buffer = result.content;
 
+  CorrectionUtils.fromUnitAndContent(CompilationUnit unit, String content)
+    : _unit = unit,
+      _buffer = content;
+
   /// The EOL sequence to use for this [CompilationUnit].
   String get endOfLine {
     return _endOfLine ??= _buffer.endOfLine ?? Platform.lineTerminator;