From 667e879ded7d5cb0a927199cbdb138fae04fd3ee Mon Sep 17 00:00:00 2001 From: Konstantin Shcheglov Date: Sat, 25 Sep 2021 17:56:10 +0000 Subject: [PATCH] Remove only libraries potentially affected by the changed file. For a small analyzer library the time to analyze it 100 times, each time changing that file that we are analyzing, so simulating typing in this file, goes from 1000 ms to 100 ms; for a bigger library 2400 vs 800 ms. For a tiny Flutter hello_world the difference is 100x, 5800 vs 50 ms. Bug: https://github.com/flutter/flutter-intellij/issues/5761 Change-Id: I15c43f4cd66399788ffe6032c52fb365665de7f8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/214502 Reviewed-by: Brian Wilkerson Commit-Queue: Konstantin Shcheglov --- .../test/lsp/server_abstract.dart | 2 +- .../lib/src/dart/analysis/driver.dart | 84 ++++++---- .../lib/src/dart/analysis/file_state.dart | 20 +++ .../lib/src/dart/analysis/file_tracker.dart | 16 +- .../src/dart/analysis/library_context.dart | 5 + .../lib/src/dart/element/element.dart | 6 + .../src/summary2/linked_element_factory.dart | 17 +- pkg/analyzer/lib/src/summary2/reference.dart | 4 +- .../test/src/dart/analysis/driver_test.dart | 154 +++++++++++++++++- 9 files changed, 253 insertions(+), 55 deletions(-) diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart index b4d14d7c603..26cd6166f24 100644 --- a/pkg/analysis_server/test/lsp/server_abstract.dart +++ b/pkg/analysis_server/test/lsp/server_abstract.dart @@ -1467,7 +1467,7 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin { text: content)), ); await sendNotificationToServer(notification); - await pumpEventQueue(); + await pumpEventQueue(times: 128); } int positionCompare(Position p1, Position p2) { diff --git a/pkg/analyzer/lib/src/dart/analysis/driver.dart b/pkg/analyzer/lib/src/dart/analysis/driver.dart index fcf5d4e5a4a..3e16e13dc32 100644 --- a/pkg/analyzer/lib/src/dart/analysis/driver.dart +++ b/pkg/analyzer/lib/src/dart/analysis/driver.dart @@ -222,7 +222,7 @@ class AnalysisDriver implements AnalysisDriverGeneric { final bool enableIndex; /// The current analysis session. - late AnalysisSessionImpl _currentSession; + late final AnalysisSessionImpl _currentSession = AnalysisSessionImpl(this); /// The current library context, consistent with the [_currentSession]. /// @@ -299,7 +299,6 @@ class AnalysisDriver implements AnalysisDriverGeneric { _sourceFactory = sourceFactory, _externalSummaries = externalSummaries, testingData = retainDataForTesting ? TestingData() : null { - _createNewSession(); _onResults = _resultController.stream.asBroadcastStream(); _testView = AnalysisDriverTestView(this); _createFileTracker(); @@ -461,14 +460,10 @@ class AnalysisDriver implements AnalysisDriverGeneric { return; } if (file_paths.isDart(resourceProvider.pathContext, path)) { + _priorityResults.clear(); + _removePotentiallyAffectedLibraries(path); _fileTracker.addFile(path); - // If the file is known, it has already been read, even if it did not - // exist. Now we are notified that the file exists, so we need to - // re-read it and make sure that we invalidate signature of the files - // that reference it. - if (_fsState.knownFilePaths.contains(path)) { - _changeFile(path); - } + _scheduler.notify(this); } } @@ -490,7 +485,15 @@ class AnalysisDriver implements AnalysisDriverGeneric { /// [changeFile] invocation. void changeFile(String path) { _throwIfNotAbsolutePath(path); - _changeFile(path); + if (!_fsState.hasUri(path)) { + return; + } + if (file_paths.isDart(resourceProvider.pathContext, path)) { + _priorityResults.clear(); + _removePotentiallyAffectedLibraries(path); + _fileTracker.changeFile(path); + _scheduler.notify(this); + } } /// Clear the library context and any related data structures. Mostly we do @@ -500,6 +503,7 @@ class AnalysisDriver implements AnalysisDriverGeneric { /// periodically. @visibleForTesting void clearLibraryContext() { + _libraryContext?.invalidAllLibraries(); _libraryContext = null; _currentSession.clearHierarchies(); } @@ -528,9 +532,15 @@ class AnalysisDriver implements AnalysisDriverGeneric { if (sourceFactory != null) { _sourceFactory = sourceFactory; } + + _priorityResults.clear(); + clearLibraryContext(); + Iterable addedFiles = _fileTracker.addedFiles; _createFileTracker(); _fileTracker.addFiles(addedFiles); + + _scheduler.notify(this); } /// Return a [Future] that completes when discovery of all files that are @@ -1197,17 +1207,25 @@ class AnalysisDriver implements AnalysisDriverGeneric { /// but does not guarantee this. void removeFile(String path) { _throwIfNotAbsolutePath(path); - _fileTracker.removeFile(path); - clearLibraryContext(); - _priorityResults.clear(); + if (!_fsState.hasUri(path)) { + return; + } + if (file_paths.isDart(resourceProvider.pathContext, path)) { + _priorityResults.clear(); + _removePotentiallyAffectedLibraries(path); + _fileTracker.removeFile(path); + _scheduler.notify(this); + } } /// Reset URI resolution, read again all files, build files graph, and ensure /// that for all added files new results are reported. void resetUriResolution() { + _priorityResults.clear(); + clearLibraryContext(); _fsState.resetUriResolution(); _fileTracker.scheduleAllAddedFiles(); - _changeHook(); + _scheduler.notify(this); } void _addDeclaredVariablesToSignature(ApiSignature buffer) { @@ -1221,22 +1239,6 @@ class AnalysisDriver implements AnalysisDriverGeneric { } } - /// Implementation for [changeFile]. - void _changeFile(String path) { - _fileTracker.changeFile(path); - clearLibraryContext(); - _priorityResults.clear(); - } - - /// Handles a notification from the [FileTracker] that there has been a change - /// of state. - void _changeHook() { - _createNewSession(); - clearLibraryContext(); - _priorityResults.clear(); - _scheduler.notify(this); - } - /// There was an exception during a file analysis, we don't know why. /// But it might have been caused by an inconsistency of files state, and /// the library context state. Reset the library context, and hope that @@ -1483,7 +1485,7 @@ class AnalysisDriver implements AnalysisDriverGeneric { externalSummaries: _externalSummaries, fileContentCache: _fileContentCache, ); - _fileTracker = FileTracker(_logger, _fsState, _changeHook); + _fileTracker = FileTracker(_logger, _fsState); } /// Return the context in which the [library] should be analyzed. @@ -1516,11 +1518,6 @@ class AnalysisDriver implements AnalysisDriverGeneric { return libraryContext; } - /// Create a new analysis session, so invalidating the current one. - void _createNewSession() { - _currentSession = AnalysisSessionImpl(this); - } - /// If this has not been done yet, schedule discovery of all files that are /// potentially available, so that they are included in [knownFiles]. void _discoverAvailableFiles() { @@ -1682,6 +1679,18 @@ class AnalysisDriver implements AnalysisDriverGeneric { 'missing', errorsResult, AnalysisDriverUnitIndexBuilder()); } + void _removePotentiallyAffectedLibraries(String path) { + _logger.run('Invalidate affected by $path.', () { + _logger.writeln('Work in $name'); + var affected = {}; + _fsState.collectAffected(path, affected); + _logger.writeln('Remove ${affected.length} libraries.'); + _libraryContext?.elementFactory.removeLibraries( + affected.map((e) => e.uriStr).toSet(), + ); + }); + } + void _reportException(String path, Object exception, StackTrace stackTrace) { String? contextKey; if (exception is _ExceptionState) { @@ -2053,6 +2062,9 @@ class AnalysisDriverTestView { FileTracker get fileTracker => driver._fileTracker; + /// TODO(scheglov) Rename this, and [libraryContext]. + LibraryContext? get libraryContext2 => driver._libraryContext; + Map get priorityResults { return driver._priorityResults; } diff --git a/pkg/analyzer/lib/src/dart/analysis/file_state.dart b/pkg/analyzer/lib/src/dart/analysis/file_state.dart index e3387ae6f9b..eff19a91e65 100644 --- a/pkg/analyzer/lib/src/dart/analysis/file_state.dart +++ b/pkg/analyzer/lib/src/dart/analysis/file_state.dart @@ -781,6 +781,26 @@ class FileSystemState { @visibleForTesting FileSystemStateTestView get test => _testView; + /// Collected files that transitively reference a file with the [path]. + /// These files are potentially affected by the change. + void collectAffected(String path, Set affected) { + final knownFiles = this.knownFiles.toList(); + + collectAffected(FileState file) { + if (affected.add(file)) { + for (var other in knownFiles) { + if (other.directReferencedFiles.contains(file)) { + collectAffected(other); + } + } + } + } + + for (var file in _pathToFiles[path] ?? []) { + collectAffected(file); + } + } + FeatureSet contextFeatureSet( String path, Uri uri, diff --git a/pkg/analyzer/lib/src/dart/analysis/file_tracker.dart b/pkg/analyzer/lib/src/dart/analysis/file_tracker.dart index 0494153a84c..45ea739a7de 100644 --- a/pkg/analyzer/lib/src/dart/analysis/file_tracker.dart +++ b/pkg/analyzer/lib/src/dart/analysis/file_tracker.dart @@ -16,10 +16,6 @@ import 'package:analyzer/src/dart/analysis/performance_logger.dart'; /// /// Provides methods for updating the file system state in response to changes. class FileTracker { - /// Callback invoked whenever a change occurs that may require the client to - /// perform analysis. - final void Function() _changeHook; - /// The logger to write performed operations and performance to. final PerformanceLog _logger; @@ -49,7 +45,7 @@ class FileTracker { /// have any special relation with changed files. var _pendingFiles = {}; - FileTracker(this._logger, this._fsState, this._changeHook); + FileTracker(this._logger, this._fsState); /// Returns the path to exactly one that needs analysis. Throws a /// [StateError] if no files need analysis. @@ -100,27 +96,24 @@ class FileTracker { /// Adds the given [path] to the set of "added files". void addFile(String path) { - _fsState.markFileForReading(path); addedFiles.add(path); - _pendingFiles.add(path); - _changeHook(); + changeFile(path); } /// Adds the given [paths] to the set of "added files". void addFiles(Iterable paths) { addedFiles.addAll(paths); _pendingFiles.addAll(paths); - _changeHook(); } /// Adds the given [path] to the set of "changed files". void changeFile(String path) { + _fsState.markFileForReading(path); _changedFiles.add(path); + if (addedFiles.contains(path)) { _pendingChangedFiles.add(path); } - _fsState.markFileForReading(path); - _changeHook(); } /// Removes the given [path] from the set of "pending files". @@ -165,7 +158,6 @@ class FileTracker { // files seems extreme. _fsState.removeFile(path); _pendingFiles.addAll(addedFiles); - _changeHook(); } /// Schedule all added files for analysis. diff --git a/pkg/analyzer/lib/src/dart/analysis/library_context.dart b/pkg/analyzer/lib/src/dart/analysis/library_context.dart index f80a74492b6..cab5839750b 100644 --- a/pkg/analyzer/lib/src/dart/analysis/library_context.dart +++ b/pkg/analyzer/lib/src/dart/analysis/library_context.dart @@ -89,6 +89,11 @@ class LibraryContext { return elementFactory.libraryOfUri2('$uri'); } + /// We are about to discard this context, mark all libraries invalid. + void invalidAllLibraries() { + elementFactory.invalidateAllLibraries(); + } + /// Load data required to access elements of the given [targetLibrary]. void load2(FileState targetLibrary) { timerLoad2.start(); diff --git a/pkg/analyzer/lib/src/dart/element/element.dart b/pkg/analyzer/lib/src/dart/element/element.dart index 766ace46203..994614b3262 100644 --- a/pkg/analyzer/lib/src/dart/element/element.dart +++ b/pkg/analyzer/lib/src/dart/element/element.dart @@ -3723,6 +3723,12 @@ class LibraryElementImpl extends _ExistingElementImpl @override final AnalysisSession session; + /// If `true`, then this library is valid in the session. + /// + /// A library becomes invalid when one of its files, or one of its + /// dependencies, changes. + bool isValid = true; + /// The language version for the library. LibraryLanguageVersion? _languageVersion; diff --git a/pkg/analyzer/lib/src/summary2/linked_element_factory.dart b/pkg/analyzer/lib/src/summary2/linked_element_factory.dart index e83e4928f55..456548ea9d5 100644 --- a/pkg/analyzer/lib/src/summary2/linked_element_factory.dart +++ b/pkg/analyzer/lib/src/summary2/linked_element_factory.dart @@ -164,6 +164,13 @@ class LinkedElementFactory { return libraryReaders[uriStr] != null; } + /// We are about to discard this factory, mark all libraries invalid. + void invalidateAllLibraries() { + for (var libraryReference in rootReference.children) { + _invalidateLibrary(libraryReference); + } + } + LibraryElementImpl? libraryOfUri(String uriStr) { var reference = rootReference.getChild(uriStr); return elementOfReference(reference) as LibraryElementImpl?; @@ -189,7 +196,8 @@ class LinkedElementFactory { for (var uriStr in uriStrSet) { _exportsOfLibrary.remove(uriStr); libraryReaders.remove(uriStr); - rootReference.removeChild(uriStr); + var libraryReference = rootReference.removeChild(uriStr); + _invalidateLibrary(libraryReference); } analysisSession.classHierarchy.removeOfLibraries(uriStrSet); @@ -239,4 +247,11 @@ class LinkedElementFactory { libraryElement.createLoadLibraryFunction(); } + + void _invalidateLibrary(Reference? libraryReference) { + var libraryElement = libraryReference?.element; + if (libraryElement is LibraryElementImpl) { + libraryElement.isValid = false; + } + } } diff --git a/pkg/analyzer/lib/src/summary2/reference.dart b/pkg/analyzer/lib/src/summary2/reference.dart index e73b634fc87..8c305bde616 100644 --- a/pkg/analyzer/lib/src/summary2/reference.dart +++ b/pkg/analyzer/lib/src/summary2/reference.dart @@ -65,8 +65,8 @@ class Reference { return map[name] ??= Reference._(this, name); } - void removeChild(String name) { - _children?.remove(name); + Reference? removeChild(String name) { + return _children?.remove(name); } @override diff --git a/pkg/analyzer/test/src/dart/analysis/driver_test.dart b/pkg/analyzer/test/src/dart/analysis/driver_test.dart index 29a974ac1ae..f2b9cde619f 100644 --- a/pkg/analyzer/test/src/dart/analysis/driver_test.dart +++ b/pkg/analyzer/test/src/dart/analysis/driver_test.dart @@ -727,6 +727,128 @@ var A = B; expect(driver.fsState.knownFilePaths, isNot(contains(b))); } + test_changeFile_potentiallyAffected_imported() async { + newFile('/test/lib/a.dart', content: ''); + var b = newFile('/test/lib/b.dart', content: ''' +import 'a.dart'; +'''); + newFile('/test/lib/c.dart', content: ''' +import 'b.dart'; +'''); + newFile('/test/lib/d.dart', content: ''' +import 'c.dart'; +'''); + newFile('/test/lib/e.dart', content: ''); + + Future getLibrary(String shortName) async { + var uriStr = 'package:test/$shortName'; + var result = await driver.getLibraryByUriValid(uriStr); + return result.element as LibraryElementImpl; + } + + var a_element = await getLibrary('a.dart'); + var b_element = await getLibrary('b.dart'); + var c_element = await getLibrary('c.dart'); + var d_element = await getLibrary('d.dart'); + var e_element = await getLibrary('e.dart'); + + // We have all libraries loaded after analysis. + driver.assertLoadedLibraryUriSet( + included: [ + 'package:test/a.dart', + 'package:test/b.dart', + 'package:test/c.dart', + 'package:test/d.dart', + 'package:test/e.dart', + ], + ); + + // All libraries are valid. + expect(a_element.isValid, true); + expect(b_element.isValid, true); + expect(c_element.isValid, true); + expect(d_element.isValid, true); + expect(e_element.isValid, true); + + // Change `b.dart`, also removes `c.dart` and `d.dart` that import it. + // But `a.dart` and `d.dart` is not affected. + driver.changeFile(b.path); + driver.assertLoadedLibraryUriSet( + excluded: [ + 'package:test/b.dart', + 'package:test/c.dart', + 'package:test/d.dart', + ], + included: [ + 'package:test/a.dart', + 'package:test/e.dart', + ], + ); + + // Only `a.dart` and `e.dart` are still valid. + expect(a_element.isValid, true); + expect(b_element.isValid, false); + expect(c_element.isValid, false); + expect(d_element.isValid, false); + expect(e_element.isValid, true); + } + + test_changeFile_potentiallyAffected_part() async { + var a = newFile('/test/lib/a.dart', content: ''' +part of 'b.dart'; +'''); + newFile('/test/lib/b.dart', content: ''' +part 'a.dart'; +'''); + newFile('/test/lib/c.dart', content: ''' +import 'b.dart'; +'''); + newFile('/test/lib/d.dart', content: ''); + + Future getLibrary(String shortName) async { + var uriStr = 'package:test/$shortName'; + var result = await driver.getLibraryByUriValid(uriStr); + return result.element as LibraryElementImpl; + } + + var b_element = await getLibrary('b.dart'); + var c_element = await getLibrary('c.dart'); + var d_element = await getLibrary('d.dart'); + + // We have all libraries loaded after analysis. + driver.assertLoadedLibraryUriSet( + included: [ + 'package:test/b.dart', + 'package:test/c.dart', + 'package:test/d.dart', + ], + ); + + // All libraries are valid. + expect(b_element.isValid, true); + expect(c_element.isValid, true); + expect(d_element.isValid, true); + + // Change `a.dart`, remove `b.dart` that part it. + // Removes `c.dart` that imports `b.dart`. + // But `d.dart` is not affected. + driver.changeFile(a.path); + driver.assertLoadedLibraryUriSet( + excluded: [ + 'package:test/b.dart', + 'package:test/c.dart', + ], + included: [ + 'package:test/d.dart', + ], + ); + + // Only `d.dart` is still valid. + expect(b_element.isValid, false); + expect(c_element.isValid, false); + expect(d_element.isValid, true); + } + test_changeFile_selfConsistent() async { var a = convertPath('/test/lib/a.dart'); var b = convertPath('/test/lib/b.dart'); @@ -987,7 +1109,7 @@ const x = 1; } test_currentSession() async { - var a = convertPath('/a.dart'); + var a = convertPath('/test/lib/a.dart'); newFile(a, content: 'var V = 1;'); await driver.getResultValid(a); @@ -1002,8 +1124,9 @@ const x = 1; var session2 = driver.currentSession; expect(session2, isNotNull); - // We get a new session. - expect(session2, isNot(session1)); + // We don't discard the session anymore. + // So, the session is always the same. + expect(session2, same(session1)); } test_discoverAvailableFiles_packages() async { @@ -3349,7 +3472,32 @@ extension on AnalysisDriver { return getFileSync2(path) as FileResult; } + Set get loadedLibraryUriSet { + var elementFactory = this.test.libraryContext2!.elementFactory; + var libraryReferences = elementFactory.rootReference.children; + return libraryReferences.map((e) => e.name).toSet(); + } + + void assertLoadedLibraryUriSet({ + Iterable? included, + Iterable? excluded, + }) { + var uriSet = loadedLibraryUriSet; + if (included != null) { + expect(uriSet, containsAll(included)); + } + if (excluded != null) { + for (var excludedUri in excluded) { + expect(uriSet, isNot(contains(excludedUri))); + } + } + } + Future getResultValid(String path) async { return await getResult2(path) as ResolvedUnitResult; } + + Future getLibraryByUriValid(String uriStr) async { + return await getLibraryByUri2(uriStr) as LibraryElementResult; + } }