Cache the results of getVersion and getIsolate

[elliette]

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.

[jacob314]

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.

[elliette]

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.

[jacob314]

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

[elliette]

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]

why is this needed?

[elliette]

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]

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

[elliette]

Now using the pre existing cache :)

[jacob314]

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.

[elliette]

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

[elliette]

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.

[jacob314]

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]

call this _registerMainIsolate for clarity.

[jacob314]

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

[jacob314]

lgtm once the two minor nit comments are addressed.