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

Jump to bottom

Merged

elliette merged 13 commits into flutter:master from elliette:ipl-improvement

Sep 3, 2021

Member

elliette commented Aug 27, 2021 •

edited

Loading

Reduces the number of calls we make to getVersion and getIsolate in order to help improvement initial page load time.

Previously we were calling both multiple times:

...
{jsonrpc: 2.0, method: getVM, id: 6}
{jsonrpc: 2.0, method: getIsolate, id: 7, params: {isolateId: 1}}
{jsonrpc: 2.0, method: getVersion, id: 9, params: {}}
{jsonrpc: 2.0, method: getVersion, id: 10, params: {}}
{jsonrpc: 2.0, method: getFlagList, id: 11, params: {}}
{jsonrpc: 2.0, method: getVM, id: 12, params: {}}
{jsonrpc: 2.0, method: getIsolate, id: 13, params: {isolateId: 1}}
{jsonrpc: 2.0, method: getIsolate, id: 14, params: {isolateId: 1}}
{jsonrpc: 2.0, method: streamListen, id: 15, params: {streamId: GC}}
{jsonrpc: 2.0, method: streamListen, id: 16, params: {streamId: Timeline}}
{jsonrpc: 2.0, method: streamListen, id: 17, params: {streamId: VM}}
{jsonrpc: 2.0, method: streamListen, id: 18, params: {streamId: Service}}
{jsonrpc: 2.0, method: getIsolate, id: 19, params: {isolateId: 1}}
{jsonrpc: 2.0, method: getScripts, id: 20, params: {isolateId: 1}}
{jsonrpc: 2.0, method: getObject, id: 21, params: {isolateId: 1, objectId: 400}}
...

After this change, we call getVersion only once, and getIsolate twice. The second getIsolate call comes from the debugger_controller.

elliette added 3 commits

August 27, 2021 09:24


          Cache calls to getIsolate

1e357ca


          Cache getVersion requests as well

b50ca27


          Clean up

72b01af

elliette linked an issue

that may be closed by this pull request

DevTools is slow to startup #3169

Open

elliette requested review from jacob314 and kenzieschmoll

August 27, 2021 18:31

elliette changed the title ~~Cache the results of getVersion and getIsolates~~ Cache the results of getVersion and getIsolate

kenzieschmoll reviewed

View reviewed changes

packages/devtools_app/lib/src/vm_service_wrapper.dart Show resolved Hide resolved

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/vm_service_wrapper.dart Outdated

@@ @@ -370,8 +371,9 @@ class VmServiceWrapper implements VmService { @@
                 }
                 @override
-                Future<Isolate> getIsolate(String isolateId) {
-                  return trackFuture('getIsolate', _vmService.getIsolate(isolateId));
+                Future<Isolate> getIsolate(String isolateId) async {

Contributor

jacob314 Aug 27, 2021

We should use the cache in the IsolateManager everywhere possible rather than making this API cached as I think there are legitimate times we would need to get a new Isolate object. Just using the cache for all the cases on the startup path should be sufficient.

Member Author

elliette Aug 27, 2021

Hmm interesting! It looks like getIsolate is only being called once from the IsolateManager on startup. The other calls on start-up are coming from the ServiceExtensionManager and the DebuggerController . One option perhaps is to cache the values just for the duration of the vmServiceOpened method.

Contributor

jacob314 Aug 27, 2021

yeah feel free to flow the value through vmServiceOpened so none of the calls within it repeat fetching the isolate

Member Author

elliette Aug 30, 2021

Ok, I'm caching the value in IsolateManager and using it in vmServiceOpened (then clearing that value at the end of vmServiceOpened). However, it's possible I still might be overly caching - for example, as part of vmServiceOpened, the _onMainIsolateChanged method gets called, and I'm using the cached value there: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/service_manager.dart#L914. Wdyt?

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/vm_service_wrapper.dart Show resolved Hide resolved

elliette added 2 commits

August 30, 2021 09:53


          Use cached isolate in vmServiceOpened

da00d2f


          Clear isolates cache after start up

f2fdb7b

elliette marked this pull request as ready for review

September 1, 2021 22:13

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated Show resolved Hide resolved

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

@@ @@ -315,6 +317,9 @@ class ServiceConnectionManager { @@
                   }
                   _connectionAvailableController.add(service);
+                  // Clear the isolates cache:
+                  isolateManager.clearIsolateIdCache();

Contributor

jacob314 Sep 2, 2021

why is this needed?

Member Author

elliette Sep 3, 2021

I was mostly worried about clearing the cached isolate value because it was being used in onMainIsolateChanged (which was called on start up). I've now split out the registration logic from onMainIsolateChanged (because that seems to be why we were calling it on start up, to register the isolate). So now:

on start up, we call registerIsolate with the cached isolate
when the isolate changes, onMainIsolateChanged is called, which calls registerIsolate with the new isolate

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

                 VmServiceWrapper _service;
                 bool _checkForFirstFrameStarted = false;
                 final ValueListenable<IsolateRef> _mainIsolate;
+                final IsolateF _getIsolateCached;

Contributor

jacob314 Sep 2, 2021

why not just use the serviceManager isolate cache directly instead of passing in this function?

Member Author

elliette Sep 3, 2021

Now using the pre existing cache :)

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

@@ @@ -542,6 +547,8 @@ class IsolateManager extends Disposer { @@
                 ValueListenable<IsolateRef> get mainIsolate => _mainIsolate;
                 final _mainIsolate = ValueNotifier<IsolateRef>(null);
+                final _isolatesCache = <String, Isolate>{};

Contributor

jacob314 Sep 2, 2021

there is already an isolateCache. Use it directly for cases where a cache is suitable rather than creating another one. I believe it is in IsolateManager.

Member Author

elliette Sep 2, 2021

It looks like the existing cache is being used to cache the isolate states, not the isolate itself. It never actually sends a request for the isolate[1], which we need to do. Though I don't understand how the existing cache is getting the isolate without requesting it explicitly, so I might be mistaken!

https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/service_manager.dart#L734-L738

Member Author

elliette Sep 3, 2021

Realized why the pre-existing cache wasn't working is because I was using it in _loadIsolateState. Now we are using the cache everywhere except loadIsolateState, which is where we make the request for the isolate.

elliette added 7 commits

September 2, 2021 16:33


          use existing cache

9329e7e


          Merge branch 'master' into ipl-improvement

681b3a9


          Fetch the isolate in loadIsolateState

4e014fa


          delete cache

e832435


          actually delete the cache

ab6758e


          Clear the cache again

a559dc8


          register isolate

b126831

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

+                }
+                Future<void> _registerIsolate(Isolate isolate,
+                    [IsolateRef previousIsolateRef]) async {

Contributor

jacob314 Sep 3, 2021

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.

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

+                  final isolateRef = _isolateManager.mainIsolate.value;
+                  final Isolate isolate = await _isolateManager.getIsolateCached(isolateRef);
+                  await _registerIsolate(isolate, isolateRef);

Contributor

jacob314 Sep 3, 2021

call this _registerMainIsolate for clarity.

jacob314 reviewed

View reviewed changes

packages/devtools_app/lib/src/service_manager.dart Outdated

+                  await _registerIsolate(isolate, isolateRef);
+                }
+                Future<void> _registerIsolate(Isolate isolate,

Contributor

jacob314 Sep 3, 2021

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

jacob314 approved these changes

View reviewed changes

Contributor

jacob314 left a comment

lgtm once the two minor nit comments are addressed.


          Tweaks

e9d62ec

elliette merged commit ea94ff0 into flutter:master

elliette mentioned this pull request

Breakpoints are not hit for macos apps after hot restart or hot reload #3359

Closed

elliette deleted the ipl-improvement branch

December 24, 2021 00:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet