Language server startup time improvement #1299
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1907,4 +1907,4 @@ | |
| "publisherDisplayName": "Microsoft", | ||
| "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8" | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { createHash } from 'crypto'; | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import { ExtensionContext, Uri } from 'vscode'; | ||
| import { IApplicationShell } from '../common/application/types'; | ||
| import '../common/extensions'; | ||
| import { createDeferred } from '../common/helpers'; | ||
| import { IPlatformService } from '../common/platform/types'; | ||
| import { IPythonExecutionFactory, IPythonExecutionService } from '../common/process/types'; | ||
| import { IServiceContainer } from '../ioc/types'; | ||
|
|
||
| const DataVersion = 1; | ||
|
|
||
| export class InterpreterData { | ||
| constructor( | ||
| public readonly dataVersion: number, | ||
| // tslint:disable-next-line:no-shadowed-variable | ||
| public readonly path: string, | ||
| public readonly version: string, | ||
| public readonly prefix: string, | ||
| public readonly searchPaths: string, | ||
| public readonly hash: string | ||
| ) { } | ||
| } | ||
|
|
||
| export class InterpreterDataService { | ||
| constructor( | ||
| private readonly context: ExtensionContext, | ||
| private readonly serviceContainer: IServiceContainer) { } | ||
|
|
||
| public async getInterpreterData(resource?: Uri): Promise<InterpreterData | undefined> { | ||
| const executionFactory = this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory); | ||
| const execService = await executionFactory.create(resource); | ||
|
|
||
| const interpreterPath = await execService.getExecutablePath(); | ||
| if (interpreterPath.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| let interpreterData = this.context.globalState.get(interpreterPath) as InterpreterData; | ||
|
There was a problem hiding this comment. Please use another key for the cache storage. the python path would be too generic, easy to use this else where for other information related to the python path. Let use: const cacheKey = `InterpreterData-${interpreterPath}`; My plan is to do something very similar for interpreter information displayed in the status bar. though I might use this same class (slightly altered). Either way, lets use a non-generic key. There was a problem hiding this comment. Sure. I was not too convinced about raw path myself |
||
| let interpreterChanged = false; | ||
| if (interpreterData) { | ||
| // Check if interpreter executable changed | ||
| if (interpreterData.dataVersion !== DataVersion) { | ||
| interpreterChanged = true; | ||
| } else { | ||
| const currentHash = await this.getInterpreterHash(interpreterPath); | ||
| interpreterChanged = currentHash !== interpreterData.hash; | ||
| } | ||
| } | ||
|
|
||
| if (interpreterChanged || !interpreterData) { | ||
| interpreterData = await this.getInterpreterDataFromPython(execService, interpreterPath); | ||
| this.context.globalState.update(interpreterPath, interpreterData); | ||
| } else { | ||
| // Make sure we verify that search paths did not change. This must be done | ||
| // completely async so we don't delay Python language server startup. | ||
| this.verifySearchPathsAsync(interpreterData.searchPaths, interpreterPath, execService); | ||
| } | ||
| return interpreterData; | ||
| } | ||
|
|
||
| private async getInterpreterDataFromPython(execService: IPythonExecutionService, interpreterPath: string): Promise<InterpreterData> { | ||
| const result = await execService.exec(['-c', 'import sys; print(sys.version_info); print(sys.prefix)'], {}); | ||
| // 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:19:30) <<SOMETIMES NEW LINE HERE>> | ||
| // [MSC v.1500 32 bit (Intel)] | ||
| // C:\Python27 | ||
| if (!result.stdout) { | ||
| throw Error('Unable to determine Python interpreter version and system prefix.'); | ||
| } | ||
| const output = result.stdout.splitLines({ removeEmptyEntries: true, trim: true }); | ||
| if (!output || output.length < 2) { | ||
| throw Error('Unable to parse version and and system prefix from the Python interpreter output.'); | ||
| } | ||
| const majorMatches = output[0].match(/major=(\d*?),/); | ||
| const minorMatches = output[0].match(/minor=(\d*?),/); | ||
| if (!majorMatches || majorMatches.length < 2 || !minorMatches || minorMatches.length < 2) { | ||
| throw Error('Unable to parse interpreter version.'); | ||
| } | ||
| const prefix = output[output.length - 1]; | ||
| const hash = await this.getInterpreterHash(interpreterPath); | ||
| const searchPaths = await this.getSearchPaths(execService); | ||
| return new InterpreterData(DataVersion, interpreterPath, `${majorMatches[1]}.${minorMatches[1]}`, prefix, searchPaths, hash); | ||
| } | ||
|
|
||
| private getInterpreterHash(interpreterPath: string): Promise<string> { | ||
|
There was a problem hiding this comment. Just a thought, no change necessary: |
||
| const platform = this.serviceContainer.get<IPlatformService>(IPlatformService); | ||
| const pythonExecutable = path.join(path.dirname(interpreterPath), platform.isWindows ? 'python.exe' : 'python'); | ||
| // Hash mod time and creation time | ||
| const deferred = createDeferred<string>(); | ||
| fs.lstat(pythonExecutable, (err, stats) => { | ||
| if (err) { | ||
| deferred.resolve(''); | ||
| } else { | ||
| const actual = createHash('sha512').update(`${stats.ctimeMs}-${stats.mtimeMs}`).digest('hex'); | ||
| deferred.resolve(actual); | ||
| } | ||
| }); | ||
| return deferred.promise; | ||
| } | ||
|
|
||
| private async getSearchPaths(execService: IPythonExecutionService): Promise<string> { | ||
| const result = await execService.exec(['-c', 'import sys; print(sys.path);'], {}); | ||
| if (!result.stdout) { | ||
| throw Error('Unable to determine Python interpreter search paths.'); | ||
| } | ||
| // tslint:disable-next-line:no-unnecessary-local-variable | ||
| const paths = result.stdout.split(',') | ||
| .filter(p => this.isValidPath(p)) | ||
| .map(p => this.pathCleanup(p)); | ||
| return paths.join(';'); // PTVS uses ; on all platforms | ||
| } | ||
|
|
||
| private pathCleanup(s: string): string { | ||
| s = s.trim(); | ||
| if (s[0] === '\'') { | ||
| s = s.substr(1); | ||
| } | ||
| if (s[s.length - 1] === ']') { | ||
| s = s.substr(0, s.length - 1); | ||
| } | ||
| if (s[s.length - 1] === '\'') { | ||
| s = s.substr(0, s.length - 1); | ||
| } | ||
| return s; | ||
| } | ||
|
|
||
| private isValidPath(s: string): boolean { | ||
| return s.length > 0 && s[0] !== '['; | ||
| } | ||
|
|
||
| private verifySearchPathsAsync(currentPaths: string, interpreterPath: string, execService: IPythonExecutionService): void { | ||
|
There was a problem hiding this comment. OK. This was mostly for the calling code to show that this is actually async and is not awaited for. |
||
| this.getSearchPaths(execService) | ||
| .then(async paths => { | ||
| if (paths !== currentPaths) { | ||
| this.context.globalState.update(interpreterPath, undefined); | ||
| const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell); | ||
| await appShell.showWarningMessage('Search paths have changed for this Python interpreter. Please reload the extension to ensure that the IntelliSense works correctly.'); | ||
| } | ||
| }).ignoreErrors(); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, not sure how I missed this. Why are we returning a promise?
Shouldn't we return an error as part of the rejection. I.e. change this to
Promise<void>and errors will be captured incatch?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. Returning exception is questionable pattern.