Skip to content
This repository has been archived by the owner on Feb 18, 2023. It is now read-only.

Commit

Permalink
Fix issues related to fronted querying too often.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
krassowski committed Dec 12, 2022
1 parent b22e7f8 commit f9e0a04
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 53 deletions.
22 changes: 11 additions & 11 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
import { INotebookTracker } from '@jupyterlab/notebook';
import { LabIcon } from '@jupyterlab/ui-components';
import { ICommandPalette } from '@jupyterlab/apputils';
import { ILauncher } from '@jupyterlab/launcher';
import { KernelUsagePanel } from './panel';
import tachometer from '../style/tachometer.svg';

Expand All @@ -19,24 +18,25 @@ namespace CommandIDs {
const plugin: JupyterFrontEndPlugin<void> = {
id: 'kernelusage:plugin',
requires: [ICommandPalette, INotebookTracker],
optional: [ILauncher],
autoStart: true,
activate: (
app: JupyterFrontEnd,
palette: ICommandPalette,
notebookTracker: INotebookTracker,
launcher: ILauncher | null
notebookTracker: INotebookTracker
) => {
const { commands, shell } = app;
const category = 'Kernel Resource';

async function createPanel(): Promise<KernelUsagePanel> {
const panel = new KernelUsagePanel({
widgetAdded: notebookTracker.widgetAdded,
currentNotebookChanged: notebookTracker.currentChanged
});
shell.add(panel, 'right', { rank: 200 });
return panel;
let panel: KernelUsagePanel | null = null;

function createPanel() {
if (!panel || panel.isDisposed) {
panel = new KernelUsagePanel({
widgetAdded: notebookTracker.widgetAdded,
currentNotebookChanged: notebookTracker.currentChanged
});
shell.add(panel, 'right', { rank: 200 });
}
}

commands.addCommand(CommandIDs.getKernelUsage, {
Expand Down
120 changes: 78 additions & 42 deletions src/widget.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from 'react';
import React, { useState, useEffect } from 'react';
import { ISignal } from '@lumino/signaling';
import { ReactWidget, ISessionContext } from '@jupyterlab/apputils';
import { IChangedArgs } from '@jupyterlab/coreutils';
Expand Down Expand Up @@ -32,6 +32,19 @@ type Usage = {

const POLL_INTERVAL_SEC = 5;

type KernelChangeCallback = (
_sender: ISessionContext,
args: IChangedArgs<
Kernel.IKernelConnection | null,
Kernel.IKernelConnection | null,
'kernel'
>
) => void;
let kernelChangeCallback: {
callback: KernelChangeCallback;
panel: NotebookPanel;
} | null = null;

const KernelUsage = (props: {
widgetAdded: ISignal<INotebookTracker, NotebookPanel | null>;
currentNotebookChanged: ISignal<INotebookTracker, NotebookPanel | null>;
Expand All @@ -44,61 +57,84 @@ const KernelUsage = (props: {

useInterval(async () => {
if (kernelId && panel.isVisible) {
requestUsage(kernelId).then(usage => setUsage(usage));
requestUsage(kernelId)
.then(usage => setUsage(usage))
.catch(() => {
console.warn(`Request failed for ${kernelId}. Kernel restarting?`);
});
}
}, POLL_INTERVAL_SEC * 1000);

const requestUsage = (kid: string) =>
requestAPI<any>(`get_usage/${kid}`).then(data => {
const requestUsage = (kid: string) => {
return requestAPI<any>(`get_usage/${kid}`).then(data => {
const usage: Usage = {
...data.content,
kernelId: kid,
timestamp: new Date()
};
return usage;
});
};

props.currentNotebookChanged.connect(
(sender: INotebookTracker, panel: NotebookPanel | null) => {
panel?.sessionContext.kernelChanged.connect(
(
_sender: ISessionContext,
args: IChangedArgs<
Kernel.IKernelConnection | null,
Kernel.IKernelConnection | null,
'kernel'
>
) => {
/*
const oldKernelId = args.oldValue?.id;
if (oldKernelId) {
const poll = kernelPools.get(oldKernelId);
poll?.poll.dispose();
kernelPools.delete(oldKernelId);
}
*/
const newKernelId = args.newValue?.id;
if (newKernelId) {
setKernelId(newKernelId);
const path = panel?.sessionContext.session?.model.path;
setPath(path);
requestUsage(newKernelId).then(usage => setUsage(usage));
}
useEffect(() => {
const createKernelChangeCallback = (panel: NotebookPanel) => {
return (
_sender: ISessionContext,
args: IChangedArgs<
Kernel.IKernelConnection | null,
Kernel.IKernelConnection | null,
'kernel'
>
) => {
const newKernelId = args.newValue?.id;
if (newKernelId) {
setKernelId(newKernelId);
const path = panel?.sessionContext.session?.model.path;
setPath(path);
requestUsage(newKernelId).then(usage => setUsage(usage));
}
);
if (panel?.sessionContext.session?.id !== kernelId) {
if (panel?.sessionContext.session?.kernel?.id) {
const kernelId = panel?.sessionContext.session?.kernel?.id;
if (kernelId) {
setKernelId(kernelId);
const path = panel?.sessionContext.session?.model.path;
setPath(path);
requestUsage(kernelId).then(usage => setUsage(usage));
}
};
};

const notebookChangeCallback = (
sender: INotebookTracker,
panel: NotebookPanel | null
) => {
if (panel === null) {
// Ideally we would switch to a new "select a notebook to get kernel
// usage" screen instead of showing outdated info.
return;
}
if (kernelChangeCallback) {
kernelChangeCallback.panel.sessionContext.kernelChanged.disconnect(
kernelChangeCallback.callback
);
}
kernelChangeCallback = {
callback: createKernelChangeCallback(panel),
panel
};
panel.sessionContext.kernelChanged.connect(kernelChangeCallback.callback);

if (panel.sessionContext.session?.kernel?.id !== kernelId) {
const kernelId = panel.sessionContext.session?.kernel?.id;
if (kernelId) {
setKernelId(kernelId);
const path = panel.sessionContext.session?.model.path;
setPath(path);
requestUsage(kernelId).then(usage => setUsage(usage));
}
}
}
);
};
props.currentNotebookChanged.connect(notebookChangeCallback);
return () => {
props.currentNotebookChanged.disconnect(notebookChangeCallback);
// In the ideal world we would disconnect kernelChangeCallback from
// last panel here, but this can lead to a race condition. Instead,
// we make sure there is ever only one callback active by holding
// it in a global state.
};
}, [kernelId]);

if (kernelId) {
if (usage) {
Expand Down

0 comments on commit f9e0a04

Please sign in to comment.