Few Perf Improvements for kernel startup & more logging #8317
Conversation
| @@ -94,8 +95,11 @@ export abstract class BaseInstaller { | |||
| .get<IPythonInstaller>(IPythonInstaller) | |||
| .install(product, resource, cancel, reInstallAndUpdate, installPipIfRequired); | |||
| } | |||
|
|
|||
| public async isInstalled(product: Product, resource?: InterpreterUri): Promise<boolean | undefined> { | |||
| @traceDecorators.verbose('Checking if product is installed') | |||
There was a problem hiding this comment.
Added some logging, these were useful
| @@ -70,14 +70,14 @@ export type TraceInfo = | |||
| export function tracing<T>(log: (t: TraceInfo) => void, run: () => T, logBeforeCall?: boolean): T { | |||
| const timer = new StopWatch(); | |||
| try { | |||
| if (logBeforeCall) { | |||
| log(undefined); | |||
| } | |||
There was a problem hiding this comment.
Bug fix, we were logging just after the method was called (which was incorrect)
| ) { | ||
| return (notebookMetadata as any).interpreter.hash; | ||
| } | ||
| } |
There was a problem hiding this comment.
An unnecessary change, but will be required later.
Had this as part of some other perf fixes that I'm not submitting just yet, basically moved the extraction of interpreterHash from notebook metadata into a function.
| @inject(IDisposableRegistry) disposables: IDisposableRegistry, | ||
| @inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalState: Memento | ||
| ) { | ||
| this.envVarsProvider.onDidEnvironmentVariablesChange( |
There was a problem hiding this comment.
Clear the cache if user updates their env variables
|
|
||
| /** | ||
| * This should return a WRITABLE place that jupyter will look for a kernel as documented | ||
| * here: https://jupyter-client.readthedocs.io/en/stable/kernels.html#kernel-specs | ||
| */ | ||
| @traceDecorators.verbose('Getting Jupyter KernelSpec Root Path') | ||
| public async getKernelSpecRootPath(): Promise<string | undefined> { |
There was a problem hiding this comment.
THis logging was very useful,
On windows this is very slow as tryGetRealPath is slow in windows.
Caching takes only 15-20ms and without that it takes around 400ms on windows, every time. this gets called at least 3 times, thus we shaved well over 1s with this simple change.
There was a problem hiding this comment.
Didn't you have some decorator way to cache results from a function?
There was a problem hiding this comment.
Oh yes, yo'ure right, let me check
There was a problem hiding this comment.
The existing decorator will not , because that uses a memory based cache store, i.e. when user reloads VS Code the cache is empty.
I'd need to create a decorator that uses the global memento? Would definitely make the code cleaner & improve readability.
Will do that next week (I.e. will not merge this PR).
| @@ -76,27 +104,41 @@ export class JupyterPaths { | |||
| * We need to look at the 'kernels' sub-directory and these paths are supposed to come first in the searching | |||
| * https://jupyter.readthedocs.io/en/latest/projects/jupyter-directories.html#envvar-JUPYTER_PATH | |||
| */ | |||
| @traceDecorators.verbose('Get Jupyter Paths') | |||
| private async getJupyterPathPaths(cancelToken?: CancellationToken): Promise<string[]> { | |||
There was a problem hiding this comment.
The logging and caching was very useful, see comments earlier.
There was a problem hiding this comment.
Same comment on this one. I swear you had something that would cache results for a function call into a memento.
There was a problem hiding this comment.
Yes now I remember, we had code to cache in memento, I think its in the Python extension, & it may have been removed from this repo as it wasn't used.
| @@ -391,7 +391,7 @@ export class NotebookControllerManager implements INotebookControllerManager, IE | |||
|
|
|||
| // Prep so that we can track the selected controller for this document | |||
| traceInfoIfCI(`Clear controller mapping for ${getDisplayPath(document.uri)}`); | |||
| const loadControllersPromise = this.loadNotebookControllers(); | |||
| void this.loadNotebookControllers(); | |||
There was a problem hiding this comment.
Its ok if the controllers haven't loaded,
We can create the controllers as required.
After all, that's what we do, we get controller information and create the controller.
If we call that again its not re-created.
This way, we don't have to wait for all 10-20 or 50 controllers to get loaded.
I.e. we can create controllers one by one based on the information available.
Result, we can now run button will be available even earlier.
Got more changes that will imporve this scenario & make this change all the more useful (separate PR).
| ); | ||
| // Possible the kernel discovery hasn't completed yet. | ||
| if (preferredConnection) { | ||
| this.createNotebookControllers([preferredConnection]); |
There was a problem hiding this comment.
If not available, then just create it.
| @@ -34,7 +33,7 @@ export class PythonKernelLauncherDaemon implements IDisposable { | |||
| @inject(KernelEnvironmentVariablesService) | |||
| private readonly kernelEnvVarsService: KernelEnvironmentVariablesService | |||
| ) {} | |||
| @traceDecorators.verbose('Launching kernel daemon', TraceOptions.BeforeCall) | |||
| @traceDecorators.verbose('Launching kernel daemon') | |||
There was a problem hiding this comment.
TraceOptions.BeforeCall is the default, hence removed this in a few places
Codecov Report
@@ Coverage Diff @@
## main #8317 +/- ##
=====================================
Coverage 72% 72%
=====================================
Files 372 372
Lines 23232 23259 +27
Branches 3553 3558 +5
=====================================
+ Hits 16784 16817 +33
+ Misses 4998 4996 -2
+ Partials 1450 1446 -4
|
Part of #7849