Skip to content

Commit f9e0a04

Browse files
krassowskikrassowski
committed
Fix issues related to fronted querying too often.
There were three issues: - the `notebookTracker.currentChanged` signal was being connected on each re-render of React widget and re-renders were triggered by update to state that was defined in the callback leading to O(n^2) accumulation of callbacks. - the `kernelChanged` signal was being connected on each change of the notebook leading to O(n) accumulation of callbacks. - the sidebar panel could have been created multiple times if invoked from command palette - O(1) accumulation of callbacks.
1 parent b22e7f8 commit f9e0a04

File tree

2 files changed

+89
-53
lines changed

2 files changed

+89
-53
lines changed

‎src/index.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import {
55
import { INotebookTracker } from '@jupyterlab/notebook';
66
import { LabIcon } from '@jupyterlab/ui-components';
77
import { ICommandPalette } from '@jupyterlab/apputils';
8-
import { ILauncher } from '@jupyterlab/launcher';
98
import { KernelUsagePanel } from './panel';
109
import tachometer from '../style/tachometer.svg';
1110

@@ -19,24 +18,25 @@ namespace CommandIDs {
1918
const plugin: JupyterFrontEndPlugin<void> = {
2019
id: 'kernelusage:plugin',
2120
requires: [ICommandPalette, INotebookTracker],
22-
optional: [ILauncher],
2321
autoStart: true,
2422
activate: (
2523
app: JupyterFrontEnd,
2624
palette: ICommandPalette,
27-
notebookTracker: INotebookTracker,
28-
launcher: ILauncher | null
25+
notebookTracker: INotebookTracker
2926
) => {
3027
const { commands, shell } = app;
3128
const category = 'Kernel Resource';
3229

33-
async function createPanel(): Promise<KernelUsagePanel> {
34-
const panel = new KernelUsagePanel({
35-
widgetAdded: notebookTracker.widgetAdded,
36-
currentNotebookChanged: notebookTracker.currentChanged
37-
});
38-
shell.add(panel, 'right', { rank: 200 });
39-
return panel;
30+
let panel: KernelUsagePanel | null = null;
31+
32+
function createPanel() {
33+
if (!panel || panel.isDisposed) {
34+
panel = new KernelUsagePanel({
35+
widgetAdded: notebookTracker.widgetAdded,
36+
currentNotebookChanged: notebookTracker.currentChanged
37+
});
38+
shell.add(panel, 'right', { rank: 200 });
39+
}
4040
}
4141

4242
commands.addCommand(CommandIDs.getKernelUsage, {

‎src/widget.tsx

Lines changed: 78 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { useState } from 'react';
1+
import React, { useState, useEffect } from 'react';
22
import { ISignal } from '@lumino/signaling';
33
import { ReactWidget, ISessionContext } from '@jupyterlab/apputils';
44
import { IChangedArgs } from '@jupyterlab/coreutils';
@@ -32,6 +32,19 @@ type Usage = {
3232

3333
const POLL_INTERVAL_SEC = 5;
3434

35+
type KernelChangeCallback = (
36+
_sender: ISessionContext,
37+
args: IChangedArgs<
38+
Kernel.IKernelConnection | null,
39+
Kernel.IKernelConnection | null,
40+
'kernel'
41+
>
42+
) => void;
43+
let kernelChangeCallback: {
44+
callback: KernelChangeCallback;
45+
panel: NotebookPanel;
46+
} | null = null;
47+
3548
const KernelUsage = (props: {
3649
widgetAdded: ISignal<INotebookTracker, NotebookPanel | null>;
3750
currentNotebookChanged: ISignal<INotebookTracker, NotebookPanel | null>;
@@ -44,61 +57,84 @@ const KernelUsage = (props: {
4457

4558
useInterval(async () => {
4659
if (kernelId && panel.isVisible) {
47-
requestUsage(kernelId).then(usage => setUsage(usage));
60+
requestUsage(kernelId)
61+
.then(usage => setUsage(usage))
62+
.catch(() => {
63+
console.warn(`Request failed for ${kernelId}. Kernel restarting?`);
64+
});
4865
}
4966
}, POLL_INTERVAL_SEC * 1000);
5067

51-
const requestUsage = (kid: string) =>
52-
requestAPI<any>(`get_usage/${kid}`).then(data => {
68+
const requestUsage = (kid: string) => {
69+
return requestAPI<any>(`get_usage/${kid}`).then(data => {
5370
const usage: Usage = {
5471
...data.content,
5572
kernelId: kid,
5673
timestamp: new Date()
5774
};
5875
return usage;
5976
});
77+
};
6078

61-
props.currentNotebookChanged.connect(
62-
(sender: INotebookTracker, panel: NotebookPanel | null) => {
63-
panel?.sessionContext.kernelChanged.connect(
64-
(
65-
_sender: ISessionContext,
66-
args: IChangedArgs<
67-
Kernel.IKernelConnection | null,
68-
Kernel.IKernelConnection | null,
69-
'kernel'
70-
>
71-
) => {
72-
/*
73-
const oldKernelId = args.oldValue?.id;
74-
if (oldKernelId) {
75-
const poll = kernelPools.get(oldKernelId);
76-
poll?.poll.dispose();
77-
kernelPools.delete(oldKernelId);
78-
}
79-
*/
80-
const newKernelId = args.newValue?.id;
81-
if (newKernelId) {
82-
setKernelId(newKernelId);
83-
const path = panel?.sessionContext.session?.model.path;
84-
setPath(path);
85-
requestUsage(newKernelId).then(usage => setUsage(usage));
86-
}
79+
useEffect(() => {
80+
const createKernelChangeCallback = (panel: NotebookPanel) => {
81+
return (
82+
_sender: ISessionContext,
83+
args: IChangedArgs<
84+
Kernel.IKernelConnection | null,
85+
Kernel.IKernelConnection | null,
86+
'kernel'
87+
>
88+
) => {
89+
const newKernelId = args.newValue?.id;
90+
if (newKernelId) {
91+
setKernelId(newKernelId);
92+
const path = panel?.sessionContext.session?.model.path;
93+
setPath(path);
94+
requestUsage(newKernelId).then(usage => setUsage(usage));
8795
}
88-
);
89-
if (panel?.sessionContext.session?.id !== kernelId) {
90-
if (panel?.sessionContext.session?.kernel?.id) {
91-
const kernelId = panel?.sessionContext.session?.kernel?.id;
92-
if (kernelId) {
93-
setKernelId(kernelId);
94-
const path = panel?.sessionContext.session?.model.path;
95-
setPath(path);
96-
requestUsage(kernelId).then(usage => setUsage(usage));
97-
}
96+
};
97+
};
98+
99+
const notebookChangeCallback = (
100+
sender: INotebookTracker,
101+
panel: NotebookPanel | null
102+
) => {
103+
if (panel === null) {
104+
// Ideally we would switch to a new "select a notebook to get kernel
105+
// usage" screen instead of showing outdated info.
106+
return;
107+
}
108+
if (kernelChangeCallback) {
109+
kernelChangeCallback.panel.sessionContext.kernelChanged.disconnect(
110+
kernelChangeCallback.callback
111+
);
112+
}
113+
kernelChangeCallback = {
114+
callback: createKernelChangeCallback(panel),
115+
panel
116+
};
117+
panel.sessionContext.kernelChanged.connect(kernelChangeCallback.callback);
118+
119+
if (panel.sessionContext.session?.kernel?.id !== kernelId) {
120+
const kernelId = panel.sessionContext.session?.kernel?.id;
121+
if (kernelId) {
122+
setKernelId(kernelId);
123+
const path = panel.sessionContext.session?.model.path;
124+
setPath(path);
125+
requestUsage(kernelId).then(usage => setUsage(usage));
98126
}
99127
}
100-
}
101-
);
128+
};
129+
props.currentNotebookChanged.connect(notebookChangeCallback);
130+
return () => {
131+
props.currentNotebookChanged.disconnect(notebookChangeCallback);
132+
// In the ideal world we would disconnect kernelChangeCallback from
133+
// last panel here, but this can lead to a race condition. Instead,
134+
// we make sure there is ever only one callback active by holding
135+
// it in a global state.
136+
};
137+
}, [kernelId]);
102138

103139
if (kernelId) {
104140
if (usage) {

0 commit comments

Comments
 (0)