Skip to content

Commit

Permalink
[analyzer] Don't ask for subtypes for the same elements several times…
Browse files Browse the repository at this point in the history
… when searching for references

When asking for all references of a member it starts by finding all
supertypes for the class, then for each supertype (including itself)
(that has a member with the same name as the one we're querying for)
it searches for all subtypes.

We recall that when getting all subtypes, it really gets all direct
subtypes, then for each of those, their direct subtypes etc.
Naturally, then, with getting all subtypes for all supertypes we
will often ask for all direct subtypes of the same class several times.

In fact, it's O(n^2):

As an example, if asking for references to `foo` on this
(in a file in analyzer, with 8 workspaces):
```
class X0 { void foo() { print('hello'); } }
class X1 extends X0 { void foo() { print('hello'); } }
class X2 extends X1 { void foo() { print('hello'); } }
[...]
class X149 extends X148 { void foo() { print('hello'); } }
```
we ask for subtypes 90600 (150 classes * 151 / 2 * 8 workspaces) times.

This CL stops that from happening and in the example from above we
"only" asks for subtypes 1200 (150 classes * 8 workspaces) times.

For the `newFile` from `AbstractSingleUnitTest` example, for instance,
we go from 28,264 asks to 17,432 asks (a ~38% reduction).

Non-first runtimes when asking for references:

Before:
0:00:03.074853
0:00:03.021881
0:00:03.034707
0:00:03.115596
0:00:03.032574

After:
0:00:02.223978
0:00:02.149937
0:00:02.150236
0:00:02.104704
0:00:02.175859

Difference at 95.0% confidence
        -0.894979 +/- 0.060283
        -29.2867% +/- 1.97266%
        (Student's t, pooled s = 0.0413338)

Change-Id: Id792a595e74de01c7186ab1263c38728f051f603
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/272623
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
  • Loading branch information
jensjoha authored and Commit Queue committed Dec 5, 2022
1 parent a464fc3 commit b812517
Show file tree
Hide file tree
Showing 8 changed files with 109 additions and 51 deletions.
Expand Up @@ -61,10 +61,11 @@ class ImplementationHandler
}
final needsMember = helper.findMemberElement(interfaceElement) != null;

Set<InterfaceElement> allSubtypes = await performance.runAsync(
"searchAllSubtypes",
var allSubtypes = <InterfaceElement>{};
await performance.runAsync(
"appendAllSubtypes",
(performance) => server.searchEngine
.searchAllSubtypes(interfaceElement, performance: performance));
.appendAllSubtypes(interfaceElement, allSubtypes, performance));
final locations = performance.run(
"filter and get location",
(_) => allSubtypes
Expand Down
Expand Up @@ -20,6 +20,7 @@ import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/dart/analysis/session_helper.dart';
import 'package:analyzer/src/generated/java_core.dart';
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/util/performance/operation_performance.dart';

/// Checks if creating a method with the given [name] in [interfaceElement] will
/// cause any conflicts.
Expand Down Expand Up @@ -275,7 +276,9 @@ class _CreateClassMemberValidator extends _BaseClassMemberValidator {
Future<RefactoringStatus> validate() async {
_checkClassAlreadyDeclares();
// do chained computations
var subClasses = await searchEngine.searchAllSubtypes(interfaceElement);
var subClasses = <InterfaceElement>{};
await searchEngine.appendAllSubtypes(
interfaceElement, subClasses, OperationPerformanceImpl("<root>"));
// check shadowing of class names
if (interfaceElement.name == name) {
result.addError(
Expand Down Expand Up @@ -363,7 +366,9 @@ class _RenameClassMemberValidator extends _BaseClassMemberValidator {
_checkClassAlreadyDeclares();
// do chained computations
await _prepareReferences();
var subClasses = await searchEngine.searchAllSubtypes(interfaceElement);
var subClasses = <InterfaceElement>{};
await searchEngine.appendAllSubtypes(
interfaceElement, subClasses, OperationPerformanceImpl("<root>"));
// check shadowing of class names
for (var element in elements) {
var enclosingElement = element.enclosingElement;
Expand Down
22 changes: 12 additions & 10 deletions pkg/analysis_server/lib/src/services/search/hierarchy.dart
Expand Up @@ -105,23 +105,25 @@ Future<Set<ClassMemberElement>> getHierarchyMembers(
...enclosingElement.allSupertypes.map((e) => e.element),
enclosingElement,
];
var subClasses = <InterfaceElement>{};
for (var superClass in searchClasses) {
// ignore if super- class does not declare member
if (getClassMembers(superClass, name).isEmpty) {
continue;
}
// check all sub- classes
var subClasses = await performance.runAsync(
"searchAllSubtypes",
(performance) => searchEngine.searchAllSubtypes(superClass,
performance: performance));
await performance.runAsync(
"appendAllSubtypes",
(performance) => searchEngine.appendAllSubtypes(
superClass, subClasses, performance));
subClasses.add(superClass);
for (var subClass in subClasses) {
var subClassMembers = getChildren(subClass, name);
for (var member in subClassMembers) {
if (member is ClassMemberElement) {
result.add(member);
}
}

for (var subClass in subClasses) {
var subClassMembers = getChildren(subClass, name);
for (var member in subClassMembers) {
if (member is ClassMemberElement) {
result.add(member);
}
}
}
Expand Down
15 changes: 9 additions & 6 deletions pkg/analysis_server/lib/src/services/search/search_engine.dart
Expand Up @@ -70,17 +70,20 @@ class MatchKind {
/// The interface [SearchEngine] defines the behavior of objects that can be
/// used to search for various pieces of information.
abstract class SearchEngine {
/// Adds all subtypes of the given [type] into [allSubtypes].
///
/// If [allSubtypes] already contains an element it is assumed that it
/// contains the entire subtree and the element won't be search on further.
///
/// [type] - the [InterfaceElement] being subtyped by the found matches.
Future<void> appendAllSubtypes(InterfaceElement type,
Set<InterfaceElement> allSubtypes, OperationPerformanceImpl performance);

/// If the [type] has subtypes, return the set of names of members which these
/// subtypes declare, possibly empty. If the [type] does not have subtypes,
/// return `null`.
Future<Set<String>?> membersOfSubtypes(InterfaceElement type);

/// Returns all subtypes of the given [type].
///
/// [type] - the [InterfaceElement] being subtyped by the found matches.
Future<Set<InterfaceElement>> searchAllSubtypes(InterfaceElement type,
{OperationPerformanceImpl? performance});

/// Returns declarations of class members with the given name.
///
/// [name] - the name being declared by the found matches.
Expand Down
Expand Up @@ -17,6 +17,29 @@ class SearchEngineImpl implements SearchEngine {

SearchEngineImpl(this._drivers);

@override
Future<void> appendAllSubtypes(
InterfaceElement type,
Set<InterfaceElement> allSubtypes,
OperationPerformanceImpl performance) async {
var searchEngineCache = SearchEngineCache();

Future<void> addSubtypes(InterfaceElement type) async {
var directResults = await performance.runAsync(
"_searchDirectSubtypes",
(performance) =>
_searchDirectSubtypes(type, searchEngineCache, performance));
for (var directResult in directResults) {
var directSubtype = directResult.enclosingElement as InterfaceElement;
if (allSubtypes.add(directSubtype)) {
await addSubtypes(directSubtype);
}
}
}

await addSubtypes(type);
}

@override
Future<Set<String>?> membersOfSubtypes(InterfaceElement type) async {
var drivers = _drivers.toList();
Expand Down Expand Up @@ -53,33 +76,6 @@ class SearchEngineImpl implements SearchEngine {
return members;
}

@override
Future<Set<InterfaceElement>> searchAllSubtypes(
InterfaceElement type, {
// TODO(jensj): Possibly make this required.
OperationPerformanceImpl? performance,
}) async {
var nnPerformance = performance ?? OperationPerformanceImpl("<root>");
var allSubtypes = <InterfaceElement>{};
var searchEngineCache = SearchEngineCache();

Future<void> addSubtypes(InterfaceElement type) async {
var directResults = await nnPerformance.runAsync(
"_searchDirectSubtypes",
(performance) =>
_searchDirectSubtypes(type, searchEngineCache, performance));
for (var directResult in directResults) {
var directSubtype = directResult.enclosingElement as InterfaceElement;
if (allSubtypes.add(directSubtype)) {
await addSubtypes(directSubtype);
}
}
}

await addSubtypes(type);
return allSubtypes;
}

@override
Future<List<SearchMatch>> searchMemberDeclarations(String name) async {
var allDeclarations = <SearchMatch>[];
Expand Down
38 changes: 38 additions & 0 deletions pkg/analysis_server/test/services/search/hierarchy_test.dart
Expand Up @@ -5,6 +5,7 @@
import 'package:analysis_server/src/services/search/hierarchy.dart';
import 'package:analysis_server/src/services/search/search_engine_internal.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/util/performance/operation_performance.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';

Expand Down Expand Up @@ -144,6 +145,43 @@ class C extends B {
}
}

Future<void> test_getHierarchyMembers_linear_number_of_calls() async {
const count = 150;
const last = count - 1;
StringBuffer sb = StringBuffer();
for (int i = 0; i < count; i++) {
if (i == 0) {
sb.writeln("class X0 { void foo() { print('hello'); } }");
} else {
sb.writeln(
"class X$i extends X${i - 1} { void foo() { print('hello'); } }");
}
}

await _indexTestUnit(sb.toString());
var classLast = findElement.class_('X$last');
ClassMemberElement member =
classLast.methods.where((element) => element.name == "foo").single;
OperationPerformanceImpl performance = OperationPerformanceImpl("<root>");
var result = await performance.runAsync(
"getHierarchyMembers",
(performance) => getHierarchyMembers(searchEngine, member,
performance: performance));
expect(result, hasLength(count));

var worklist = <OperationPerformance>[];
worklist.add(performance);
while (worklist.isNotEmpty) {
var performance = worklist.removeLast();
expect(
performance.count,
lessThanOrEqualTo(count + 1),
reason: performance.toString(),
);
worklist.addAll(performance.children);
}
}

Future<void> test_getHierarchyMembers_methods() async {
await _indexTestUnit('''
class A {
Expand Down
13 changes: 10 additions & 3 deletions pkg/analysis_server/test/services/search/search_engine_test.dart
Expand Up @@ -9,6 +9,7 @@ import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/test_utilities/find_element.dart';
import 'package:analyzer/src/test_utilities/find_node.dart';
import 'package:analyzer/src/test_utilities/package_config_file_builder.dart';
import 'package:analyzer/src/util/performance/operation_performance.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';

Expand Down Expand Up @@ -209,7 +210,9 @@ class C implements B {}

var element = findElement.class_('T');

var subtypes = await searchEngine.searchAllSubtypes(element);
var subtypes = <InterfaceElement>{};
await searchEngine.appendAllSubtypes(
element, subtypes, OperationPerformanceImpl("<root>"));
expect(subtypes, hasLength(3));
_assertContainsClass(subtypes, 'A');
_assertContainsClass(subtypes, 'B');
Expand All @@ -233,7 +236,9 @@ class C extends B {}
await resolveFile2('$aaaRootPath/lib/a.dart');
var element = findElement.class_('T');

var subtypes = await searchEngine.searchAllSubtypes(element);
var subtypes = <InterfaceElement>{};
await searchEngine.appendAllSubtypes(
element, subtypes, OperationPerformanceImpl("<root>"));
expect(subtypes, hasLength(3));
_assertContainsClass(subtypes, 'A');
_assertContainsClass(subtypes, 'B');
Expand All @@ -255,7 +260,9 @@ mixin E implements C {}

var element = findElement.class_('T');

var subtypes = await searchEngine.searchAllSubtypes(element);
var subtypes = <InterfaceElement>{};
await searchEngine.appendAllSubtypes(
element, subtypes, OperationPerformanceImpl("<root>"));
expect(subtypes, hasLength(5));
_assertContainsClass(subtypes, 'A');
_assertContainsClass(subtypes, 'B');
Expand Down
Expand Up @@ -9,6 +9,9 @@ abstract class OperationPerformance {
/// The child operations, might be empty.
List<OperationPerformance> get children;

/// The number of times this child has been started/run.
int get count;

/// The data attachments, for non-timing data, e.g. how many files were read,
/// or how many bytes were processed.
List<OperationPerformanceData> get data;
Expand Down Expand Up @@ -84,6 +87,9 @@ class OperationPerformanceImpl implements OperationPerformance {
return _children;
}

@override
int get count => _count;

@override
List<OperationPerformanceData<Object>> get data {
return _data.values.toList();
Expand Down

0 comments on commit b812517

Please sign in to comment.