Skip to content

Performance improvements to load times of extension #931

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 1 commit into from
Mar 2, 2018
Merged

Performance improvements to load times of extension #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/client/interpreter/display/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {
await Promise.all([
this.fileSystem.fileExistsAsync(pythonPath),
this.versionProvider.getVersion(pythonPath, defaultDisplayName),
this.getVirtualEnvironmentName(pythonPath)
this.getVirtualEnvironmentName(pythonPath).catch(() => '')
])
.then(([interpreterExists, displayName, virtualEnvName]) => {
const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : '';
Expand Down
14 changes: 2 additions & 12 deletions src/client/interpreter/interpreterService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import * as path from 'path';
import { ConfigurationTarget, Disposable, Event, EventEmitter, Uri } from 'vscode';
import { IDocumentManager, IWorkspaceService } from '../common/application/types';
import { PythonSettings } from '../common/configSettings';
import { IFileSystem } from '../common/platform/types';
import { IPythonExecutionFactory } from '../common/process/types';
import { IConfigurationService, IDisposableRegistry } from '../common/types';
import * as utils from '../common/utils';
Expand All @@ -21,13 +20,11 @@ export class InterpreterService implements Disposable, IInterpreterService {
private readonly locator: IInterpreterLocatorService;
private readonly pythonPathUpdaterService: IPythonPathUpdaterServiceManager;
private readonly helper: IInterpreterHelper;
private readonly fs: IFileSystem;
private readonly didChangeInterpreterEmitter = new EventEmitter<void>();

constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
this.locator = serviceContainer.get<IInterpreterLocatorService>(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE);
this.helper = serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
this.pythonPathUpdaterService = this.serviceContainer.get<IPythonPathUpdaterServiceManager>(IPythonPathUpdaterServiceManager);
}

Expand Down Expand Up @@ -134,21 +131,14 @@ export class InterpreterService implements Disposable, IInterpreterService {
return false;
}
if (activeWorkspace.configTarget === ConfigurationTarget.Workspace) {
return !await this.isPythonPathDefined(pythonPathInConfig!.workspaceValue);
return pythonPathInConfig!.workspaceValue === undefined || pythonPathInConfig!.workspaceValue === 'python';
Copy link
Author

Choose a reason for hiding this comment

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

@MikhailArkhipov
I reverted the change you made. I've added a catch in display/index.ts which would handle cases where interpreter is invalid (does not exist).
This fs check alone takes over 1.5 seconds on my Mac.

Copy link

Choose a reason for hiding this comment

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

Single file existence check? Wow.

}
if (activeWorkspace.configTarget === ConfigurationTarget.WorkspaceFolder) {
return !await this.isPythonPathDefined(pythonPathInConfig!.workspaceFolderValue);
return pythonPathInConfig!.workspaceFolderValue === undefined || pythonPathInConfig!.workspaceFolderValue === 'python';
}
return false;
}

private async isPythonPathDefined(pythonPath: string | undefined): Promise<boolean> {
if (pythonPath === undefined || pythonPath === 'python') {
return false;
}
return await this.fs.directoryExistsAsync(pythonPath);
}

private onConfigChanged = () => {
this.didChangeInterpreterEmitter.fire();
const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
Expand Down
55 changes: 12 additions & 43 deletions src/client/interpreter/locators/services/pipEnvService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,79 +5,48 @@ import { inject, injectable } from 'inversify';
import * as path from 'path';
import { Uri } from 'vscode';
import { IApplicationShell, IWorkspaceService } from '../../../common/application/types';
import { createDeferred, Deferred } from '../../../common/helpers';
import { IFileSystem } from '../../../common/platform/types';
import { IProcessService } from '../../../common/process/types';
import { getPythonExecutable } from '../../../debugger/Common/Utils';
import { IServiceContainer } from '../../../ioc/types';
import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts';
import { IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts';
import { CacheableLocatorService } from './cacheableLocatorService';

const execName = 'pipenv';
const CACHE_TIMEOUT = 2000;

@injectable()
export class PipEnvService implements IInterpreterLocatorService {
export class PipEnvService extends CacheableLocatorService {
Copy link
Author

Choose a reason for hiding this comment

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

Modified to inherit CacheableLocatorService.
Something I should have picked up during code review, my fault.

Copy link

Choose a reason for hiding this comment

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

Yeah I thought about cacheable too

private readonly versionService: IInterpreterVersionService;
private readonly process: IProcessService;
private readonly workspace: IWorkspaceService;
private readonly fs: IFileSystem;

private pendingPromises: Deferred<PythonInterpreter[]>[] = [];
private readonly cachedInterpreters = new Map<string, PythonInterpreter>();

constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
super('PipEnvService', serviceContainer);
this.versionService = this.serviceContainer.get<IInterpreterVersionService>(IInterpreterVersionService);
this.process = this.serviceContainer.get<IProcessService>(IProcessService);
this.workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
}

public getInterpreters(resource?: Uri): Promise<PythonInterpreter[]> {
// tslint:disable-next-line:no-empty
public dispose() { }
protected getInterpretersImplementation(resource?: Uri): Promise<PythonInterpreter[]> {
const pipenvCwd = this.getPipenvWorkingDirectory(resource);
if (!pipenvCwd) {
return Promise.resolve([]);
}

// Try cache first
const interpreter = this.cachedInterpreters[pipenvCwd];
if (interpreter) {
return Promise.resolve([interpreter]);
}
// We don't want multiple requests executing pipenv
const deferred = createDeferred<PythonInterpreter[]>();
this.pendingPromises.push(deferred);
if (this.pendingPromises.length === 1) {
// First call, start worker
this.getInterpreter(pipenvCwd)
.then(x => this.resolveDeferred(x ? [x] : []))
.catch(e => this.resolveDeferred([]));
}
return deferred.promise;
}

public dispose() {
this.resolveDeferred([]);
}

private resolveDeferred(result: PythonInterpreter[]) {
this.pendingPromises.forEach(p => p.resolve(result));
this.pendingPromises = [];
}

private async getInterpreter(pipenvCwd: string): Promise<PythonInterpreter | undefined> {
const interpreter = await this.getInterpreterFromPipenv(pipenvCwd);
if (interpreter) {
this.cachedInterpreters[pipenvCwd] = interpreter;
setTimeout(() => this.cachedInterpreters.clear(), CACHE_TIMEOUT);
}
return interpreter;
return this.getInterpreterFromPipenv(pipenvCwd)
.then(item => item ? [item] : [])
.catch(() => []);
}

private async getInterpreterFromPipenv(pipenvCwd: string): Promise<PythonInterpreter | undefined> {
const interpreterPath = await this.getInterpreterPathFromPipenv(pipenvCwd);
if (!interpreterPath) {
return;
}

const pythonExecutablePath = getPythonExecutable(interpreterPath);
const ver = await this.versionService.getVersion(pythonExecutablePath, '');
return {
Expand Down