Skip to content

Commit

Permalink
Notebook Performance Improvements to Cell Editing/Output Changes/Exec…
Browse files Browse the repository at this point in the history 
…ution Count Changes (microsoft#6867)

* edit perf

* Save multiline source in notebooks

* More merges

* Single, multi line works needs work

* Works with single + multi and recomputes active

* Actual perf improvements this time

* code cleanup

* Calculating output position on the fly

* Hmm can we use brackets to make this simpler?

* monday progress

* output working. lots of improvements.

* First tests working

* 10 tests now, fixed bugs

* Cleanup, add output test

* More fixes

* Need to still fix execution count bug

* Tests pass, added comments

* Cleanup

* PR comments round 1

* Deal with merge issues from master, layering

* Deleting duplicate file

* More PR Comments

* PR Comments
  • Loading branch information
@chlafreniere
chlafreniere authored Aug 26, 2019
1 parent 4afa282 commit 84b3e87
Show file tree
Hide file tree
Showing 19 changed files with 1,157 additions and 73 deletions.
19 changes: 19 additions & 0 deletions extensions/notebook/language-configuration.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"comments": {
"lineComment": "//",
"blockComment": [ "/*", "*/" ]
},
"brackets": [
["{", "}"],
["[", "]"]
],
"autoClosingPairs": [
{ "open": "{", "close": "}", "notIn": ["string"] },
{ "open": "[", "close": "]", "notIn": ["string"] },
{ "open": "(", "close": ")", "notIn": ["string"] },
{ "open": "'", "close": "'", "notIn": ["string"] },
{ "open": "/*", "close": "*/", "notIn": ["string"] },
{ "open": "\"", "close": "\"", "notIn": ["string", "comment"] },
{ "open": "`", "close": "`", "notIn": ["string", "comment"] }
]
}
3 changes: 2 additions & 1 deletion extensions/notebook/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@
],
"aliases": [
"Notebook"
]
],
"configuration": "./language-configuration.json"
}
],
"menus": {
Expand Down
1 change: 1 addition & 0 deletions src/sql/azdata.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4448,6 +4448,7 @@ declare module 'azdata' {
source: string | string[];
metadata?: {
language?: string;
azdata_cell_guid?: string;
};
execution_count?: number;
outputs?: ICellOutput[];
Expand Down
7 changes: 5 additions & 2 deletions src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,7 @@ export class MainThreadNotebookDocumentsAndEditors extends Disposable implements
case NotebookChangeType.CellOutputUpdated:
case NotebookChangeType.CellSourceUpdated:
case NotebookChangeType.DirtyStateChanged:
case NotebookChangeType.CellOutputCleared:
return NotebookChangeKind.ContentUpdated;
case NotebookChangeType.KernelChanged:
case NotebookChangeType.TrustChanged:
Expand Down Expand Up @@ -654,7 +655,8 @@ export class MainThreadNotebookDocumentsAndEditors extends Disposable implements
cell_type: cell.cellType,
execution_count: cell.executionCount,
metadata: {
language: cell.language
language: cell.language,
azdata_cell_guid: cell.cellGuid
},
source: undefined,
outputs: [...cell.outputs]
Expand All @@ -669,7 +671,8 @@ export class MainThreadNotebookDocumentsAndEditors extends Disposable implements
cell_type: cells.cellType,
execution_count: undefined,
metadata: {
language: cells.language
language: cells.language,
azdata_cell_guid: cells.cellGuid
},
source: undefined
}
Expand Down
1 change: 1 addition & 0 deletions src/sql/workbench/parts/notebook/browser/cellViews/code.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ export class CodeComponent extends AngularDisposable implements OnInit, OnChange
this._register(this._editorInput);
this._register(this._editorModel.onDidChangeContent(e => {
this._editor.setHeightToScrollHeight();
this.cellModel.modelContentChangedEvent = e;
this.cellModel.source = this._editorModel.getValue();
this.onContentChanged.emit();
this.checkForLanguageMagics();
Expand Down
2 changes: 1 addition & 1 deletion src/sql/workbench/parts/notebook/browser/cellViews/codeCell.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class CodeCellComponent extends CellView implements OnInit, OnChanges {
@HostListener('document:keydown.escape', ['$event'])
handleKeyboardEvent() {
this.cellModel.active = false;
this._model.activeCell = undefined;
this._model.updateActiveCell(undefined);
}

private _model: NotebookModel;
Expand Down
88 changes: 69 additions & 19 deletions src/sql/workbench/parts/notebook/browser/models/notebookInput.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { IInstantiationService } from 'vs/platform/instantiation/common/instanti
import { ITextModelService } from 'vs/editor/common/services/resolverService';
import { INotebookModel, IContentManager, NotebookContentChange } from 'sql/workbench/parts/notebook/common/models/modelInterfaces';
import { TextFileEditorModel } from 'vs/workbench/services/textfile/common/textFileEditorModel';
import { Range } from 'vs/editor/common/core/range';
import { UntitledEditorModel } from 'vs/workbench/common/editor/untitledEditorModel';
import { Schemas } from 'vs/base/common/network';
import { ITextFileService, ISaveOptions, StateChange } from 'vs/workbench/services/textfile/common/textfiles';
Expand All @@ -26,12 +25,17 @@ import { UntitledEditorInput } from 'vs/workbench/common/editor/untitledEditorIn
import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions';
import { IDisposable } from 'vs/base/common/lifecycle';
import { NotebookChangeType } from 'sql/workbench/parts/notebook/common/models/contracts';
import { Deferred } from 'sql/base/common/promise';
import { NotebookTextFileModel } from 'sql/workbench/parts/notebook/common/models/notebookTextFileModel';

export type ModeViewSaveHandler = (handle: number) => Thenable<boolean>;

export class NotebookEditorModel extends EditorModel {
private dirty: boolean;
private _dirty: boolean;
private _changeEventsHookedUp: boolean = false;
private _notebookTextFileModel: NotebookTextFileModel = new NotebookTextFileModel();
private readonly _onDidChangeDirty: Emitter<void> = this._register(new Emitter<void>());
private _lastEditFullReplacement: boolean;
constructor(public readonly notebookUri: URI,
private textEditorModel: TextFileEditorModel | UntitledEditorModel,
@INotebookService private notebookService: INotebookService,
Expand All @@ -41,9 +45,17 @@ export class NotebookEditorModel extends EditorModel {
this._register(this.notebookService.onNotebookEditorAdd(notebook => {
if (notebook.id === this.notebookUri.toString()) {
// Hook to content change events
notebook.modelReady.then(() => {
this._register(notebook.model.kernelChanged(e => this.updateModel()));
this._register(notebook.model.contentChanged(e => this.updateModel(e)));
notebook.modelReady.then((model) => {
if (!this._changeEventsHookedUp) {
this._changeEventsHookedUp = true;
this._register(model.kernelChanged(e => this.updateModel(undefined, NotebookChangeType.KernelChanged)));
this._register(model.contentChanged(e => this.updateModel(e, e.changeType)));
this._register(notebook.model.onActiveCellChanged((cell) => {
if (cell) {
this._notebookTextFileModel.activeCellGuid = cell.cellGuid;
}
}));
}
}, err => undefined);
}
}));
Expand All @@ -58,23 +70,27 @@ export class NotebookEditorModel extends EditorModel {
}
}));
}
this.dirty = this.textEditorModel.isDirty();
this._dirty = this.textEditorModel.isDirty();
}

public get contentString(): string {
let model = this.textEditorModel.textEditorModel;
return model.getValue();
}

public get lastEditFullReplacement(): boolean {
return this._lastEditFullReplacement;
}

isDirty(): boolean {
return this.textEditorModel.isDirty();
}

public setDirty(dirty: boolean): void {
if (this.dirty === dirty) {
if (this._dirty === dirty) {
return;
}
this.dirty = dirty;
this._dirty = dirty;
this._onDidChangeDirty.fire();
}

Expand All @@ -92,7 +108,8 @@ export class NotebookEditorModel extends EditorModel {
}
}

public updateModel(contentChange?: NotebookContentChange): void {
public updateModel(contentChange?: NotebookContentChange, type?: NotebookChangeType): void {
this._lastEditFullReplacement = false;
if (contentChange && contentChange.changeType === NotebookChangeType.Saved) {
// We send the saved events out, so ignore. Otherwise we double-count this as a change
// and cause the text to be reapplied
Expand All @@ -104,22 +121,42 @@ export class NotebookEditorModel extends EditorModel {
// Request serialization so trusted state is preserved but don't update the model
this.sendNotebookSerializationStateChange();
} else {
// For all other changes, update the backing model with the latest contents
let notebookModel = this.getNotebookModel();
let editAppliedSuccessfully = false;
if (notebookModel && this.textEditorModel && this.textEditorModel.textEditorModel) {
let content = JSON.stringify(notebookModel.toJSON(), undefined, ' ');
let model = this.textEditorModel.textEditorModel;
let endLine = model.getLineCount();
let endCol = model.getLineMaxColumn(endLine);

this.textEditorModel.textEditorModel.applyEdits([{
range: new Range(1, 1, endLine, endCol),
text: content
}]);
if (contentChange && contentChange.cells && contentChange.cells[0]) {
if (type === NotebookChangeType.CellSourceUpdated) {
if (this._notebookTextFileModel.transformAndApplyEditForSourceUpdate(contentChange, this.textEditorModel)) {
editAppliedSuccessfully = true;
}
} else if (type === NotebookChangeType.CellOutputUpdated) {
if (this._notebookTextFileModel.transformAndApplyEditForOutputUpdate(contentChange, this.textEditorModel)) {
editAppliedSuccessfully = true;
}
} else if (type === NotebookChangeType.CellOutputCleared) {
if (this._notebookTextFileModel.transformAndApplyEditForClearOutput(contentChange, this.textEditorModel)) {
editAppliedSuccessfully = true;
}
} else if (type === NotebookChangeType.CellExecuted) {
if (this._notebookTextFileModel.transformAndApplyEditForCellUpdated(contentChange, this.textEditorModel)) {
editAppliedSuccessfully = true;
}
}
}
// If edit was already applied, skip replacing entire text model
if (editAppliedSuccessfully) {
return;
}
this.replaceEntireTextEditorModel(notebookModel, type);
this._lastEditFullReplacement = true;
}
}
}

public replaceEntireTextEditorModel(notebookModel: INotebookModel, type: NotebookChangeType) {
this._notebookTextFileModel.replaceEntireTextEditorModel(notebookModel, type, this.textEditorModel);
}

private sendNotebookSerializationStateChange(): void {
let notebookModel = this.getNotebookModel();
if (notebookModel) {
Expand All @@ -142,6 +179,10 @@ export class NotebookEditorModel extends EditorModel {
get onDidChangeDirty(): Event<void> {
return this._onDidChangeDirty.event;
}

get editorModel() {
return this.textEditorModel;
}
}

export class NotebookInput extends EditorInput {
Expand All @@ -161,6 +202,8 @@ export class NotebookInput extends EditorInput {
private _providersLoaded: Promise<void>;
private _dirtyListener: IDisposable;
private _notebookEditorOpenedTimestamp: number;
private _modelResolveInProgress: boolean = false;
private _modelResolved: Deferred<void> = new Deferred<void>();

constructor(private _title: string,
private resource: URI,
Expand Down Expand Up @@ -283,6 +326,12 @@ export class NotebookInput extends EditorInput {
}

async resolve(): Promise<NotebookEditorModel> {
if (!this._modelResolveInProgress) {
this._modelResolveInProgress = true;
} else {
await this._modelResolved;
return this._model;
}
if (this._model) {
return Promise.resolve(this._model);
} else {
Expand All @@ -296,6 +345,7 @@ export class NotebookInput extends EditorInput {
}
this._model = this.instantiationService.createInstance(NotebookEditorModel, this.resource, textOrUntitledEditorModel);
this.hookDirtyListener(this._model.onDidChangeDirty, () => this._onDidChangeDirty.fire());
this._modelResolved.resolve();
return this._model;
}
}
Expand Down
25 changes: 8 additions & 17 deletions src/sql/workbench/parts/notebook/browser/notebook.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,8 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
if (event) {
event.stopPropagation();
}
if (cell !== this.model.activeCell) {
if (this.model.activeCell) {
this.model.activeCell.active = false;
}
this._model.activeCell = cell;
this._model.activeCell.active = true;
this.detectChanges();
}
this.model.updateActiveCell(cell);
this.detectChanges();
}

//Saves scrollTop value on scroll change
Expand All @@ -192,10 +186,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
}

public unselectActiveCell() {
if (this.model && this.model.activeCell) {
this.model.activeCell.active = false;
this.model.activeCell = undefined;
}
this.model.updateActiveCell(undefined);
this.detectChanges();
}

Expand Down Expand Up @@ -311,10 +302,10 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
editorLoadedTimestamp: this._notebookParams.input.editorOpenedTimestamp
}, this.profile, this.logService, this.notificationService, this.telemetryService);
let trusted = await this.notebookService.isNotebookTrustCached(this._notebookParams.notebookUri, this.isDirty());
model.onError((errInfo: INotification) => this.handleModelError(errInfo));
model.contentChanged((change) => this.handleContentChanged(change));
model.onProviderIdChange((provider) => this.handleProviderIdChanged(provider));
model.kernelChanged((kernelArgs) => this.handleKernelChanged(kernelArgs));
this._register(model.onError((errInfo: INotification) => this.handleModelError(errInfo)));
this._register(model.contentChanged((change) => this.handleContentChanged()));
this._register(model.onProviderIdChange((provider) => this.handleProviderIdChanged(provider)));
this._register(model.kernelChanged((kernelArgs) => this.handleKernelChanged(kernelArgs)));
this._model = this._register(model);
await this._model.loadContents(trusted);
this.setLoading(false);
Expand Down Expand Up @@ -382,7 +373,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
this.notificationService.notify(notification);
}

private handleContentChanged(change: NotebookContentChange) {
private handleContentChanged() {
// Note: for now we just need to set dirty state and refresh the UI.
this.detectChanges();
}
Expand Down
Loading

0 comments on commit 84b3e87

Please sign in to comment.