Skip to content

Cache the results of getVersion and getIsolate #3309

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
merged 13 commits into from
Sep 3, 2021
Merged

Cache the results of getVersion and getIsolate #3309

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1e357ca
Cache calls to getIsolate
elliette Aug 27, 2021
b50ca27
Cache getVersion requests as well
elliette Aug 27, 2021
72b01af
Clean up
elliette Aug 27, 2021
da00d2f
Use cached isolate in vmServiceOpened
elliette Aug 30, 2021
f2fdb7b
Clear isolates cache after start up
elliette Aug 30, 2021
9329e7e
use existing cache
elliette Sep 2, 2021
681b3a9
Merge branch 'master' into ipl-improvement
elliette Sep 2, 2021
4e014fa
Fetch the isolate in loadIsolateState
elliette Sep 2, 2021
e832435
delete cache
elliette Sep 2, 2021
ab6758e
actually delete the cache
elliette Sep 2, 2021
a559dc8
Clear the cache again
elliette Sep 3, 2021
b126831
register isolate
elliette Sep 3, 2021
e9d62ec
Tweaks
elliette Sep 3, 2021
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
62 changes: 37 additions & 25 deletions packages/devtools_app/lib/src/service_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ const defaultRefreshRate = 60.0;
// instead of Streams.
class ServiceConnectionManager {
ServiceConnectionManager() {
_serviceExtensionManager =
ServiceExtensionManager(isolateManager.mainIsolate);
_serviceExtensionManager = ServiceExtensionManager(isolateManager);
}

final StreamController<VmServiceWrapper> _connectionAvailableController =
Expand Down Expand Up @@ -763,13 +762,13 @@ class IsolateManager extends Disposer {

/// Manager that handles tracking the service extension for the main isolate.
class ServiceExtensionManager extends Disposer {
ServiceExtensionManager(this._mainIsolate);
ServiceExtensionManager(this._isolateManager);

VmServiceWrapper _service;

bool _checkForFirstFrameStarted = false;

final ValueListenable<IsolateRef> _mainIsolate;
final IsolateManager _isolateManager;

Future<void> get firstFrameReceived => _firstFrameReceived.future;
Completer<void> _firstFrameReceived = Completer();
Expand Down Expand Up @@ -903,21 +902,30 @@ class ServiceExtensionManager extends Disposer {
}

Future<void> _onMainIsolateChanged() async {
if (_mainIsolate.value == null) {
if (_isolateManager.mainIsolate.value == null) {
_mainIsolateClosed();
return;
}
_checkForFirstFrameStarted = false;

final isolateRef = _mainIsolate.value;
final Isolate isolate = await _service.getIsolate(isolateRef.id);
if (isolateRef != _mainIsolate.value) {
final isolateRef = _isolateManager.mainIsolate.value;
final Isolate isolate = await _isolateManager.getIsolateCached(isolateRef);

await _registerIsolate(isolate, isolateRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

call this _registerMainIsolate for clarity.

}

Future<void> _registerIsolate(Isolate isolate,
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter name should not be previousIsolateRef. It should be expectedMainIsolateRef or similar.

[IsolateRef previousIsolateRef]) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

in general avoid optional positional parameters. they just lead to bugs when they are sometimes used and sometimes not used. In general prefer optional named parameters as they provide more clarity about the intent of the parameters for cases where optional parameters are needed. not sure if this is in the style guide but is implicitly the case for flutter.

if (previousIsolateRef != null &&
previousIsolateRef != _isolateManager.mainIsolate.value) {
// Isolate has changed again.
return;
}

if (isolate.extensionRPCs != null) {
if (await connectedApp.isFlutterApp) {
if (isolateRef != _mainIsolate.value) {
if (previousIsolateRef != null &&
previousIsolateRef != _isolateManager.mainIsolate.value) {
// Isolate has changed again.
return;
}
Expand All @@ -935,7 +943,7 @@ class ServiceExtensionManager extends Disposer {
}

Future<void> _maybeCheckForFirstFlutterFrame() async {
final _lastMainIsolate = _mainIsolate.value;
final _lastMainIsolate = _isolateManager.mainIsolate.value;
if (_checkForFirstFrameStarted ||
_firstFrameEventReceived ||
_lastMainIsolate == null) return;
Expand All @@ -948,7 +956,7 @@ class ServiceExtensionManager extends Disposer {
extensions.didSendFirstFrameEvent,
isolateId: _lastMainIsolate.id,
);
if (_lastMainIsolate != _mainIsolate.value) {
if (_lastMainIsolate != _isolateManager.mainIsolate.value) {
// The active isolate has changed since we started querying the first
// frame.
return;
Expand Down Expand Up @@ -995,7 +1003,7 @@ class ServiceExtensionManager extends Disposer {
}

Future<void> _restoreExtensionFromDevice(String name) async {
final isolateRef = _mainIsolate.value;
final isolateRef = _isolateManager.mainIsolate.value;
if (isolateRef == null) return;

if (!extensions.serviceExtensionsAllowlist.containsKey(name)) {
Expand All @@ -1006,14 +1014,14 @@ class ServiceExtensionManager extends Disposer {

Future<void> restore() async {
// The restore request is obsolete if the isolate has changed.
if (isolateRef != _mainIsolate.value) return;
if (isolateRef != _isolateManager.mainIsolate.value) return;
try {
final response = await _service.callServiceExtension(
name,
isolateId: isolateRef.id,
);

if (isolateRef != _mainIsolate.value) return;
if (isolateRef != _isolateManager.mainIsolate.value) return;

switch (expectedValueType) {
case bool:
Expand Down Expand Up @@ -1044,10 +1052,10 @@ class ServiceExtensionManager extends Disposer {
}
}

if (isolateRef != _mainIsolate.value) return;
if (isolateRef != _isolateManager.mainIsolate.value) return;

final Isolate isolate = await _service.getIsolate(isolateRef.id);
if (isolateRef != _mainIsolate.value) return;
final Isolate isolate = await _isolateManager.getIsolateCached(isolateRef);
if (isolateRef != _isolateManager.mainIsolate.value) return;

// Do not try to restore Dart IO extensions for a paused isolate.
if (extensions.isDartIoExtension(name) &&
Expand Down Expand Up @@ -1075,9 +1083,9 @@ class ServiceExtensionManager extends Disposer {
return;
}

final mainIsolate = _mainIsolate.value;
final mainIsolate = _isolateManager.mainIsolate.value;
Future<void> callExtension() async {
if (_mainIsolate.value != mainIsolate) return;
if (_isolateManager.mainIsolate.value != mainIsolate) return;

assert(value != null);
if (value is bool) {
Expand Down Expand Up @@ -1131,8 +1139,8 @@ class ServiceExtensionManager extends Disposer {
}

if (mainIsolate == null) return;
final Isolate isolate = await _service.getIsolate(mainIsolate.id);
if (_mainIsolate.value != mainIsolate) return;
final Isolate isolate = await _isolateManager.getIsolateCached(mainIsolate);
if (_isolateManager.mainIsolate.value != mainIsolate) return;

// Do not try to call Dart IO extensions for a paused isolate.
if (extensions.isDartIoExtension(name) &&
Expand Down Expand Up @@ -1247,7 +1255,8 @@ class ServiceExtensionManager extends Disposer {
);
}

void vmServiceOpened(VmServiceWrapper service, ConnectedApp connectedApp) {
void vmServiceOpened(
VmServiceWrapper service, ConnectedApp connectedApp) async {
_checkForFirstFrameStarted = false;
cancel();
_connectedApp = connectedApp;
Expand All @@ -1258,11 +1267,14 @@ class ServiceExtensionManager extends Disposer {
hasServiceExtension(extensions.didSendFirstFrameEvent),
_maybeCheckForFirstFlutterFrame,
);
addAutoDisposeListener(_mainIsolate, _onMainIsolateChanged);
addAutoDisposeListener(_isolateManager.mainIsolate, _onMainIsolateChanged);
autoDispose(service.onDebugEvent.listen(_handleDebugEvent));
autoDispose(service.onIsolateEvent.listen(_handleIsolateEvent));
if (_mainIsolate.value != null) {
_onMainIsolateChanged();
if (_isolateManager.mainIsolate.value != null) {
_checkForFirstFrameStarted = false;
final isolate = await _isolateManager
.getIsolateCached(_isolateManager.mainIsolate.value);
await _registerIsolate(isolate);
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/devtools_app/lib/src/vm_service_wrapper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,10 @@ class VmServiceWrapper implements VmService {
}

@override
Future<Version> getVersion() =>
trackFuture('getVersion', _vmService.getVersion());
Future<Version> getVersion() async {
return _protocolVersion ??=
await trackFuture('getVersion', _vmService.getVersion());
}

Future<Version> getDartIOVersion(String isolateId) =>
trackFuture('_getDartIOVersion', _vmService.getDartIOVersion(isolateId));
Expand Down