Skip to content

Commit d4f4b6e

Browse files
krassowskikrassowski
committed
Improve performance of diagnostics and highlights
1 parent 449922d commit d4f4b6e

File tree

3 files changed

+191
-23
lines changed

3 files changed

+191
-23
lines changed

‎packages/jupyterlab-lsp/src/editor_integration/codemirror.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,9 @@ export abstract class CodeMirrorIntegration
245245
}
246246
}
247247

248+
/**
249+
* @deprecated
250+
*/
248251
protected highlight_range(
249252
range: IEditorRange,
250253
class_name: string

‎packages/jupyterlab-lsp/src/features/diagnostics/diagnostics.ts‎

Lines changed: 100 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,18 @@ export const diagnostics_databases = new WeakMap<
306306
DiagnosticsDatabase
307307
>();
308308

309+
interface IMarkerDefinition {
310+
options: CodeMirror.TextMarkerOptions;
311+
start: IEditorPosition;
312+
end: IEditorPosition;
313+
hash: string;
314+
}
315+
316+
interface IMarkedDiagnostic {
317+
editor: CodeMirror.Editor;
318+
marker: CodeMirror.TextMarker;
319+
}
320+
309321
export class DiagnosticsCM extends CodeMirrorIntegration {
310322
private last_response: lsProtocol.PublishDiagnosticsParams;
311323

@@ -339,7 +351,7 @@ export class DiagnosticsCM extends CodeMirrorIntegration {
339351
}
340352

341353
private unique_editor_ids: DefaultMap<CodeMirror.Editor, number>;
342-
private marked_diagnostics: Map<string, CodeMirror.TextMarker> = new Map();
354+
private marked_diagnostics: Map<string, IMarkedDiagnostic> = new Map();
343355
/**
344356
* Allows access to the most recent diagnostics in context of the editor.
345357
*
@@ -477,6 +489,11 @@ export class DiagnosticsCM extends CodeMirrorIntegration {
477489
this.filterDiagnostics(response.diagnostics)
478490
);
479491

492+
const markerOptionsByEditor = new Map<
493+
CodeMirror.Editor,
494+
IMarkerDefinition[]
495+
>();
496+
480497
diagnostics_by_range.forEach(
481498
(diagnostics: lsProtocol.Diagnostic[], range: lsProtocol.Range) => {
482499
const start = PositionConverter.lsp_to_cm(
@@ -641,23 +658,70 @@ export class DiagnosticsCM extends CodeMirrorIntegration {
641658
.join('\n'),
642659
className: classNames.join(' ')
643660
};
661+
662+
let optionsList = markerOptionsByEditor.get(cm_editor);
663+
if (!optionsList) {
664+
optionsList = [];
665+
markerOptionsByEditor.set(cm_editor, optionsList);
666+
}
667+
optionsList.push({
668+
options,
669+
start: start_in_editor,
670+
end: end_in_editor,
671+
hash: diagnostic_hash
672+
});
673+
}
674+
}
675+
);
676+
677+
for (const [
678+
cmEditor,
679+
markerDefinitions
680+
] of markerOptionsByEditor.entries()) {
681+
// note: could possibly be wrapped in `requestAnimationFrame()`
682+
// at a risk of sometimes having an inconsistent state in database.
683+
// note: using `operation()` significantly improves performance.
684+
// test cases:
685+
// - one cell with 1000 `math.pi` and `import math`; comment out import,
686+
// wait for 1000 diagnostics, then uncomment import, wait for removal:
687+
// - before:
688+
// - diagnostics show up in: 13.6s (blocking!)
689+
// - diagnostics removal in: 13.2s (blocking!)
690+
// - after:
691+
// - diagnostics show up in: 254ms
692+
// - diagnostics removal in: 160.4ms
693+
// - first open of a notebook with 10 cells, each with 88 diagnostics:
694+
// - before: 2.75s (each time)
695+
// - after 208.75ms (order of magnitude faster!)
696+
// - first open of a notebook with 100 cells, each with 1 diagnostic
697+
// this scenario is expected to have no gain (measures overhead)
698+
// - before 280.34ms, 377ms, 399ms
699+
// - after 385.29ms, 301.97ms, 309.4ms
700+
cmEditor.operation(() => {
701+
const doc = cmEditor.getDoc();
702+
for (const definition of markerDefinitions) {
644703
let marker;
645704
try {
646-
marker = cm_editor
647-
.getDoc()
648-
.markText(start_in_editor, end_in_editor, options);
705+
marker = doc.markText(
706+
definition.start,
707+
definition.end,
708+
definition.options
709+
);
649710
} catch (e) {
650711
this.console.warn(
651712
'Marking inspection (diagnostic text) failed:',
652-
diagnostics,
713+
definition,
653714
e
654715
);
655716
return;
656717
}
657-
this.marked_diagnostics.set(diagnostic_hash, marker);
718+
this.marked_diagnostics.set(definition.hash, {
719+
marker,
720+
editor: cmEditor
721+
});
658722
}
659-
}
660-
);
723+
});
724+
}
661725

662726
// remove the markers which were not included in the new message
663727
this.removeUnusedDiagnosticMarkers(markers_to_retain);
@@ -696,14 +760,36 @@ export class DiagnosticsCM extends CodeMirrorIntegration {
696760
}
697761

698762
protected removeUnusedDiagnosticMarkers(to_retain: Set<string>) {
699-
this.marked_diagnostics.forEach(
700-
(marker: CodeMirror.TextMarker, diagnostic_hash: string) => {
701-
if (!to_retain.has(diagnostic_hash)) {
702-
this.marked_diagnostics.delete(diagnostic_hash);
703-
marker.clear();
763+
const toRemoveByEditor = new Map<
764+
CodeMirror.Editor,
765+
{ marker: CodeMirror.TextMarker; hash: string }[]
766+
>();
767+
768+
for (const [
769+
diagnosticHash,
770+
markedDiagnostic
771+
] of this.marked_diagnostics.entries()) {
772+
if (!to_retain.has(diagnosticHash)) {
773+
let diagnosticsList = toRemoveByEditor.get(markedDiagnostic.editor);
774+
if (!diagnosticsList) {
775+
diagnosticsList = [];
776+
toRemoveByEditor.set(markedDiagnostic.editor, diagnosticsList);
704777
}
778+
diagnosticsList.push({
779+
marker: markedDiagnostic.marker,
780+
hash: diagnosticHash
781+
});
705782
}
706-
);
783+
}
784+
785+
for (const [cmEditor, markers] of toRemoveByEditor.entries()) {
786+
cmEditor.operation(() => {
787+
for (const markerData of markers) {
788+
markerData.marker.clear();
789+
this.marked_diagnostics.delete(markerData.hash);
790+
}
791+
});
792+
}
707793
}
708794

709795
remove(): void {

‎packages/jupyterlab-lsp/src/features/highlights.ts‎

Lines changed: 88 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ import { CodeHighlights as LSPHighlightsSettings } from '../_highlights';
1414
import { CodeMirrorIntegration } from '../editor_integration/codemirror';
1515
import { FeatureSettings } from '../feature';
1616
import { DocumentHighlightKind } from '../lsp';
17-
import { IRootPosition, IVirtualPosition } from '../positioning';
17+
import {
18+
IEditorPosition,
19+
IRootPosition,
20+
IVirtualPosition
21+
} from '../positioning';
1822
import { ILSPFeatureManager, PLUGIN_ID } from '../tokens';
1923
import { VirtualDocument } from '../virtual/document';
2024

@@ -23,8 +27,19 @@ export const highlightIcon = new LabIcon({
2327
svgstr: highlightSvg
2428
});
2529

30+
interface IHighlightDefinition {
31+
options: CodeMirror.TextMarkerOptions;
32+
start: IEditorPosition;
33+
end: IEditorPosition;
34+
}
35+
2636
export class HighlightsCM extends CodeMirrorIntegration {
27-
protected highlight_markers: CodeMirror.TextMarker[] = [];
37+
protected highlightMarkers: Map<CodeMirror.Editor, CodeMirror.TextMarker[]> =
38+
new Map();
39+
/*
40+
* @deprecated
41+
*/
42+
protected highlight_markers: CodeMirror.TextMarker[];
2843
private debounced_get_highlight: Debouncer<
2944
lsProtocol.DocumentHighlight[] | undefined
3045
>;
@@ -63,9 +78,14 @@ export class HighlightsCM extends CodeMirrorIntegration {
6378
}
6479

6580
protected clear_markers() {
66-
for (let marker of this.highlight_markers) {
67-
marker.clear();
81+
for (const [cmEditor, markers] of this.highlightMarkers.entries()) {
82+
cmEditor.operation(() => {
83+
for (const marker of markers) {
84+
marker.clear();
85+
}
86+
});
6887
}
88+
this.highlightMarkers = new Map();
6989
this.highlight_markers = [];
7090
}
7191

@@ -78,16 +98,75 @@ export class HighlightsCM extends CodeMirrorIntegration {
7898
return;
7999
}
80100

101+
const highlightOptionsByEditor = new Map<
102+
CodeMirror.Editor,
103+
IHighlightDefinition[]
104+
>();
105+
81106
for (let item of items) {
82107
let range = this.range_to_editor_range(item.range);
83108
let kind_class = item.kind
84109
? 'cm-lsp-highlight-' + DocumentHighlightKind[item.kind]
85110
: '';
86-
let marker = this.highlight_range(
87-
range,
88-
'cm-lsp-highlight ' + kind_class
89-
);
90-
this.highlight_markers.push(marker);
111+
112+
let optionsList = highlightOptionsByEditor.get(range.editor);
113+
114+
if (!optionsList) {
115+
optionsList = [];
116+
highlightOptionsByEditor.set(range.editor, optionsList);
117+
}
118+
optionsList.push({
119+
options: {
120+
className: 'cm-lsp-highlight ' + kind_class
121+
},
122+
start: range.start,
123+
end: range.end
124+
});
125+
}
126+
127+
for (const [
128+
cmEditor,
129+
markerDefinitions
130+
] of highlightOptionsByEditor.entries()) {
131+
// note: using `operation()` significantly improves performance.
132+
// test cases:
133+
// - one cell with 1000 `math.pi` and `import math`; move cursor to `math`,
134+
// wait for 1000 highlights, then move to `pi`:
135+
// - before:
136+
// - highlight `math`: 13.1s
137+
// - then highlight `pi`: 16.6s
138+
// - after:
139+
// - highlight `math`: 160ms
140+
// - then highlight `pi`: 227ms
141+
// - 100 cells with `math.pi` and one with `import math`; move cursor to `math`,
142+
// wait for 1000 highlights, then move to `pi` (this is overhead control,
143+
// no gains expected):
144+
// - before:
145+
// - highlight `math`: 385ms
146+
// - then highlight `pi`: 683 ms
147+
// - after:
148+
// - highlight `math`: 390ms
149+
// - then highlight `pi`: 870ms
150+
cmEditor.operation(() => {
151+
const doc = cmEditor.getDoc();
152+
const markersList: CodeMirror.TextMarker[] = [];
153+
for (const definition of markerDefinitions) {
154+
let marker;
155+
try {
156+
marker = doc.markText(
157+
definition.start,
158+
definition.end,
159+
definition.options
160+
);
161+
} catch (e) {
162+
this.console.warn('Marking highlight failed:', definition, e);
163+
return;
164+
}
165+
markersList.push(marker);
166+
this.highlight_markers.push(marker);
167+
}
168+
this.highlightMarkers.set(cmEditor, markersList);
169+
});
91170
}
92171
};
93172

0 commit comments

Comments
 (0)