notebook diff editor perf improvement#112176
Conversation
rheh
left a comment
rheh
left a comment
There was a problem hiding this comment.
A few points - mostly suggestions on removing duplication and improving readability. One question I cannot answer is do these changes require and changes to existing unit tests?
| private _maxComputationTime: number; | ||
| private _renderIndicators: boolean; | ||
| private _enableSplitViewResizing: boolean; | ||
| private _wordWrap: 'off' | 'on' | 'wordWrapColumn' | 'bounded' | undefined; |
There was a problem hiding this comment.
Would make sense to be of type enum
enum wordWrapEnum {
Off,
On,
WordWrapColumn,
Bounded
};
private _wordWrap: wordWrapEnum | null;
| private _enableSplitViewResizing: boolean; | ||
| private _wordWrap: 'off' | 'on' | 'wordWrapColumn' | 'bounded' | undefined; | ||
| private _wordWrapMinified: boolean | undefined; | ||
| private _renderOverviewRuler: boolean; |
There was a problem hiding this comment.
As stated above these are not used? Is this intended?
| this._contextKeyService.createKey('isInEmbeddedDiffEditor', false); | ||
| } | ||
|
|
||
| this._renderOverviewRuler = true; |
There was a problem hiding this comment.
Could roll this up into one-linear
this._renderOverviewRuler = typeof options.renderOverviewRuler !== 'undefined' ? Boolean(options.renderOverviewRuler) : true;
| this._isHandlingScrollEvent = false; | ||
|
|
||
| this._elementSizeObserver = this._register(new ElementSizeObserver(this._containerDomElement, undefined, () => this._onDidContainerSizeChanged())); | ||
| this._elementSizeObserver = this._register(new ElementSizeObserver(this._containerDomElement, options.dimension, () => this._onDidContainerSizeChanged())); |
There was a problem hiding this comment.
nit-pick: arrow function not required:
this._elementSizeObserver = this._register(new ElementSizeObserver(this._containerDomElement, options.dimension, this._onDidContainerSizeChanged);
| result.scrollbar!.verticalHasArrows = false; | ||
| result.extraEditorClassName = 'modified-in-monaco-diff-editor'; | ||
| return result; | ||
| return { |
There was a problem hiding this comment.
Duplicate of lines 1156 - 1162. One could add a private helper function e.g.
function addDimension(result) : editorBrowser.IEditorConstructionOptions {
return {
...result,
dimension: {
height: 0,
width: 0
}
};
Then call the helper from both places e.g.
return addDimension(result);
| */ | ||
| isInEmbeddedEditor?: boolean; | ||
| /** | ||
| * Is the diff editor should render overview ruler |
| readonly notebookEditor: INotebookTextDiffEditor, | ||
| readonly cell: CellDiffViewModel, | ||
| readonly templateData: CellDiffSingleSideRenderTemplate, | ||
| readonly style: 'left' | 'right' | 'full', |
There was a problem hiding this comment.
This could be an enum?
enum styleAlignments {
Left,
Right,
Full
};
| this._register(this._editor); | ||
| this._editor = this.templateData.sourceEditor; | ||
| this._editor.layout({ | ||
| width: (this.notebookEditor.getLayoutInfo().width - 2 * DIFF_CELL_MARGIN) / 2 - 18, |
There was a problem hiding this comment.
The 18 is a magic number. A named constant would help understand what this is for. That said, as if is a copy-paste then we can just skip adding the constant if you do not know.
| buildBody() { | ||
| const body = this.templateData.body; | ||
| this._diffEditorContainer = this.templateData.diffEditorContainer; | ||
| switch (this.style) { |
There was a problem hiding this comment.
This switch statement duplicated above. We could move this into a new function on the shared parent abstraction can call it from both places?
2 participants
This PR fixes #109680. It consists of following major changes:
The diff editor is slower than the native notebook editor, simply because in the diff editor, the amount of monaco editors doubled. The creation and deletion of monaco editors has some cost, even if it's a few miliseconds, rendering 20 monaco editors in a large viewport can cost ~100ms. So the performance of scrolling in the diff editor is definitely slower than scrolling in a notebook editor with the same resource.