Skip to content
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.

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

perf optimizations for large stream notebook outputs #187766

Merged
merged 10 commits into from Jul 18, 2023
54 changes: 36 additions & 18 deletions extensions/notebook-renderers/src/index.ts
Expand Up @@ -4,8 +4,8 @@
*--------------------------------------------------------------------------------------------*/

import type { ActivationFunction, OutputItem, RendererContext } from 'vscode-notebook-renderer';
import { createOutputContent, scrollableClass } from './textHelper';
import { HtmlRenderingHook, IDisposable, IRichRenderContext, JavaScriptRenderingHook, RenderOptions } from './rendererTypes';
import { appendScrollableOutput, createOutputContent, scrollableClass } from './textHelper';
import { HtmlRenderingHook, IDisposable, IRichRenderContext, JavaScriptRenderingHook, OutputWithAppend, RenderOptions } from './rendererTypes';
import { ttPolicy } from './htmlHelper';

function clearContainer(container: HTMLElement) {
Expand Down Expand Up @@ -172,7 +172,7 @@ function renderError(
outputElement.classList.add('traceback');

const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);
const content = createOutputContent(outputInfo.id, [err.stack ?? ''], ctx.settings.lineLimit, outputScrolling, trustHTML);
const content = createOutputContent(outputInfo.id, err.stack ?? '', ctx.settings.lineLimit, outputScrolling, trustHTML);
const contentParent = document.createElement('div');
contentParent.classList.toggle('word-wrap', ctx.settings.outputWordWrap);
disposableStore.push(ctx.onDidChangeSettings(e => {
Expand Down Expand Up @@ -270,29 +270,29 @@ function scrollingEnabled(output: OutputItem, options: RenderOptions) {
// div.output.output-stream <-- outputElement parameter
// div.scrollable? tabindex="0" <-- contentParent
// div output-item-id="{guid}" <-- content from outputItem parameter
function renderStream(outputInfo: OutputItem, outputElement: HTMLElement, error: boolean, ctx: IRichRenderContext): IDisposable {
function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, error: boolean, ctx: IRichRenderContext): IDisposable {
const appendedText = outputInfo.appendedText?.();
const disposableStore = createDisposableStore();
const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);

outputElement.classList.add('output-stream');

const text = outputInfo.text();
const newContent = createOutputContent(outputInfo.id, [text], ctx.settings.lineLimit, outputScrolling, false);
newContent.setAttribute('output-item-id', outputInfo.id);
if (error) {
newContent.classList.add('error');
}

const scrollTop = outputScrolling ? findScrolledHeight(outputElement) : undefined;

const previousOutputParent = getPreviousMatchingContentGroup(outputElement);
// If the previous output item for the same cell was also a stream, append this output to the previous
if (previousOutputParent) {
const existingContent = previousOutputParent.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null;
if (existingContent) {
existingContent.replaceWith(newContent);

if (appendedText && outputScrolling) {
appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false);
}
else {
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
existingContent.replaceWith(newContent);
}
} else {
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
previousOutputParent.appendChild(newContent);
}
previousOutputParent.classList.toggle('scrollbar-visible', previousOutputParent.scrollHeight > previousOutputParent.clientHeight);
Expand All @@ -301,12 +301,20 @@ function renderStream(outputInfo: OutputItem, outputElement: HTMLElement, error:
const existingContent = outputElement.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null;
let contentParent = existingContent?.parentElement;
if (existingContent && contentParent) {
existingContent.replaceWith(newContent);
while (newContent.nextSibling) {
// clear out any stale content if we had previously combined streaming outputs into this one
newContent.nextSibling.remove();
// appending output only in scrollable ouputs currently
if (appendedText && outputScrolling) {
appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false);
}
else {
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
existingContent.replaceWith(newContent);
while (newContent.nextSibling) {
// clear out any stale content if we had previously combined streaming outputs into this one
newContent.nextSibling.remove();
}
}
} else {
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
contentParent = document.createElement('div');
contentParent.appendChild(newContent);
while (outputElement.firstChild) {
Expand All @@ -327,13 +335,23 @@ function renderStream(outputInfo: OutputItem, outputElement: HTMLElement, error:
return disposableStore;
}

function createContent(outputInfo: OutputWithAppend, ctx: IRichRenderContext, outputScrolling: boolean, error: boolean) {
const text = outputInfo.text();
const newContent = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false);
newContent.setAttribute('output-item-id', outputInfo.id);
if (error) {
newContent.classList.add('error');
}
return newContent;
}

function renderText(outputInfo: OutputItem, outputElement: HTMLElement, ctx: IRichRenderContext): IDisposable {
const disposableStore = createDisposableStore();
clearContainer(outputElement);

const text = outputInfo.text();
const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);
const content = createOutputContent(outputInfo.id, [text], ctx.settings.lineLimit, outputScrolling, false);
const content = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false);
content.classList.add('output-plaintext');
if (ctx.settings.outputWordWrap) {
content.classList.add('word-wrap');
Expand Down
4 changes: 4 additions & 0 deletions extensions/notebook-renderers/src/rendererTypes.ts
Expand Up @@ -35,3 +35,7 @@ export interface RenderOptions {
}

export type IRichRenderContext = RendererContext<void> & { readonly settings: RenderOptions; readonly onDidChangeSettings: Event<RenderOptions> };

export interface OutputWithAppend extends OutputItem {
appendedText?(): string | undefined;
}
66 changes: 61 additions & 5 deletions extensions/notebook-renderers/src/test/notebookRenderer.test.ts
Expand Up @@ -5,8 +5,8 @@

import * as assert from 'assert';
import { activate } from '..';
import { OutputItem, RendererApi } from 'vscode-notebook-renderer';
import { IDisposable, IRichRenderContext, RenderOptions } from '../rendererTypes';
import { RendererApi } from 'vscode-notebook-renderer';
import { IDisposable, IRichRenderContext, OutputWithAppend, RenderOptions } from '../rendererTypes';
import { JSDOM } from "jsdom";

const dom = new JSDOM();
Expand Down Expand Up @@ -116,10 +116,13 @@ suite('Notebook builtin output renderer', () => {
}
}

function createOutputItem(text: string, mime: string, id: string = '123'): OutputItem {
function createOutputItem(text: string, mime: string, id: string = '123', appendedText?: string): OutputWithAppend {
return {
id: id,
mime: mime,
appendedText() {
return appendedText;
},
text() {
return text;
},
Expand Down Expand Up @@ -177,9 +180,9 @@ suite('Notebook builtin output renderer', () => {
assert.ok(renderer, 'Renderer not created');

const outputElement = new OutputHtml().getFirstOuputElement();
const outputItem = createOutputItem('content', 'text/plain');
const outputItem = createOutputItem('content', mimeType);
await renderer!.renderOutputItem(outputItem, outputElement);
const outputItem2 = createOutputItem('replaced content', 'text/plain');
const outputItem2 = createOutputItem('replaced content', mimeType);
await renderer!.renderOutputItem(outputItem2, outputElement);

const inserted = outputElement.firstChild as HTMLElement;
Expand All @@ -189,6 +192,59 @@ suite('Notebook builtin output renderer', () => {

});

test('Append streaming output', async () => {
const context = createContext({ outputWordWrap: false, outputScrolling: false });
const renderer = await activate(context);
assert.ok(renderer, 'Renderer not created');

const outputElement = new OutputHtml().getFirstOuputElement();
const outputItem = createOutputItem('content', stdoutMimeType, '123', 'ignoredAppend');
await renderer!.renderOutputItem(outputItem, outputElement);
const outputItem2 = createOutputItem('content\nappended', stdoutMimeType, '\nappended');
await renderer!.renderOutputItem(outputItem2, outputElement);

const inserted = outputElement.firstChild as HTMLElement;
assert.ok(inserted.innerHTML.indexOf('>content</') !== -1, `Previous content should still exist: ${outputElement.innerHTML}`);
assert.ok(inserted.innerHTML.indexOf('ignoredAppend') === -1, `Append value should not be used on first render: ${outputElement.innerHTML}`);
assert.ok(inserted.innerHTML.indexOf('>appended</') !== -1, `Content was not appended to output element: ${outputElement.innerHTML}`);
assert.ok(inserted.innerHTML.indexOf('>content</') === inserted.innerHTML.lastIndexOf('>content</'), `Original content should not be duplicated: ${outputElement.innerHTML}`);
});

test('Append large streaming outputs', async () => {
const context = createContext({ outputWordWrap: false, outputScrolling: false });
const renderer = await activate(context);
assert.ok(renderer, 'Renderer not created');

const outputElement = new OutputHtml().getFirstOuputElement();
const lotsOfLines = new Array(4998).fill('line').join('\n') + 'endOfInitialContent';
const firstOuput = lotsOfLines + 'expected1';
const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123');
await renderer!.renderOutputItem(outputItem, outputElement);
const appended = '\n' + lotsOfLines + 'expectedAppend';
const outputItem2 = createOutputItem(firstOuput + appended, stdoutMimeType, appended);
await renderer!.renderOutputItem(outputItem2, outputElement);

const inserted = outputElement.firstChild as HTMLElement;
assert.ok(inserted.innerHTML.indexOf('>expected1</') !== -1, `Last bit of previous content should still exist: ${outputElement.innerHTML}`);
assert.ok(inserted.innerHTML.indexOf('>expectedAppend</') !== -1, `Content was not appended to output element: ${outputElement.innerHTML}`);
});

test('Streaming outputs larger than the line limit are truncated', async () => {
const context = createContext({ outputWordWrap: false, outputScrolling: false });
const renderer = await activate(context);
assert.ok(renderer, 'Renderer not created');

const outputElement = new OutputHtml().getFirstOuputElement();
const lotsOfLines = new Array(11000).fill('line').join('\n') + 'endOfInitialContent';
const firstOuput = 'shouldBeTruncated' + lotsOfLines + 'expected1';
const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123');
await renderer!.renderOutputItem(outputItem, outputElement);

const inserted = outputElement.firstChild as HTMLElement;
assert.ok(inserted.innerHTML.indexOf('>endOfInitialContent</') !== -1, `Last bit of content should exist: ${outputElement.innerHTML}`);
assert.ok(inserted.innerHTML.indexOf('>shouldBeTruncated</') === -1, `Beginning content should be truncated: ${outputElement.innerHTML}`);
});

test(`Render with wordwrap and scrolling for error output`, async () => {
const context = createContext({ outputWordWrap: true, outputScrolling: true });
const renderer = await activate(context);
Expand Down
36 changes: 31 additions & 5 deletions extensions/notebook-renderers/src/textHelper.ts
Expand Up @@ -4,9 +4,11 @@
*--------------------------------------------------------------------------------------------*/

import { handleANSIOutput } from './ansi';

export const scrollableClass = 'scrollable';

const softScrollableLineLimit = 5000;
const hardScrollableLineLimit = 8000;
Copy link
Member

Choose a reason for hiding this comment

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

This is very smart


/**
* Output is Truncated. View as a [scrollable element] or open in a [text editor]. Adjust cell output [settings...]
*/
Expand Down Expand Up @@ -91,22 +93,46 @@ function truncatedArrayOfString(id: string, buffer: string[], linesLimit: number

function scrollableArrayOfString(id: string, buffer: string[], trustHtml: boolean) {
const element = document.createElement('div');
if (buffer.length > 5000) {
if (buffer.length > softScrollableLineLimit) {
element.appendChild(generateNestedViewAllElement(id));
}

element.appendChild(handleANSIOutput(buffer.slice(-5000).join('\n'), trustHtml));
element.appendChild(handleANSIOutput(buffer.slice(-1 * softScrollableLineLimit).join('\n'), trustHtml));

return element;
}

export function createOutputContent(id: string, outputs: string[], linesLimit: number, scrollable: boolean, trustHtml: boolean): HTMLElement {
export function createOutputContent(id: string, outputText: string, linesLimit: number, scrollable: boolean, trustHtml: boolean): HTMLElement {

const buffer = outputs.join('\n').split(/\r\n|\r|\n/g);
const buffer = outputText.split(/\r\n|\r|\n/g);

if (scrollable) {
return scrollableArrayOfString(id, buffer, trustHtml);
} else {
return truncatedArrayOfString(id, buffer, linesLimit, trustHtml);
}
}

const outputLengths: Record<string, number> = {};

export function appendScrollableOutput(element: HTMLElement, id: string, appended: string, fullText: string, trustHtml: boolean) {
if (!outputLengths[id]) {
outputLengths[id] = 0;
}

const buffer = appended.split(/\r\n|\r|\n/g);
const appendedLength = buffer.length + outputLengths[id];
// Allow the output to grow to the hard limit then replace it with the last softLimit number of lines if it grows too large
amunger marked this conversation as resolved.
Show resolved Hide resolved
if (buffer.length + outputLengths[id] > hardScrollableLineLimit) {
const fullBuffer = fullText.split(/\r\n|\r|\n/g);
outputLengths[id] = Math.min(fullBuffer.length, softScrollableLineLimit);
const newElement = scrollableArrayOfString(id, fullBuffer.slice(-1 * softScrollableLineLimit), trustHtml);
newElement.setAttribute('output-item-id', id);
element.replaceWith(newElement);
}
else {
element.appendChild(handleANSIOutput(buffer.join('\n'), trustHtml));
amunger marked this conversation as resolved.
Show resolved Hide resolved
outputLengths[id] = appendedLength;
}
}

2 changes: 1 addition & 1 deletion src/vs/workbench/api/common/extHostNotebookDocument.ts
Expand Up @@ -132,7 +132,7 @@ export class ExtHostCell {
const compressed = notebookCommon.compressOutputItemStreams(mimeOutputs.get(mime)!);
output.items.push({
mime,
data: compressed.buffer
data: compressed.data.buffer
});
});
}
Expand Down
Expand Up @@ -1413,15 +1413,11 @@ export class BackLayerWebView<T extends ICommonCellInfo> extends Themable {
}

// create new output
const createOutput = () => {
const { message, renderer, transfer: transferable } = this._createOutputCreationMessage(cellInfo, content, cellTop, offset, false, false);
this._sendMessageToWebview(message, transferable);
this.insetMapping.set(content.source, { outputId: message.outputId, versionId: content.source.model.versionId, cellInfo: cellInfo, renderer, cachedCreation: message });
this.hiddenInsetMapping.delete(content.source);
this.reversedInsetMapping.set(message.outputId, content.source);
};

createOutput();
const { message, renderer, transfer: transferable } = this._createOutputCreationMessage(cellInfo, content, cellTop, offset, false, false);
this._sendMessageToWebview(message, transferable);
this.insetMapping.set(content.source, { outputId: message.outputId, versionId: content.source.model.versionId, cellInfo: cellInfo, renderer, cachedCreation: message });
this.hiddenInsetMapping.delete(content.source);
this.reversedInsetMapping.set(message.outputId, content.source);
}

private createMetadata(output: ICellOutput, mimeType: string) {
Expand Down Expand Up @@ -1503,13 +1499,21 @@ export class BackLayerWebView<T extends ICommonCellInfo> extends Themable {
}

const outputCache = this.insetMapping.get(content.source)!;

if (outputCache.versionId === content.source.model.versionId) {
// already sent this output version to the renderer
return;
}

this.hiddenInsetMapping.delete(content.source);
let updatedContent: ICreationContent | undefined = undefined;

const transfer: ArrayBuffer[] = [];
if (content.type === RenderOutputType.Extension) {
const output = content.source.model;
const firstBuffer = output.outputs.find(op => op.mime === content.mimeType)!;
const appenededData = output.appendedSinceVersion(outputCache.versionId, content.mimeType);
const appended = appenededData ? { valueBytes: appenededData.buffer, previousVersion: outputCache.versionId } : undefined;

const valueBytes = copyBufferIfNeeded(firstBuffer.data.buffer, transfer);
updatedContent = {
Expand All @@ -1519,6 +1523,7 @@ export class BackLayerWebView<T extends ICommonCellInfo> extends Themable {
output: {
mime: content.mimeType,
valueBytes,
appended: appended
},
allOutputs: output.outputs.map(output => ({ mime: output.mime }))
};
Expand Down
Expand Up @@ -188,11 +188,18 @@ export interface IOutputRequestDto {
export interface OutputItemEntry {
readonly mime: string;
readonly valueBytes: Uint8Array;
readonly appended?: { valueBytes: Uint8Array; previousVersion: number };
}

export type ICreationContent =
| { readonly type: RenderOutputType.Html; readonly htmlContent: string }
| { readonly type: RenderOutputType.Extension; readonly outputId: string; readonly metadata: unknown; readonly output: OutputItemEntry; readonly allOutputs: ReadonlyArray<{ readonly mime: string }> };
| {
readonly type: RenderOutputType.Extension;
readonly outputId: string;
readonly metadata: unknown;
readonly output: OutputItemEntry;
readonly allOutputs: ReadonlyArray<{ readonly mime: string }>;
};

export interface ICreationRequestMessage {
readonly type: 'html';
Expand Down