diff --git a/news/2 Fixes/10971.md b/news/2 Fixes/10971.md new file mode 100644 index 000000000000..41f9018e5807 --- /dev/null +++ b/news/2 Fixes/10971.md @@ -0,0 +1 @@ +Fix perf problems after running the interactive window for an extended period of time. \ No newline at end of file diff --git a/src/client/datascience/cellFactory.ts b/src/client/datascience/cellFactory.ts index e127ac208191..53459f382e98 100644 --- a/src/client/datascience/cellFactory.ts +++ b/src/client/datascience/cellFactory.ts @@ -110,7 +110,7 @@ export function hasCells(document: TextDocument, settings?: IDataScienceSettings } // CellRange is used as the basis for creating new ICells. We only use it in this file. -interface ICellRange { +export interface ICellRange { range: Range; title: string; cell_type: string; diff --git a/src/client/datascience/editor-integration/cellhashLogger.ts b/src/client/datascience/editor-integration/cellhashLogger.ts deleted file mode 100644 index 4b21ba112413..000000000000 --- a/src/client/datascience/editor-integration/cellhashLogger.ts +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -'use strict'; -import { inject, injectable } from 'inversify'; - -import { traceError } from '../../common/logger'; -import { noop } from '../../common/utils/misc'; -import { Identifiers } from '../constants'; -import { ICell, ICellHashLogger, ICellHashProvider } from '../types'; -import { CellHashProvider } from './cellhashprovider'; - -// This class provides hashes for debugging jupyter cells. Call getHashes just before starting debugging to compute all of the -// hashes for cells. -@injectable() -export class CellHashLogger implements ICellHashLogger { - constructor(@inject(ICellHashProvider) private provider: ICellHashProvider) {} - - public async preExecute(cell: ICell, silent: boolean): Promise { - const providerObj: CellHashProvider = this.provider as CellHashProvider; - - try { - if (!silent) { - // Don't log empty cells - const stripped = providerObj.extractExecutableLines(cell); - if (stripped.length > 0 && stripped.find((s) => s.trim().length > 0)) { - // When the user adds new code, we know the execution count is increasing - providerObj.incExecutionCount(); - - // Skip hash on unknown file though - if (cell.file !== Identifiers.EmptyFileName) { - await providerObj.addCellHash(cell, providerObj.getExecutionCount()); - } - } - } - } catch (exc) { - // Don't let exceptions in a preExecute mess up normal operation - traceError(exc); - } - } - - public async postExecute(_cell: ICell, _silent: boolean): Promise { - noop(); - } - - public getCellHashProvider(): ICellHashProvider { - return this.provider; - } -} diff --git a/src/client/datascience/editor-integration/cellhashprovider.ts b/src/client/datascience/editor-integration/cellhashprovider.ts index d75a66619a65..bb513779fec8 100644 --- a/src/client/datascience/editor-integration/cellhashprovider.ts +++ b/src/client/datascience/editor-integration/cellhashprovider.ts @@ -14,14 +14,13 @@ import { noop } from '../../common/utils/misc'; import { getCellResource } from '../cellFactory'; import { CellMatcher } from '../cellMatcher'; import { Identifiers } from '../constants'; -import { InteractiveWindowMessages, SysInfoReason } from '../interactive-common/interactiveWindowTypes'; import { ICell, ICellHash, ICellHashListener, ICellHashProvider, IFileHashes, - IInteractiveWindowListener + INotebookExecutionLogger } from '../types'; interface IRangedCellHash extends ICellHash { @@ -35,7 +34,7 @@ interface IRangedCellHash extends ICellHash { // This class provides hashes for debugging jupyter cells. Call getHashes just before starting debugging to compute all of the // hashes for cells. @injectable() -export class CellHashProvider implements ICellHashProvider, IInteractiveWindowListener { +export class CellHashProvider implements ICellHashProvider, INotebookExecutionLogger { // tslint:disable-next-line: no-any private postEmitter: EventEmitter<{ message: string; payload: any }> = new EventEmitter<{ message: string; @@ -71,25 +70,6 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi return this.postEmitter.event; } - // tslint:disable-next-line: no-any - public onMessage(message: string, payload?: any): void { - switch (message) { - case InteractiveWindowMessages.AddedSysInfo: - if (payload && payload.type) { - const reason = payload.type as SysInfoReason; - if (reason !== SysInfoReason.Interrupt) { - this.hashes.clear(); - this.executionCount = 0; - this.updateEventEmitter.fire(); - } - } - break; - - default: - break; - } - } - public getHashes(): IFileHashes[] { return [...this.hashes.entries()] .map((e) => { @@ -101,6 +81,12 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi .filter((e) => e.hashes.length > 0); } + public onKernelRestarted() { + this.hashes.clear(); + this.executionCount = 0; + this.updateEventEmitter.fire(); + } + public async preExecute(cell: ICell, silent: boolean): Promise { try { if (!silent) { diff --git a/src/client/datascience/editor-integration/codeLensFactory.ts b/src/client/datascience/editor-integration/codeLensFactory.ts index 1e3bed9f0bcf..ec651d70d5b2 100644 --- a/src/client/datascience/editor-integration/codeLensFactory.ts +++ b/src/client/datascience/editor-integration/codeLensFactory.ts @@ -4,26 +4,41 @@ import { inject, injectable } from 'inversify'; import { CodeLens, Command, Event, EventEmitter, Range, TextDocument, Uri } from 'vscode'; +import { IDocumentManager } from '../../common/application/types'; import { traceWarning } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; import { IConfigurationService, Resource } from '../../common/types'; import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; -import { generateCellRangesFromDocument } from '../cellFactory'; -import { CodeLensCommands, Commands } from '../constants'; -import { InteractiveWindowMessages } from '../interactive-common/interactiveWindowTypes'; +import { generateCellRangesFromDocument, ICellRange } from '../cellFactory'; +import { CodeLensCommands, Commands, Identifiers } from '../constants'; +import { + INotebookIdentity, + InteractiveWindowMessages, + SysInfoReason +} from '../interactive-common/interactiveWindowTypes'; import { ICell, - ICellHashLogger, ICellHashProvider, ICodeLensFactory, IFileHashes, IInteractiveWindowListener, INotebook, - INotebookExecutionLogger, INotebookProvider } from '../types'; +type CodeLensCacheData = { + cachedDocumentVersion: number | undefined; + cachedExecutionCount: number | undefined; + documentLenses: CodeLens[]; + cellRanges: ICellRange[]; + gotoCellLens: CodeLens[]; +}; + +/** + * This class is a singleton that generates code lenses for any document the user opens. It listens + * to cells being execute so it can add 'goto' lenses on cells that have already been run. + */ @injectable() export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowListener { private updateEvent: EventEmitter = new EventEmitter(); @@ -33,14 +48,21 @@ export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowList // tslint:disable-next-line: no-any payload: any; }>(); - private cellExecutionCounts: Map = new Map(); + private cellExecutionCounts = new Map(); + private documentExecutionCounts = new Map(); private hashProvider: ICellHashProvider | undefined; + private interactiveIdentity: Uri | undefined; // Once we have more than one interactive window, this logic won't work anymore + private codeLensCache = new Map(); constructor( @inject(IConfigurationService) private configService: IConfigurationService, @inject(INotebookProvider) private notebookProvider: INotebookProvider, - @inject(IFileSystem) private fileSystem: IFileSystem - ) {} + @inject(IFileSystem) private fileSystem: IFileSystem, + @inject(IDocumentManager) private documentManager: IDocumentManager + ) { + this.documentManager.onDidCloseTextDocument(this.onClosedDocument.bind(this)); + this.configService.getSettings(undefined).onDidChange(this.onChangedSettings.bind(this)); + } public dispose(): void { noop(); @@ -54,16 +76,47 @@ export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowList // tslint:disable-next-line: no-any public onMessage(message: string, payload?: any) { switch (message) { + case InteractiveWindowMessages.NotebookIdentity: + this.setIdentity(payload); + break; + + case InteractiveWindowMessages.NotebookClose: + if (payload.resource.toString() === this.interactiveIdentity?.toString()) { + this.interactiveIdentity = undefined; + this.hashProvider = undefined; + this.documentExecutionCounts.clear(); + + // Clear out any goto cell code lenses. + this.updateEvent.fire(); + } + break; case InteractiveWindowMessages.NotebookExecutionActivated: - this.initCellHashProvider(payload).ignoreErrors(); + this.initCellHashProvider(); + break; + + case InteractiveWindowMessages.AddedSysInfo: + if (payload && payload.type) { + const reason = payload.type as SysInfoReason; + if (reason !== SysInfoReason.Interrupt) { + this.documentExecutionCounts.clear(); + // Clear out any goto cell code lenses. + this.updateEvent.fire(); + } + } break; case InteractiveWindowMessages.FinishCell: const cell = payload as ICell; if (cell && cell.data && cell.data.execution_count) { - this.cellExecutionCounts.set(cell.id, cell.data.execution_count.toString()); + if (cell.file && cell.file !== Identifiers.EmptyFileName) { + this.cellExecutionCounts.set(cell.id, cell.data.execution_count.toString()); + this.documentExecutionCounts.set( + cell.file.toLocaleLowerCase(), + parseInt(cell.data.execution_count.toString(), 10) + ); + this.updateEvent.fire(); + } } - this.updateEvent.fire(); break; default: @@ -71,67 +124,128 @@ export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowList } } - public hashesUpdated(): void { - this.updateEvent.fire(); - } - public get updateRequired(): Event { return this.updateEvent.event; } public createCodeLenses(document: TextDocument): CodeLens[] { - const ranges = generateCellRangesFromDocument( - document, - this.configService.getSettings(document.uri).datascience - ); - const commands = this.enumerateCommands(document.uri); - const hashes = this.configService.getSettings(document.uri).datascience.addGotoCodeLenses - ? this.hashProvider - ? this.hashProvider.getHashes() - : [] - : []; - const codeLenses: CodeLens[] = []; - let firstCell = true; - - ranges.forEach((range) => { - commands.forEach((c) => { - const codeLens = this.createCodeLens(document, range, c, firstCell); + // See if we have a cached version of the code lenses for this document + const key = document.fileName.toLocaleLowerCase(); + let cache = this.codeLensCache.get(key); + let needUpdate = false; + + // If we don't have one, generate one + if (!cache) { + cache = { + cachedDocumentVersion: undefined, + cachedExecutionCount: undefined, + documentLenses: [], + cellRanges: [], + gotoCellLens: [] + }; + needUpdate = true; + this.codeLensCache.set(key, cache); + } + + // If the document version doesn't match, our cell ranges are out of date + if (cache.cachedDocumentVersion !== document.version) { + cache.cellRanges = generateCellRangesFromDocument( + document, + this.configService.getSettings(document.uri).datascience + ); + + // Because we have all new ranges, we need to recompute ALL of our code lenses. + cache.documentLenses = []; + cache.gotoCellLens = []; + cache.cachedDocumentVersion = document.version; + needUpdate = true; + } + + // If the document execution count doesn't match, then our goto cell lens is out of date + if (cache.cachedExecutionCount !== this.documentExecutionCounts.get(key)) { + cache.gotoCellLens = []; + cache.cachedExecutionCount = this.documentExecutionCounts.get(key); + needUpdate = true; + } + + // Generate our code lenses if necessary + if (cache.documentLenses.length === 0 && needUpdate && cache.cellRanges.length) { + // Enumerate the possible commands for the document based code lenses + const commands = needUpdate ? this.enumerateCommands(document.uri) : []; + + // Then iterate over all of the cell ranges and generate code lenses for each possible + // commands + let firstCell = true; + cache.cellRanges.forEach((r) => { + commands.forEach((c) => { + const codeLens = this.createCodeLens(document, r, c, firstCell); + if (codeLens) { + cache?.documentLenses.push(codeLens); // NOSONAR + } + }); + firstCell = false; + }); + } + + // Generate the goto cell lenses if necessary + if ( + needUpdate && + cache.gotoCellLens.length === 0 && + this.hashProvider && + cache.cellRanges.length && + this.configService.getSettings(document.uri).datascience.addGotoCodeLenses + ) { + const hashes = this.hashProvider.getHashes(); + cache.cellRanges.forEach((r) => { + const codeLens = this.createExecutionLens(document, r.range, hashes); if (codeLens) { - codeLenses.push(codeLens); + cache?.gotoCellLens.push(codeLens); // NOSONAR } }); - this.addExecutionCount(codeLenses, document, range.range, hashes); - firstCell = false; - }); + } - return codeLenses; + return [...cache.documentLenses, ...cache.gotoCellLens]; } - private async initCellHashProvider(notebookUri: string) { - const nbUri: Uri = Uri.parse(notebookUri); - if (!nbUri) { - return; + private setIdentity(identity: INotebookIdentity) { + if (identity.type === 'interactive') { + this.interactiveIdentity = identity.resource; } + } - // First get the active server - const nb = await this.notebookProvider.getOrCreateNotebook({ identity: nbUri, getOnly: true }); + private initCellHashProvider() { + // Try getting our notebook. This should fail if + // the user hasn't opened the interactive window yet. + this.getInteractiveWindowNotebook() + .then((nb) => { + if (nb) { + // From the notebook, find the logger that is the cell hash provider + // tslint:disable-next-line: no-any + this.hashProvider = (nb.getLoggers().find((l) => (l as any).getHashes) as any) as ICellHashProvider; + } + }) + .ignoreErrors(); + } - // If we have an executing notebook, get its cell hash provider service. - if (nb) { - this.hashProvider = this.getCellHashProvider(nb); - if (this.hashProvider) { - this.hashProvider.updated(this.hashesUpdated.bind(this)); - } - } + private async getInteractiveWindowNotebook(): Promise { + return this.interactiveIdentity + ? this.notebookProvider.getOrCreateNotebook({ identity: this.interactiveIdentity, getOnly: true }) + : undefined; } - private getCellHashProvider(nb: INotebook): ICellHashProvider | undefined { - const cellHashLogger = ( - nb.getLoggers().find((logger: INotebookExecutionLogger) => (logger).getCellHashProvider) - ); - if (cellHashLogger) { - return cellHashLogger.getCellHashProvider(); - } + private onClosedDocument(doc: TextDocument) { + this.codeLensCache.delete(doc.fileName.toLocaleLowerCase()); + + // Don't delete the document execution count, we need to keep track + // of it past the closing of a doc if the notebook or interactive window is still open. + } + + private onChangedSettings() { + // When config settings change, refresh our code lenses. + this.codeLensCache.clear(); + + // Force an update so that code lenses are recomputed now and not during execution. + this.updateEvent.fire(); } private enumerateCommands(resource: Resource): string[] { @@ -282,7 +396,7 @@ export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowList return undefined; } - private addExecutionCount(codeLens: CodeLens[], document: TextDocument, range: Range, hashes: IFileHashes[]) { + private createExecutionLens(document: TextDocument, range: Range, hashes: IFileHashes[]) { const list = hashes.find((h) => this.fileSystem.arePathsSame(h.file, document.fileName)); if (list) { // Match just the start of the range. Should be - 2 (1 for 1 based numbers and 1 for skipping the comment at the top) @@ -290,15 +404,13 @@ export class CodeLensFactory implements ICodeLensFactory, IInteractiveWindowList if (rangeMatches && rangeMatches.length) { const rangeMatch = rangeMatches[rangeMatches.length - 1]; if (this.cellExecutionCounts.has(rangeMatch.id)) { - codeLens.push( - this.generateCodeLens( - range, - Commands.ScrollToCell, - localize.DataScience.scrollToCellTitleFormatMessage().format( - this.cellExecutionCounts.get(rangeMatch.id)! - ), - [document.fileName, rangeMatch.id] - ) + return this.generateCodeLens( + range, + Commands.ScrollToCell, + localize.DataScience.scrollToCellTitleFormatMessage().format( + this.cellExecutionCounts.get(rangeMatch.id)! + ), + [document.fileName, rangeMatch.id] ); } } diff --git a/src/client/datascience/editor-integration/codewatcher.ts b/src/client/datascience/editor-integration/codewatcher.ts index 5c7846c3896f..7cf4bfe30067 100644 --- a/src/client/datascience/editor-integration/codewatcher.ts +++ b/src/client/datascience/editor-integration/codewatcher.ts @@ -16,7 +16,7 @@ import { import { IDocumentManager } from '../../common/application/types'; import { IFileSystem } from '../../common/platform/types'; -import { IConfigurationService, IDataScienceSettings, Resource } from '../../common/types'; +import { IConfigurationService, IDataScienceSettings, IDisposable, Resource } from '../../common/types'; import * as localize from '../../common/utils/localize'; import { StopWatch } from '../../common/utils/stopWatch'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; @@ -34,6 +34,8 @@ export class CodeWatcher implements ICodeWatcher { private codeLenses: CodeLens[] = []; private cachedSettings: IDataScienceSettings | undefined; private codeLensUpdatedEvent: EventEmitter = new EventEmitter(); + private updateRequiredDisposable: IDisposable | undefined; + private closeDocumentDisposable: IDisposable | undefined; constructor( @inject(IInteractiveWindowProvider) private interactiveWindowProvider: IInteractiveWindowProvider, @@ -59,7 +61,10 @@ export class CodeWatcher implements ICodeWatcher { this.codeLenses = this.codeLensFactory.createCodeLenses(document); // Listen for changes - this.codeLensFactory.updateRequired(this.onCodeLensFactoryUpdated.bind(this)); + this.updateRequiredDisposable = this.codeLensFactory.updateRequired(this.onCodeLensFactoryUpdated.bind(this)); + + // Make sure to stop listening for changes when this document closes. + this.closeDocumentDisposable = this.documentManager.onDidCloseTextDocument(this.onDocumentClosed.bind(this)); } public get codeLensUpdated(): Event { @@ -368,6 +373,14 @@ export class CodeWatcher implements ICodeWatcher { this.codeLensUpdatedEvent.fire(); } + private onDocumentClosed(doc: TextDocument): void { + if (this.document && this.fileSystem.arePathsSame(doc.fileName, this.document.fileName)) { + this.codeLensUpdatedEvent.dispose(); + this.closeDocumentDisposable?.dispose(); // NOSONAR + this.updateRequiredDisposable?.dispose(); // NOSONAR + } + } + private async addCode( code: string, file: string, diff --git a/src/client/datascience/gather/gatherLogger.ts b/src/client/datascience/gather/gatherLogger.ts index ffa85f006547..f3117102cf36 100644 --- a/src/client/datascience/gather/gatherLogger.ts +++ b/src/client/datascience/gather/gatherLogger.ts @@ -14,6 +14,10 @@ export class GatherLogger implements IGatherLogger { @inject(IConfigurationService) private configService: IConfigurationService ) {} + public onKernelRestarted() { + noop(); + } + public async preExecute(_vscCell: IVscCell, _silent: boolean): Promise { // This function is just implemented here for compliance with the INotebookExecutionLogger interface noop(); diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index bb5189757f64..22c4834732ef 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -706,8 +706,8 @@ export class IntellisenseProvider implements IInteractiveWindowListener { } private setIdentity(identity: INotebookIdentity) { - this.notebookIdentity = Uri.parse(identity.resource); - this.potentialResource = Uri.parse(identity.resource); + this.notebookIdentity = identity.resource; + this.potentialResource = identity.resource; } private async getNotebook(): Promise { diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts index 86d4f8babf1a..7112f7806f1d 100644 --- a/src/client/datascience/interactive-common/interactiveBase.ts +++ b/src/client/datascience/interactive-common/interactiveBase.ts @@ -55,6 +55,7 @@ import { ICopyCode, IGotoCode, IInteractiveWindowMapping, + INotebookIdentity, InteractiveWindowMessages, IReExecuteCells, IRemoteAddCode, @@ -187,10 +188,8 @@ export abstract class InteractiveBase extends WebViewHost { this.getNotebookIdentity() - .then((uri) => - this.listeners.forEach((l) => - l.onMessage(InteractiveWindowMessages.NotebookIdentity, { resource: uri.toString() }) - ) + .then((identity) => + this.listeners.forEach((l) => l.onMessage(InteractiveWindowMessages.NotebookIdentity, identity)) ) .ignoreErrors(); }, 0); @@ -340,7 +339,12 @@ export abstract class InteractiveBase extends WebViewHost l.dispose()); + this.getNotebookIdentity() + .then((r) => { + // Tell listeners we're closing. They can decide if they should dispose themselves or not. + this.listeners.forEach((l) => l.onMessage(InteractiveWindowMessages.NotebookClose, r)); + }) + .ignoreErrors(); this.updateContexts(undefined); } @@ -488,7 +492,7 @@ export abstract class InteractiveBase extends WebViewHost; + protected abstract getNotebookIdentity(): Promise; protected abstract closeBecauseOfFailure(exc: Error): Promise; @@ -1034,14 +1038,14 @@ export abstract class InteractiveBase extends WebViewHost { let notebook: INotebook | undefined; while (!notebook) { - const [resource, uri, metadata] = await Promise.all([ + const [resource, identity, metadata] = await Promise.all([ this.getOwningResource(), this.getNotebookIdentity(), this.getNotebookMetadata() ]); try { - notebook = uri - ? await this.notebookProvider.getOrCreateNotebook({ identity: uri, metadata }) + notebook = identity + ? await this.notebookProvider.getOrCreateNotebook({ identity: identity.resource, metadata }) : undefined; } catch (e) { // If we get an invalid kernel error, make sure to ask the user to switch diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index ff4b76308f05..49c1dc4c0ee7 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -91,6 +91,7 @@ export enum InteractiveWindowMessages { ScrollToCell = 'scroll_to_cell', ReExecuteCells = 'reexecute_cells', NotebookIdentity = 'identity', + NotebookClose = 'close', NotebookDirty = 'dirty', NotebookClean = 'clean', SaveAll = 'save_all', @@ -327,7 +328,8 @@ export interface IScrollToCell { } export interface INotebookIdentity { - resource: string; + resource: Uri; + type: 'interactive' | 'native'; } export interface ISaveAll { @@ -576,6 +578,7 @@ export class IInteractiveWindowMapping { public [InteractiveWindowMessages.ScrollToCell]: IScrollToCell; public [InteractiveWindowMessages.ReExecuteCells]: IReExecuteCells; public [InteractiveWindowMessages.NotebookIdentity]: INotebookIdentity; + public [InteractiveWindowMessages.NotebookClose]: INotebookIdentity; public [InteractiveWindowMessages.NotebookDirty]: never | undefined; public [InteractiveWindowMessages.NotebookClean]: never | undefined; public [InteractiveWindowMessages.SaveAll]: ISaveAll; diff --git a/src/client/datascience/interactive-common/synchronization.ts b/src/client/datascience/interactive-common/synchronization.ts index 1edbeed104d6..d26cfbeab21a 100644 --- a/src/client/datascience/interactive-common/synchronization.ts +++ b/src/client/datascience/interactive-common/synchronization.ts @@ -130,6 +130,7 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.NotebookDirty]: MessageType.other, [InteractiveWindowMessages.NotebookExecutionActivated]: MessageType.other, [InteractiveWindowMessages.NotebookIdentity]: MessageType.other, + [InteractiveWindowMessages.NotebookClose]: MessageType.other, [InteractiveWindowMessages.NotebookRunAllCells]: MessageType.other, [InteractiveWindowMessages.NotebookRunSelectedCell]: MessageType.other, [InteractiveWindowMessages.OpenLink]: MessageType.other, diff --git a/src/client/datascience/interactive-ipynb/autoSaveService.ts b/src/client/datascience/interactive-ipynb/autoSaveService.ts index b5299e035262..a5ef4b51a39a 100644 --- a/src/client/datascience/interactive-ipynb/autoSaveService.ts +++ b/src/client/datascience/interactive-ipynb/autoSaveService.ts @@ -60,9 +60,10 @@ export class AutoSaveService implements IInteractiveWindowListener { public onMessage(message: string, payload?: any): void { if (message === InteractiveWindowMessages.NotebookIdentity) { - this.notebookUri = Uri.parse((payload as INotebookIdentity).resource); - } - if (message === InteractiveWindowMessages.LoadAllCellsComplete) { + this.notebookUri = (payload as INotebookIdentity).resource; + } else if (message === InteractiveWindowMessages.NotebookClose) { + this.dispose(); + } else if (message === InteractiveWindowMessages.LoadAllCellsComplete) { const notebook = this.getNotebook(); if (!notebook) { traceError( diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index 9d6f8fcadac2..fc104cc53f84 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -54,6 +54,7 @@ import { import { InteractiveBase } from '../interactive-common/interactiveBase'; import { INativeCommand, + INotebookIdentity, InteractiveWindowMessages, IReExecuteCells, ISubmitNewCell, @@ -333,9 +334,10 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { this.postMessage(InteractiveWindowMessages.NotebookAddCellBelow, { newCellId: uuid() }).ignoreErrors(); } - public getOwningResource(): Promise { + public async getOwningResource(): Promise { // Resource to use for loading and our identity are the same. - return this.getNotebookIdentity(); + const identity = await this.getNotebookIdentity(); + return identity.resource; } protected addSysInfo(reason: SysInfoReason): Promise { @@ -425,13 +427,16 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { } } - protected async getNotebookIdentity(): Promise { + protected async getNotebookIdentity(): Promise { if (this.loadedPromise) { await this.loadedPromise.promise; } // File should be set now - return this.file; + return { + resource: this.file, + type: 'native' + }; } protected async setLaunchingFile(_file: string): Promise { diff --git a/src/client/datascience/interactive-window/interactiveWindow.ts b/src/client/datascience/interactive-window/interactiveWindow.ts index 4286ad90a0ed..56b84e0bfafa 100644 --- a/src/client/datascience/interactive-window/interactiveWindow.ts +++ b/src/client/datascience/interactive-window/interactiveWindow.ts @@ -33,6 +33,7 @@ import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; import { EditorContexts, Identifiers, Telemetry } from '../constants'; import { InteractiveBase } from '../interactive-common/interactiveBase'; import { + INotebookIdentity, InteractiveWindowMessages, ISubmitNewCell, NotebookModelChange, @@ -311,9 +312,12 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi // Do nothing as this data isn't stored in our options. } - protected async getNotebookIdentity(): Promise { + protected async getNotebookIdentity(): Promise { // Always the same identity (for now) - return Uri.parse(Identifiers.InteractiveWindowIdentity); + return { + resource: Uri.parse(Identifiers.InteractiveWindowIdentity), + type: 'interactive' + }; } protected updateContexts(info: IInteractiveWindowInfo | undefined) { diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts index ff3a451ffc9a..db83fc71c6b0 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts @@ -174,6 +174,8 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal this.saveIdentity(payload).catch((ex) => traceError(`Failed to initialize ${(this as Object).constructor.name}`, ex) ); + } else if (message === InteractiveWindowMessages.NotebookClose) { + this.dispose(); } else if (message === InteractiveWindowMessages.ConvertUriForUseInWebViewResponse) { const response: undefined | { request: Uri; response: Uri } = payload; if (response && this.uriConversionPromises.get(response.request.toString())) { @@ -218,7 +220,7 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal } } private async saveIdentity(args: INotebookIdentity) { - this.notebookIdentity = Uri.parse(args.resource); + this.notebookIdentity = args.resource; await this.initialize(); } diff --git a/src/client/datascience/ipywidgets/ipywidgetHandler.ts b/src/client/datascience/ipywidgets/ipywidgetHandler.ts index f3d259bb3b47..77cfca280c09 100644 --- a/src/client/datascience/ipywidgets/ipywidgetHandler.ts +++ b/src/client/datascience/ipywidgets/ipywidgetHandler.ts @@ -121,7 +121,7 @@ export class IPyWidgetHandler implements IInteractiveWindowListener { } private async saveIdentity(args: INotebookIdentity) { - this.notebookIdentity = Uri.parse(args.resource); + this.notebookIdentity = args.resource; const dispatcher = this.getIPyWidgetMessageDispatcher(); if (dispatcher) { diff --git a/src/client/datascience/jupyter/jupyterCellOutputMimeTypeTracker.ts b/src/client/datascience/jupyter/jupyterCellOutputMimeTypeTracker.ts index d8be8d2add24..98a597f3901b 100644 --- a/src/client/datascience/jupyter/jupyterCellOutputMimeTypeTracker.ts +++ b/src/client/datascience/jupyter/jupyterCellOutputMimeTypeTracker.ts @@ -20,6 +20,10 @@ export class CellOutputMimeTypeTracker implements IExtensionSingleActivationServ constructor(@inject(INotebookEditorProvider) private notebookEditorProvider: INotebookEditorProvider) { this.notebookEditorProvider.onDidOpenNotebookEditor((t) => this.onOpenedOrClosedNotebook(t)); } + + public onKernelRestarted() { + // Do nothing on restarted + } public async preExecute(_cell: ICell, _silent: boolean): Promise { // Do nothing on pre execute } diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index 963fe100dc28..b7032a9d8f0b 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -450,6 +450,10 @@ export class JupyterNotebookBase implements INotebook { traceInfo('restartKernel - initialSetup'); await this.initialize(); traceInfo('restartKernel - initialSetup completed'); + + // Tell our loggers + this.loggers.forEach((l) => l.onKernelRestarted()); + this.kernelRestarted.fire(); return; } diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index 730d30e91345..f3e0ed9da4cb 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -18,7 +18,6 @@ import { DataViewerProvider } from './data-viewing/dataViewerProvider'; import { DataScience } from './datascience'; import { DataScienceSurveyBannerLogger } from './dataScienceSurveyBanner'; import { DebugLocationTrackerFactory } from './debugLocationTrackerFactory'; -import { CellHashLogger } from './editor-integration/cellhashLogger'; import { CellHashProvider } from './editor-integration/cellhashprovider'; import { CodeLensFactory } from './editor-integration/codeLensFactory'; import { DataScienceCodeLensProvider } from './editor-integration/codelensprovider'; @@ -85,7 +84,6 @@ import { StatusProvider } from './statusProvider'; import { ThemeFinder } from './themeFinder'; import { ICellHashListener, - ICellHashLogger, ICellHashProvider, ICodeCssGenerator, ICodeLensFactory, @@ -131,8 +129,7 @@ export function registerTypes(serviceManager: IServiceManager) { const useCustomEditorApi = serviceManager.get(IApplicationEnvironment).packageJson.enableProposedApi; serviceManager.addSingletonInstance(UseCustomEditorApi, useCustomEditorApi); - serviceManager.add(ICellHashLogger, CellHashLogger, undefined, [INotebookExecutionLogger]); - serviceManager.add(ICellHashProvider, CellHashProvider); + serviceManager.add(ICellHashProvider, CellHashProvider, undefined, [INotebookExecutionLogger]); serviceManager.add(ICodeWatcher, CodeWatcher); serviceManager.addSingleton(IDataScienceErrorHandler, DataScienceErrorHandler); serviceManager.add(IDataViewer, DataViewer); diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index f3809a40c8e9..d8b2bf1c4150 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -188,6 +188,7 @@ export const INotebookExecutionLogger = Symbol('INotebookExecutionLogger'); export interface INotebookExecutionLogger { preExecute(cell: ICell, silent: boolean): Promise; postExecute(cell: ICell, silent: boolean): Promise; + onKernelRestarted(): void; } export const IGatherProvider = Symbol('IGatherProvider'); @@ -778,11 +779,6 @@ export interface ICellHashProvider { incExecutionCount(): void; } -export const ICellHashLogger = Symbol('ICellHashLogger'); -export interface ICellHashLogger extends INotebookExecutionLogger { - getCellHashProvider(): ICellHashProvider; -} - export interface IDebugLocation { fileName: string; lineNumber: number; diff --git a/src/client/telemetry/importTracker.ts b/src/client/telemetry/importTracker.ts index 85566d235c82..e7c85f8a2398 100644 --- a/src/client/telemetry/importTracker.ts +++ b/src/client/telemetry/importTracker.ts @@ -59,6 +59,9 @@ export class ImportTracker implements IExtensionSingleActivationService, INotebo this.notebookEditorProvider.onDidOpenNotebookEditor((t) => this.onOpenedOrClosedNotebook(t)); this.notebookEditorProvider.onDidCloseNotebookEditor((t) => this.onOpenedOrClosedNotebook(t)); } + public onKernelRestarted() { + // Do nothing on restarted + } public async preExecute(_cell: ICell, _silent: boolean): Promise { // Do nothing on pre execute } diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index a2bc8420844b..42e0fb2b4570 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -173,7 +173,6 @@ import { DataViewer } from '../../client/datascience/data-viewing/dataViewer'; import { DataViewerDependencyService } from '../../client/datascience/data-viewing/dataViewerDependencyService'; import { DataViewerProvider } from '../../client/datascience/data-viewing/dataViewerProvider'; import { DebugLocationTrackerFactory } from '../../client/datascience/debugLocationTrackerFactory'; -import { CellHashLogger } from '../../client/datascience/editor-integration/cellhashLogger'; import { CellHashProvider } from '../../client/datascience/editor-integration/cellhashprovider'; import { CodeLensFactory } from '../../client/datascience/editor-integration/codeLensFactory'; import { DataScienceCodeLensProvider } from '../../client/datascience/editor-integration/codelensprovider'; @@ -231,7 +230,6 @@ import { StatusProvider } from '../../client/datascience/statusProvider'; import { ThemeFinder } from '../../client/datascience/themeFinder'; import { ICellHashListener, - ICellHashLogger, ICellHashProvider, ICodeCssGenerator, ICodeLensFactory, @@ -714,8 +712,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer { } this.serviceManager.add(IProtocolParser, ProtocolParser); this.serviceManager.addSingleton(IDebugService, MockDebuggerService); - this.serviceManager.add(ICellHashProvider, CellHashProvider); - this.serviceManager.add(ICellHashLogger, CellHashLogger, undefined, [ + this.serviceManager.add(ICellHashProvider, CellHashProvider, undefined, [ INotebookExecutionLogger ]); this.serviceManager.add(IGatherProvider, GatherProvider); diff --git a/src/test/datascience/editor-integration/cellhashprovider.unit.test.ts b/src/test/datascience/editor-integration/cellhashprovider.unit.test.ts index 6b86569e4a13..5d5b29e8b459 100644 --- a/src/test/datascience/editor-integration/cellhashprovider.unit.test.ts +++ b/src/test/datascience/editor-integration/cellhashprovider.unit.test.ts @@ -8,12 +8,7 @@ import { Position, Range, Uri } from 'vscode'; import { IDebugService } from '../../../client/common/application/types'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, IDataScienceSettings, IPythonSettings } from '../../../client/common/types'; -import { CellHashLogger } from '../../../client/datascience/editor-integration/cellhashLogger'; import { CellHashProvider } from '../../../client/datascience/editor-integration/cellhashprovider'; -import { - InteractiveWindowMessages, - SysInfoReason -} from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { CellState, ICell, ICellHashListener, IFileHashes } from '../../../client/datascience/types'; import { MockDocumentManager } from '../mockDocumentManager'; @@ -28,7 +23,6 @@ class HashListener implements ICellHashListener { // tslint:disable-next-line: max-func-body-length suite('CellHashProvider Unit Tests', () => { let hashProvider: CellHashProvider; - let hashLogger: CellHashLogger; let documentManager: MockDocumentManager; let configurationService: TypeMoq.IMock; let dataScienceSettings: TypeMoq.IMock; @@ -55,7 +49,6 @@ suite('CellHashProvider Unit Tests', () => { fileSystem.object, [hashListener] ); - hashLogger = new CellHashLogger(hashProvider); }); function addSingleChange(file: string, range: Range, newText: string) { @@ -76,7 +69,7 @@ suite('CellHashProvider Unit Tests', () => { id: '1', state: CellState.init }; - return hashLogger.preExecute(cell, false); + return hashProvider.preExecute(cell, false); } test('Add a cell and edit it', async () => { @@ -534,7 +527,7 @@ suite('CellHashProvider Unit Tests', () => { assert.equal(hashes[0].hashes[0].executionCount, 1, 'Wrong execution count'); // Restart the kernel - hashProvider.onMessage(InteractiveWindowMessages.AddedSysInfo, { type: SysInfoReason.Restart }); + hashProvider.onKernelRestarted(); hashes = hashProvider.getHashes(); assert.equal(hashes.length, 0, 'Restart should have cleared'); diff --git a/src/test/datascience/editor-integration/codewatcher.unit.test.ts b/src/test/datascience/editor-integration/codewatcher.unit.test.ts index cf5ed1ec7837..ba85886ab208 100644 --- a/src/test/datascience/editor-integration/codewatcher.unit.test.ts +++ b/src/test/datascience/editor-integration/codewatcher.unit.test.ts @@ -8,7 +8,6 @@ import * as TypeMoq from 'typemoq'; import { CancellationTokenSource, CodeLens, Disposable, Range, Selection, TextEditor, Uri } from 'vscode'; import { ICommandManager, IDebugService, IDocumentManager } from '../../../client/common/application/types'; -import { PythonSettings } from '../../../client/common/configSettings'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService } from '../../../client/common/types'; import { Commands, EditorContexts } from '../../../client/datascience/constants'; @@ -26,6 +25,7 @@ import { import { IServiceContainer } from '../../../client/ioc/types'; import { ICodeExecutionHelper } from '../../../client/terminals/types'; import { MockAutoSelectionService } from '../../mocks/autoSelector'; +import { MockPythonSettings } from '../mockPythonSettings'; import { createDocument } from './helpers'; //tslint:disable:no-any @@ -47,11 +47,7 @@ suite('DataScience Code Watcher Unit Tests', () => { let debugService: TypeMoq.IMock; let debugLocationTracker: TypeMoq.IMock; const contexts: Map = new Map(); - const pythonSettings = new (class extends PythonSettings { - public fireChangeEvent() { - this.changed.fire(); - } - })(undefined, new MockAutoSelectionService()); + const pythonSettings = new MockPythonSettings(undefined, new MockAutoSelectionService()); const disposables: Disposable[] = []; setup(() => { @@ -105,7 +101,15 @@ suite('DataScience Code Watcher Unit Tests', () => { // Setup the file system fileSystem.setup((f) => f.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())).returns(() => true); - const codeLensFactory = new CodeLensFactory(configService.object, notebookProvider.object, fileSystem.object); + // Setup config service + configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings); + + const codeLensFactory = new CodeLensFactory( + configService.object, + notebookProvider.object, + fileSystem.object, + documentManager.object + ); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(ICodeWatcher))) .returns( @@ -132,9 +136,6 @@ suite('DataScience Code Watcher Unit Tests', () => { // Setup our active text editor documentManager.setup((dm) => dm.activeTextEditor).returns(() => textEditor.object); - // Setup config service - configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings); - commandManager .setup((c) => c.executeCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns((c, n, v) => { @@ -144,7 +145,12 @@ suite('DataScience Code Watcher Unit Tests', () => { return Promise.resolve(); }); - const codeLens = new CodeLensFactory(configService.object, notebookProvider.object, fileSystem.object); + const codeLens = new CodeLensFactory( + configService.object, + notebookProvider.object, + fileSystem.object, + documentManager.object + ); codeWatcher = new CodeWatcher( interactiveWindowProvider.object, @@ -895,6 +901,7 @@ testing2`; // Change settings pythonSettings.datascience.codeRegularExpression = '#%%%.*dude'; + pythonSettings.fireChangeEvent(); result = codeLensProvider.provideCodeLenses(document.object, tokenSource.token); expect(result, 'result not okay').to.be.ok; codeLens = result as CodeLens[]; @@ -904,6 +911,7 @@ testing2`; // Change settings to empty pythonSettings.datascience.codeRegularExpression = ''; + pythonSettings.fireChangeEvent(); result = codeLensProvider.provideCodeLenses(document.object, tokenSource.token); expect(result, 'result not okay').to.be.ok; codeLens = result as CodeLens[]; diff --git a/src/test/datascience/editor-integration/gotocell.functional.test.ts b/src/test/datascience/editor-integration/gotocell.functional.test.ts index 53abd84d8d7b..ec4c28c6b2cb 100644 --- a/src/test/datascience/editor-integration/gotocell.functional.test.ts +++ b/src/test/datascience/editor-integration/gotocell.functional.test.ts @@ -5,12 +5,15 @@ import { assert } from 'chai'; import { ChildProcess } from 'child_process'; import * as path from 'path'; import * as uuid from 'uuid/v4'; -import { CodeLens, Disposable, Position, Range, Uri } from 'vscode'; +import { CodeLens, Disposable, Position, Range, TextDocument, Uri } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; +import { range } from 'lodash'; import { IDocumentManager } from '../../../client/common/application/types'; import { EXTENSION_ROOT_DIR } from '../../../client/common/constants'; import { traceError } from '../../../client/common/logger'; +import { IDataScienceSettings } from '../../../client/common/types'; +import * as CellFactory from '../../../client/datascience/cellFactory'; import { Commands, Identifiers } from '../../../client/datascience/constants'; import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { @@ -105,6 +108,10 @@ suite('DataScience gotocell tests', () => { Uri.parse(Identifiers.InteractiveWindowIdentity) ); const listener = (codeLensFactory as any) as IInteractiveWindowListener; + listener.onMessage(InteractiveWindowMessages.NotebookIdentity, { + resource: Uri.parse(Identifiers.InteractiveWindowIdentity), + type: 'interactive' + }); listener.onMessage( InteractiveWindowMessages.NotebookExecutionActivated, Identifiers.InteractiveWindowIdentity @@ -134,7 +141,7 @@ suite('DataScience gotocell tests', () => { addMockData(c.code, c.result, c.cellType); docText = docText.concat(c.code, '\n'); }); - documentManager.addDocument(docText, filePath); + return documentManager.addDocument(docText, filePath); } function srcDirectory() { @@ -198,9 +205,9 @@ suite('DataScience gotocell tests', () => { assert.ok(scrollTo!.command!.title.includes(count), 'Wrong goto on cell'); } - function addSingleChange(range: Range, newText: string) { + function addSingleChange(r: Range, newText: string) { const filePath = path.join(srcDirectory(), 'foo.py'); - documentManager.changeDocument(filePath, [{ range, newText }]); + documentManager.changeDocument(filePath, [{ range: r, newText }]); } runTest('Basic execution', async () => { @@ -295,4 +302,53 @@ suite('DataScience gotocell tests', () => { // Our 1st execute should have moved verifyGoto('1', 3); }); + + runTest('Verify not recreating code lenses when not necessary', async () => { + // Override the function that generates cell ranges. We want to count how many times this is called + let generateCount = 0; + const oldGenerateRanges = (CellFactory as any).generateCellRangesFromDocument; + (CellFactory as any).generateCellRangesFromDocument = ( + document: TextDocument, + settings?: IDataScienceSettings + ) => { + generateCount = generateCount + 1; + return oldGenerateRanges(document, settings); + }; + + for (const i of range(0, 10)) { + const filePath = path.join(srcDirectory(), `foo${i}.py`); + const doc = addDocument( + [ + { + code: `#%%\na=1\na`, + result: 1 + }, + { + code: `#%%\na+=1\na`, + result: 2 + }, + { + code: `#%%\na+=4\na`, + result: 6 + }, + { + code: `#%%\n`, + result: undefined + } + ], + filePath + ); + codeLensFactory.createCodeLenses(doc); + } + + const server = await createNotebook(true); + assert.ok(server, 'No server created'); + const currentGenerateCount = generateCount; + + // Execute the second cell + await executeCell(1, server!); + + // verify we did not generate any new cell ranges + assert.equal(generateCount, currentGenerateCount, 'Should not be regenerating cell ranges on execute'); + }); }); diff --git a/src/test/datascience/mockDocumentManager.ts b/src/test/datascience/mockDocumentManager.ts index 56451411810e..cba1dcb80d14 100644 --- a/src/test/datascience/mockDocumentManager.ts +++ b/src/test/datascience/mockDocumentManager.ts @@ -96,6 +96,7 @@ export class MockDocumentManager implements IDocumentManager { public addDocument(code: string, file: string) { const mockDoc = new MockDocument(code, file, this.saveDocument); this.textDocuments.push(mockDoc); + return mockDoc; } public changeDocument(file: string, changes: { range: Range; newText: string }[]) { diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index 2fb75837a6a6..b49a605dc5df 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -1398,6 +1398,10 @@ plt.show()`, const outputs: string[] = []; @injectable() class Logger implements INotebookExecutionLogger { + public onKernelRestarted() { + // Do nothing on restarted + } + public async preExecute(cell: ICell, _silent: boolean): Promise { cellInputs.push(concatMultilineStringInput(cell.data.source)); }