Skip to content

Comment out _updateLocalClasses code #4394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
CoderDake merged 3 commits into from
Aug 19, 2022
Merged

Comment out _updateLocalClasses code #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e155de2
comment out update local classes code that his the inspector service …
CoderDake Aug 19, 2022
e175c5c
comment added
CoderDake Aug 19, 2022
a125455
skip the local classes test
CoderDake Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 22 additions & 19 deletions packages/devtools_app/lib/src/screens/inspector/inspector_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ class InspectorService extends InspectorServiceBase {

@override
bool isLocalClass(RemoteDiagnosticsNode node) {
// TODO(https://github.com/flutter/devtools/issues/4393): localClasses is
// not currently being filled.
if (node.widgetRuntimeType == null) return false;
// widgetRuntimeType may contain some generic type arguments which we need
// to strip out. If widgetRuntimeType is "FooWidget<Bar>" then we are only
Expand Down Expand Up @@ -388,25 +390,26 @@ class InspectorService extends InspectorServiceBase {
}

Future<void> _updateLocalClasses() async {
localClasses.clear();
if (_rootDirectories.value.isNotEmpty) {
final isolate = inspectorLibrary.isolate!;
for (var libraryRef in isolate.libraries!) {
if (isLocalUri(libraryRef.uri!)) {
try {
final Library library = await inspectorLibrary.service

@bkonyi bkonyi Aug 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is probably the culprit for the worst of the slowdown. This can probably be updated to something like this:

final isolate = inspectorLibrary.isolate!;
final futures = <Future>[];
for (final libraryRef in isolate.libraries!) {
  if (isLocalUri(libraryRef.uri!)) {
    final future = inspectorLibrary.service.getObject(
        isolate.id!, libraryRef.id!
      ).then((result) {
        final library = result as Library;
        for (final classRef in library.classes!) {
          localClasses[classRef.name!] = classRef;
        }
      });
    futures.add(future);
  }
}
await Future.wait(futures);

This will make it so that you're not waiting on individual requests before requesting the next library, so we can really take advantage of parallelism.

@CoderDake CoderDake Aug 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this feedback to the issue.
I'm not sure it would 100% solve the issue as the network gets flooded with requests, so it may still slow things down

@bkonyi bkonyi Aug 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, multiple outstanding network requests don't slow things down nearly as much as serializing them. This is a common pattern we use when we need to make a bunch of service requests (e.g., in the ProgramExplorer we do this to request all fields and functions in libraries and classes).

@jacob314 jacob314 Aug 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this pattern. Can't believe I wrote taht code. Wish I had an aggressive lint that said "no await within a for loop"

.getObject(isolate.id!, libraryRef.id!) as Library;
for (var classRef in library.classes!) {
localClasses[classRef.name!] = classRef;
}
} catch (e) {
// Workaround until https://github.com/flutter/devtools/issues/3110
// is fixed.
assert(serviceManager.connectedApp!.isDartWebAppNow!);
}
}
}
}
// TODO(https://github.com/flutter/devtools/issues/4393)
Comment thread
CoderDake marked this conversation as resolved.
// localClasses.clear();
// if (_rootDirectories.value.isNotEmpty) {
// final isolate = inspectorLibrary.isolate!;
// for (var libraryRef in isolate.libraries!) {
// if (isLocalUri(libraryRef.uri!)) {
// try {
// final Library library = await inspectorLibrary.service
// .getObject(isolate.id!, libraryRef.id!) as Library;
// for (var classRef in library.classes!) {
// localClasses[classRef.name!] = classRef;
// }
// } catch (e) {
// // Workaround until https://github.com/flutter/devtools/issues/3110
// // is fixed.
// assert(serviceManager.connectedApp!.isDartWebAppNow!);
// }
// }
// }
// }
}

@visibleForTesting
Expand Down
6 changes: 4 additions & 2 deletions packages/devtools_app/lib/src/shared/preferences.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,10 @@ class InspectorPreferencesController extends DisposableController
if (_customPubRootDirectories.value.isEmpty) {
// If there are no pub root directories set on the first connection
// then try inferring them.
await _inspectorService?.inferPubRootDirectoryIfNeeded();
await loadCustomPubRootDirectories();
await _customPubRootDirectoryBusyTracker(() async {
await _inspectorService?.inferPubRootDirectoryIfNeeded();
await loadCustomPubRootDirectories();
});
}
}

Expand Down
218 changes: 111 additions & 107 deletions packages/devtools_app/test/inspector/inspector_service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,113 +119,117 @@ void main() async {
);
});

test('local classes', () async {
await env.setupEnvironment();
final inspectorServiceLocal = inspectorService!;

final group = inspectorServiceLocal.createObjectGroup('test-group');
// These tests are moot if widget creation is not tracked.
expect(await inspectorServiceLocal.isWidgetCreationTracked(), isTrue);
await inspectorServiceLocal.addPubRootDirectories([]);
final List<String> rootDirectories =
await inspectorServiceLocal.inferPubRootDirectoryIfNeeded();
expect(rootDirectories.length, 1);
expect(rootDirectories.first, endsWith('/fixtures/flutter_app'));
final originalRootDirectories = rootDirectories.toList();
try {
expect(
(inspectorServiceLocal.localClasses.keys.toList()..sort()),
equals(
[
'AnotherClass',
'ExportedClass',
'FooClass',
'MyApp',
'MyOtherWidget',
'NotAWidget',
'_PrivateClass',
'_PrivateExportedClass',
],
),
);

await inspectorServiceLocal
.addPubRootDirectories(['${rootDirectories.first}/lib/src']);
// Adding src does not change the directory as local classes are
// computed at the library level.
expect(
(inspectorServiceLocal.localClasses.keys.toList()..sort()),
equals(
[
'AnotherClass',
'ExportedClass',
'FooClass',
'MyApp',
'MyOtherWidget',
'NotAWidget',
'_PrivateClass',
'_PrivateExportedClass'
],
),
);

expect(
inspectorServiceLocal.rootPackages.toList(),
equals(['flutter_app']),
);
expect(inspectorServiceLocal.rootPackagePrefixes.toList(), isEmpty);

await inspectorServiceLocal.addPubRootDirectories(
['/usr/jacobr/foo/lib', '/usr/jacobr/bar/lib/bla'],
);
expect(
inspectorServiceLocal.rootPackages.toList(),
equals(['foo', 'bar']),
);
expect(inspectorServiceLocal.rootPackagePrefixes.toList(), isEmpty);

expect(
inspectorServiceLocal.isLocalUri('package:foo/src/bar.dart'),
isTrue,
);
expect(
inspectorServiceLocal.isLocalUri('package:foo.bla/src/bar.dart'),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri('package:foos/src/bar.dart'),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri('package:bar/src/bar.dart'),
isTrue,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.core/src/bar.dart',
),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.core.bla/src/bar.dart',
),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.cores/src/bar.dart',
),
isFalse,
);
} finally {
// Restore.
await inspectorServiceLocal
.addPubRootDirectories(originalRootDirectories);

await group.dispose();
}
});
test(
'local classes',
() async {
await env.setupEnvironment();
final inspectorServiceLocal = inspectorService!;

final group = inspectorServiceLocal.createObjectGroup('test-group');
// These tests are moot if widget creation is not tracked.
expect(await inspectorServiceLocal.isWidgetCreationTracked(), isTrue);
await inspectorServiceLocal.addPubRootDirectories([]);
final List<String> rootDirectories =
await inspectorServiceLocal.inferPubRootDirectoryIfNeeded();
expect(rootDirectories.length, 1);
expect(rootDirectories.first, endsWith('/fixtures/flutter_app'));
final originalRootDirectories = rootDirectories.toList();
try {
expect(
(inspectorServiceLocal.localClasses.keys.toList()..sort()),
equals(
[
'AnotherClass',
'ExportedClass',
'FooClass',
'MyApp',
'MyOtherWidget',
'NotAWidget',
'_PrivateClass',
'_PrivateExportedClass',
],
),
);

await inspectorServiceLocal
.addPubRootDirectories(['${rootDirectories.first}/lib/src']);
// Adding src does not change the directory as local classes are
// computed at the library level.
expect(
(inspectorServiceLocal.localClasses.keys.toList()..sort()),
equals(
[
'AnotherClass',
'ExportedClass',
'FooClass',
'MyApp',
'MyOtherWidget',
'NotAWidget',
'_PrivateClass',
'_PrivateExportedClass'
],
),
);

expect(
inspectorServiceLocal.rootPackages.toList(),
equals(['flutter_app']),
);
expect(inspectorServiceLocal.rootPackagePrefixes.toList(), isEmpty);

await inspectorServiceLocal.addPubRootDirectories(
['/usr/jacobr/foo/lib', '/usr/jacobr/bar/lib/bla'],
);
expect(
inspectorServiceLocal.rootPackages.toList(),
equals(['foo', 'bar']),
);
expect(inspectorServiceLocal.rootPackagePrefixes.toList(), isEmpty);

expect(
inspectorServiceLocal.isLocalUri('package:foo/src/bar.dart'),
isTrue,
);
expect(
inspectorServiceLocal.isLocalUri('package:foo.bla/src/bar.dart'),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri('package:foos/src/bar.dart'),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri('package:bar/src/bar.dart'),
isTrue,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.core/src/bar.dart',
),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.core.bla/src/bar.dart',
),
isFalse,
);
expect(
inspectorServiceLocal.isLocalUri(
'package:bar.cores/src/bar.dart',
),
isFalse,
);
} finally {
// Restore.
await inspectorServiceLocal
.addPubRootDirectories(originalRootDirectories);

await group.dispose();
}
},
skip: true, // TODO(https://github.com/flutter/devtools/issues/4393)
);

test('local classes for bazel projects', () async {
await env.setupEnvironment();
Expand Down