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.

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

Jump to bottom

Cache outline headers & ref count outline provider #210213

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Cache outline headers & ref count outline provider #210213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be30a9a
Cache outline headers & ref count outline provider
DonJayamanne Apr 12, 2024
2a70745
Fix tests
DonJayamanne Apr 16, 2024
efdf336
Remove handle
DonJayamanne Apr 17, 2024
90143fa
Oops
DonJayamanne Apr 17, 2024
4669ff4
Simpler cachine
DonJayamanne Apr 17, 2024
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
31 changes: 16 additions & 15 deletions src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { IListAccessibilityProvider } from 'vs/base/browser/ui/list/listWidget';
import { IDataSource, ITreeNode, ITreeRenderer } from 'vs/base/browser/ui/tree/tree';
import { Emitter, Event } from 'vs/base/common/event';
import { FuzzyScore, createMatches } from 'vs/base/common/filters';
import { Disposable, DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
import { Disposable, DisposableStore, IDisposable, toDisposable, type IReference } from 'vs/base/common/lifecycle';
import { ThemeIcon } from 'vs/base/common/themables';
import { URI } from 'vs/base/common/uri';
import { getIconClassesForLanguageId } from 'vs/editor/common/services/getIconClasses';
Expand Down Expand Up @@ -51,6 +51,7 @@ import { IOutlinePane } from 'vs/workbench/contrib/outline/browser/outline';
import { Codicon } from 'vs/base/common/codicons';
import { NOTEBOOK_IS_ACTIVE_EDITOR } from 'vs/workbench/contrib/notebook/common/notebookContextKeys';
import { NotebookOutlineConstants } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory';
import { INotebookCellOutlineProviderFactory } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProviderFactory';

class NotebookOutlineTemplate {

Expand Down Expand Up @@ -337,7 +338,7 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
readonly onDidChange: Event<OutlineChangeEvent> = this._onDidChange.event;

get entries(): OutlineEntry[] {
return this._outlineProvider?.entries ?? [];
return this._outlineProviderReference?.object?.entries ?? [];
}

private readonly _entriesDisposables = new DisposableStore();
Expand All @@ -347,10 +348,10 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
readonly outlineKind = 'notebookCells';

get activeElement(): OutlineEntry | undefined {
return this._outlineProvider?.activeElement;
return this._outlineProviderReference?.object?.activeElement;
}

private _outlineProvider: NotebookCellOutlineProvider | undefined;
private _outlineProviderReference: IReference<NotebookCellOutlineProvider> | undefined;
private readonly _localDisposables = new DisposableStore();

constructor(
Expand All @@ -363,14 +364,14 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
const installSelectionListener = () => {
const notebookEditor = _editor.getControl();
if (!notebookEditor?.hasModel()) {
this._outlineProvider?.dispose();
this._outlineProvider = undefined;
this._outlineProviderReference?.dispose();
this._outlineProviderReference = undefined;
this._localDisposables.clear();
} else {
this._outlineProvider?.dispose();
this._outlineProviderReference?.dispose();
this._localDisposables.clear();
this._outlineProvider = instantiationService.createInstance(NotebookCellOutlineProvider, notebookEditor, _target);
this._localDisposables.add(this._outlineProvider.onDidChange(e => {
this._outlineProviderReference = instantiationService.invokeFunction((accessor) => accessor.get(INotebookCellOutlineProviderFactory).getOrCreate(notebookEditor, _target));
this._localDisposables.add(this._outlineProviderReference.object.onDidChange(e => {
this._onDidChange.fire(e);
}));
}
Expand Down Expand Up @@ -411,7 +412,7 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
return result;
}
},
quickPickDataSource: instantiationService.createInstance(NotebookQuickPickProvider, () => (this._outlineProvider?.entries ?? [])),
quickPickDataSource: instantiationService.createInstance(NotebookQuickPickProvider, () => (this._outlineProviderReference?.object?.entries ?? [])),
treeDataSource,
delegate,
renderers,
Expand All @@ -425,7 +426,7 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
const showCodeCellSymbols = configurationService.getValue<boolean>(NotebookSetting.outlineShowCodeCellSymbols);
const showMarkdownHeadersOnly = configurationService.getValue<boolean>(NotebookSetting.outlineShowMarkdownHeadersOnly);

for (const entry of parent instanceof NotebookCellOutline ? (this._outlineProvider?.entries ?? []) : parent.children) {
for (const entry of parent instanceof NotebookCellOutline ? (this._outlineProviderReference?.object?.entries ?? []) : parent.children) {
if (entry.cell.cellKind === CellKind.Markup) {
if (!showMarkdownHeadersOnly) {
yield entry;
Expand All @@ -444,14 +445,14 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
}

async setFullSymbols(cancelToken: CancellationToken) {
await this._outlineProvider?.setFullSymbols(cancelToken);
await this._outlineProviderReference?.object?.setFullSymbols(cancelToken);
}

get uri(): URI | undefined {
return this._outlineProvider?.uri;
return this._outlineProviderReference?.object?.uri;
}
get isEmpty(): boolean {
return this._outlineProvider?.isEmpty ?? true;
return this._outlineProviderReference?.object?.isEmpty ?? true;
}
async reveal(entry: OutlineEntry, options: IEditorOptions, sideBySide: boolean): Promise<void> {
await this._editorService.openEditor({
Expand Down Expand Up @@ -530,7 +531,7 @@ export class NotebookCellOutline implements IOutline<OutlineEntry> {
this._onDidChange.dispose();
this._dispoables.dispose();
this._entriesDisposables.dispose();
this._outlineProvider?.dispose();
this._outlineProviderReference?.dispose();
this._localDisposables.dispose();
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import { ILabelService } from 'vs/platform/label/common/label';
import { IEditorGroupsService } from 'vs/workbench/services/editor/common/editorGroupsService';
import { NotebookRendererMessagingService } from 'vs/workbench/contrib/notebook/browser/services/notebookRendererMessagingServiceImpl';
import { INotebookRendererMessagingService } from 'vs/workbench/contrib/notebook/common/notebookRendererMessagingService';
import { INotebookCellOutlineProviderFactory, NotebookCellOutlineProviderFactory } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProviderFactory';

// Editor Controller
import 'vs/workbench/contrib/notebook/browser/controller/coreActions';
Expand Down Expand Up @@ -755,6 +756,7 @@ registerSingleton(INotebookExecutionStateService, NotebookExecutionStateService,
registerSingleton(INotebookRendererMessagingService, NotebookRendererMessagingService, InstantiationType.Delayed);
registerSingleton(INotebookKeymapService, NotebookKeymapService, InstantiationType.Delayed);
registerSingleton(INotebookLoggingService, NotebookLoggingService, InstantiationType.Delayed);
registerSingleton(INotebookCellOutlineProviderFactory, NotebookCellOutlineProviderFactory, InstantiationType.Delayed);

const schemas: IJSONSchemaMap = {};
function isConfigurationPropertySchema(x: IConfigurationPropertySchema | { [path: string]: IConfigurationPropertySchema }): x is IConfigurationPropertySchema {
Expand Down
1 change: 1 addition & 0 deletions src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ export interface ICellViewModel extends IGenericCellViewModel {
focusedOutputId?: string | undefined;
outputIsHovered: boolean;
getText(): string;
getAlternativeId(): number;
getTextLength(): number;
getHeight(lineHeight: number): number;
metadata: NotebookCellMetadata;
Expand Down
7 changes: 1 addition & 6 deletions src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,8 @@ import { Schemas } from 'vs/base/common/network';
import { DropIntoEditorController } from 'vs/editor/contrib/dropOrPasteInto/browser/dropIntoEditorController';
import { CopyPasteController } from 'vs/editor/contrib/dropOrPasteInto/browser/copyPasteController';
import { NotebookStickyScroll } from 'vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll';
import { NotebookCellOutlineProvider } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider';
import { AccessibilityVerbositySettingId } from 'vs/workbench/contrib/accessibility/browser/accessibilityConfiguration';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { OutlineTarget } from 'vs/workbench/services/outline/browser/outline';
import { PixelRatio } from 'vs/base/browser/pixelRatio';
import { ICodeEditorService } from 'vs/editor/browser/services/codeEditorService';
import { PreventDefaultContextMenuItemsContextKeyName } from 'vs/workbench/contrib/webview/browser/webview.contribution';
Expand Down Expand Up @@ -278,7 +276,6 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
public readonly scopedContextKeyService: IContextKeyService;
private readonly instantiationService: IInstantiationService;
private readonly _notebookOptions: NotebookOptions;
public readonly _notebookOutline: NotebookCellOutlineProvider;

private _currentProgress: IProgressRunner | undefined;

Expand Down Expand Up @@ -335,8 +332,6 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD

this._register(this.instantiationService.createInstance(NotebookEditorContextKeys, this));

this._notebookOutline = this._register(this.instantiationService.createInstance(NotebookCellOutlineProvider, this, OutlineTarget.QuickPick));

this._register(notebookKernelService.onDidChangeSelectedNotebooks(e => {
if (isEqual(e.notebook, this.viewModel?.uri)) {
this._loadKernelPreloads();
Expand Down Expand Up @@ -1054,7 +1049,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
}

private _registerNotebookStickyScroll() {
this._notebookStickyScroll = this._register(this.instantiationService.createInstance(NotebookStickyScroll, this._notebookStickyScrollContainer, this, this._notebookOutline, this._list));
this._notebookStickyScroll = this._register(this.instantiationService.createInstance(NotebookStickyScroll, this._notebookStickyScrollContainer, this, this._list));

const localDisposableStore = this._register(new DisposableStore());

Expand Down
4 changes: 4 additions & 0 deletions src/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,10 @@ export abstract class BaseCellViewModel extends Disposable {
return this.model.getValue();
}

getAlternativeId(): number {
return this.model.alternativeId;
}

getTextLength(): number {
return this.model.getTextLength();
}
Expand Down
16 changes: 12 additions & 4 deletions src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type entryDesc = {
export class NotebookOutlineEntryFactory {

private cellOutlineEntryCache: Record<string, entryDesc[]> = {};

private readonly cachedMarkdownOutlineEntries = new WeakMap<ICellViewModel, { alternativeId: number; headers: { depth: number, text: string }[] }>();
constructor(
private readonly executionStateService: INotebookExecutionStateService
) { }
Expand All @@ -47,23 +47,31 @@ export class NotebookOutlineEntryFactory {
let hasHeader = false;

if (isMarkdown) {
const cache = this.cachedMarkdownOutlineEntries.get(cell);
const markdownEntries: OutlineEntry[] = [];
const fullContent = cell.getText().substring(0, 10000);
for (const { depth, text } of getMarkdownHeadersInCell(fullContent)) {
const headers = cache?.alternativeId === cell.getAlternativeId() ? cache.headers : Array.from(getMarkdownHeadersInCell(fullContent));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caching results in saving 50% of the time used to calculate outlines.
Down from 50ms to 26

I.e. this PR brings down the total time in keystokes from 150ms to 0 (when debounced) and when user stops typing, then we spend 20ms.

We should see some significant perf gains here.

this.cachedMarkdownOutlineEntries.set(cell, { alternativeId: cell.getAlternativeId(), headers });

for (const { depth, text } of headers) {
hasHeader = true;
entries.push(new OutlineEntry(index++, depth, cell, text, false, false));
markdownEntries.push(new OutlineEntry(index++, depth, cell, text, false, false));
}

if (!hasHeader) {
const fullContent = cell.getText().substring(0, 10000);
// no markdown syntax headers, try to find html tags
const match = fullContent.match(/<h([1-6]).*>(.*)<\/h\1>/i);
if (match) {
hasHeader = true;
const level = parseInt(match[1]);
const text = match[2].trim();
entries.push(new OutlineEntry(index++, level, cell, text, false, false));
markdownEntries.push(new OutlineEntry(index++, level, cell, text, false, false));
}
}

entries.push(...markdownEntries);

if (!hasHeader) {
content = renderMarkdownAsPlaintext({ value: content });
}
Expand Down
86 changes: 47 additions & 39 deletions src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,21 @@
*--------------------------------------------------------------------------------------------*/

import { Emitter, Event } from 'vs/base/common/event';
import { DisposableStore, MutableDisposable, combinedDisposable } from 'vs/base/common/lifecycle';
import { DisposableStore, MutableDisposable } from 'vs/base/common/lifecycle';
import { isEqual } from 'vs/base/common/resources';
import { URI } from 'vs/base/common/uri';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { IMarkerService } from 'vs/platform/markers/common/markers';
import { IThemeService } from 'vs/platform/theme/common/themeService';
import { IActiveNotebookEditor, ICellViewModel, INotebookEditor, INotebookViewCellsUpdateEvent } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
import { CellKind, NotebookSetting } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { INotebookExecutionStateService, NotebookExecutionType, type ICellExecutionStateChangedEvent, type IExecutionStateChangedEvent } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService';
import { IActiveNotebookEditor, ICellViewModel, INotebookEditor, type INotebookViewCellsUpdateEvent } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
import { CellKind, NotebookCellsChangeType, NotebookSetting } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { INotebookExecutionStateService, NotebookExecutionType } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService';
import { OutlineChangeEvent, OutlineConfigKeys, OutlineTarget } from 'vs/workbench/services/outline/browser/outline';
import { OutlineEntry } from './OutlineEntry';
import { IOutlineModelService } from 'vs/editor/contrib/documentSymbols/browser/outlineModel';
import { CancellationToken } from 'vs/base/common/cancellation';
import { NotebookOutlineConstants, NotebookOutlineEntryFactory } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory';
import { Delayer } from 'vs/base/common/async';

export class NotebookCellOutlineProvider {
private readonly _disposables = new DisposableStore();
Expand All @@ -41,7 +42,6 @@ export class NotebookCellOutlineProvider {
}

private readonly _outlineEntryFactory: NotebookOutlineEntryFactory;

constructor(
private readonly _editor: INotebookEditor,
private readonly _target: OutlineTarget,
Expand All @@ -53,29 +53,34 @@ export class NotebookCellOutlineProvider {
) {
this._outlineEntryFactory = new NotebookOutlineEntryFactory(notebookExecutionStateService);

const selectionListener = new MutableDisposable();
this._disposables.add(selectionListener);

selectionListener.value = combinedDisposable(
Event.debounce<void, void>(
_editor.onDidChangeSelection,
(last, _current) => last,
200
)(this._recomputeActive, this),
Event.debounce<INotebookViewCellsUpdateEvent, INotebookViewCellsUpdateEvent>(
_editor.onDidChangeViewCells,
(last, _current) => last ?? _current,
200
)(this._recomputeState, this)
this._disposables.add(Event.debounce<void, void>(
_editor.onDidChangeSelection,
(last, _current) => last,
200
)(() => {
this._recomputeActive();
}, this))
this._disposables.add(Event.debounce<INotebookViewCellsUpdateEvent, INotebookViewCellsUpdateEvent>(
_editor.onDidChangeViewCells,
(last, _current) => last ?? _current,
200
)(() => {
this._recomputeActive();
}, this)
);

// .3s of a delay is sufficient, 100-200s is too quick and will unnecessarily block the ui thread.
// Given we're only updating the outline when the user types, we can afford to wait a bit.
const delayer = this._disposables.add(new Delayer<void>(300));
const delayedRecompute = () => delayer.trigger(() => this._recomputeState());

this._disposables.add(_configurationService.onDidChangeConfiguration(e => {
if (e.affectsConfiguration(NotebookSetting.outlineShowMarkdownHeadersOnly) ||
e.affectsConfiguration(NotebookSetting.outlineShowCodeCells) ||
e.affectsConfiguration(NotebookSetting.outlineShowCodeCellSymbols) ||
e.affectsConfiguration(NotebookSetting.breadcrumbsShowCodeCells)
) {
this._recomputeState();
delayedRecompute();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debouncing in outline provider resulted in significant improvements
From 60s to run a cell to 2-3s.
(this was already fixed in a previous PR, but this now adds the same debouncing for typing in editor).

When ever we make changes in editor (typing), we trigger outline changes synchronously
As a result, ever key stroke results in outline calculation.
With a large notebook (100 markdown cells and each markdown cell having 10-20 lines), then it takes around 50ms to calculate the outline.
This is ignoring code cells in outline.

Thus every key stroke results in a 50ms delay in renderer.
Solution = debounce

}
}));

Expand All @@ -84,17 +89,28 @@ export class NotebookCellOutlineProvider {
}));

this._disposables.add(
Event.debounce<ICellExecutionStateChangedEvent | IExecutionStateChangedEvent>(
notebookExecutionStateService.onDidChangeExecution,
(last, _current) => last ?? _current,
200)(e => {
if (e.type === NotebookExecutionType.cell && !!this._editor.textModel && e.affectsNotebook(this._editor.textModel?.uri)) {
this._recomputeState();
}
})
notebookExecutionStateService.onDidChangeExecution(e => {
if (e.type === NotebookExecutionType.cell && !!this._editor.textModel && e.affectsNotebook(this._editor.textModel?.uri)) {
delayedRecompute();
}
})
);

this._recomputeState();
const disposable = this._disposables.add(new DisposableStore());
const monitorModelChanges = () => {
disposable.clear();
if (!this._editor.textModel) {
return;
}
disposable.add(this._editor.textModel.onDidChangeContent(contentChanges => {
if (contentChanges.rawEvents.some(c => c.kind === NotebookCellsChangeType.ChangeCellContent)) {
delayedRecompute();
}
}));
}
this._disposables.add(this._editor.onDidChangeModel(monitorModelChanges));
monitorModelChanges();
this._recomputeState()
}

dispose(): void {
Expand All @@ -104,10 +120,6 @@ export class NotebookCellOutlineProvider {
this._disposables.dispose();
}

init(): void {
this._recomputeState();
}

async setFullSymbols(cancelToken: CancellationToken) {
const notebookEditorWidget = this._editor;

Expand All @@ -126,8 +138,9 @@ export class NotebookCellOutlineProvider {

this._recomputeState();
}

private handle = 0;
private _recomputeState(): void {
this.handle = this.handle + 1;
this._entriesDisposables.clear();
this._activeEntry = undefined;
this._uri = undefined;
Expand Down Expand Up @@ -159,11 +172,6 @@ export class NotebookCellOutlineProvider {
const entries: OutlineEntry[] = [];
for (const cell of notebookCells) {
entries.push(...this._outlineEntryFactory.getOutlineEntries(cell, this._target, entries.length));
// send an event whenever any of the cells change
this._entriesDisposables.add(cell.model.onDidChangeContent(() => {
this._recomputeState();
this._onDidChange.fire({});
}));
}

// build a tree from the list of entries
Expand Down
Loading
Loading