Fix viewer-charts context cleanup, implement pooling in blit mode (now default).

Signed-off-by: Andrew Stein <steinlink@gmail.com>

Author: texodus

packages/viewer-charts/src/ts/config.ts

@@ -33,7 +33,27 @@ export const RUNTIME_MODE: "worker" | "inprocess" = "worker";
  *   `ImageBitmap` over the control channel; the host blits the bitmap
  *   into the visible canvas via `drawImage`.
  */
-export const RENDER_BLIT_MODE: "direct" | "blit" = "direct";
+export const RENDER_BLIT_MODE: "direct" | "blit" = "blit";
+
+/**
+ * Number of shared WebGL contexts in pooled blit mode.
+ *
+ * The browser caps live contexts per agent (~16 in Chromium) and
+ * force-loses the oldest past that cap, so a page with more charts than
+ * the cap cannot give each its own context. When `> 0` *and*
+ * `RENDER_BLIT_MODE === "blit"`, every chart borrows one of this many
+ * shared contexts (round-robin, sticky for the chart's lifetime) instead
+ * of allocating its own — decoupling live-context count from chart
+ * count. N charts render through K = this many contexts; the scheduler
+ * serializes renders that land on the same context.
+ *
+ * `0` disables pooling (every chart gets its own context — the original
+ * behavior). Pooling never applies to `"direct"` mode, which renders
+ * into the host's transferred visible canvas and is permanently 1:1 with
+ * a context; keep pages that need more than ~16 simultaneous charts on
+ * `"blit"`.
+ */
+export const RENDER_CONTEXT_POOL_SIZE: number = 4;
 
 /**
  * Strict-mode validation for `BufferPool.upload`.

packages/viewer-charts/src/ts/plugin/plugin.ts

@@ -125,6 +125,22 @@ const GLOBAL_STYLES = (() => {
     return [sheet];
 })();
 
+/**
+ * Process-global GL presentation strategy, shared by *every* chart-type
+ * plugin in this renderer. Seeded from the build-time {@link RENDER_BLIT_MODE}
+ * and overridable at runtime via
+ * {@link HTMLPerspectiveViewerWebGLPluginElement.setBlitMode}.
+ *
+ * Module scope (not per-instance) on purpose: blit-vs-direct is a
+ * whole-renderer decision — in `"blit"` mode the worker shares a pool of
+ * GL contexts across all charts ([webgl/context-pool.ts]), so a page
+ * can't sensibly mix strategies per chart. Every per-chart-type subclass
+ * registered in [index.ts] reads this one value when it builds its
+ * renderer, so setting it once (before the first chart renders) applies
+ * to all of them.
+ */
+let BLIT_MODE: "direct" | "blit" = RENDER_BLIT_MODE;
+
 export class HTMLPerspectiveViewerWebGLPluginElement
     extends HTMLElement
     implements IPerspectiveViewerPlugin
@@ -139,7 +155,6 @@ export class HTMLPerspectiveViewerWebGLPluginElement
     private _rendererPromise: Promise<RendererTransport> | null = null;
     private _rawEventForwarder: RawEventForwarder | null = null;
     private _generation = 0;
-    private _renderBlitMode: "direct" | "blit" = RENDER_BLIT_MODE;
     private _resetClickAbort: AbortController | null = null;
 
     /**
@@ -257,13 +272,41 @@ export class HTMLPerspectiveViewerWebGLPluginElement
             return this._rendererPromise;
         }
 
-        this._rendererPromise = this._buildRenderer(view).then((r) => {
-            this._renderer = r;
-            this._setupInteraction(r);
-            return r;
-        });
+        // `_buildRenderer` is async — it awaits `getClient`, `getTable`,
+        // and a full worker handshake — so a `disconnectedCallback`
+        // (toggle / chart-type switch) frequently lands *before* this
+        // `.then` runs. At that point `this._renderer` is still null,
+        // so `delete()`'s `if (this._renderer)` teardown is skipped and
+        // the freshly-built transport (and its WebGL context) would be
+        // assigned to a detached element and leak — one context per
+        // raced toggle, until the browser evicts the oldest.
+        //
+        // `delete()` sets `_rendererPromise = null`, so promise identity
+        // is the dispose signal: if `this._rendererPromise` no longer
+        // points at *this* build when it resolves, the element was
+        // deleted (or a reconnect started a newer build) and we destroy
+        // the orphan instead of adopting it. Promise identity — not
+        // `_generation`, which every `draw`/`update` bumps — is the
+        // right token: a rapid draw→update must NOT tear down the
+        // single in-flight build it shares via this memoized promise.
+        const p: Promise<RendererTransport> = this._buildRenderer(view).then(
+            (r) => {
+                if (this._rendererPromise !== p) {
+                    r.destroy();
+                    throw new Error("renderer disposed during init");
+                }
+
+                this._renderer = r;
+                this._setupInteraction(r);
+                return r;
+            },
+        );
 
-        return this._rendererPromise;
+        // Swallow the dispose rejection so it doesn't surface as an
+        // unhandled rejection; `_drawImpl` catches it and bails.
+        p.catch(() => {});
+        this._rendererPromise = p;
+        return p;
     }
 
     /**
@@ -350,15 +393,28 @@ export class HTMLPerspectiveViewerWebGLPluginElement
             },
             pluginConfig: this._pluginConfig,
             defaultChartType: this._chartType.default_chart_type,
-            renderBlitMode: this._renderBlitMode,
+            renderBlitMode: BLIT_MODE,
         });
 
         return transport;
     }
 
-    setBlitMode(mode: "direct" | "blit") {
-        console.assert(this._initialized, "Already initialized");
-        this._renderBlitMode = mode;
+    /**
+     * Select the GL presentation strategy for *all* chart-type plugins
+     * in this renderer. Static + process-global: the value is shared by
+     * every per-chart-type subclass, so it must be set once before the
+     * charts that should use it build their renderers (a renderer reads
+     * {@link BLIT_MODE} at construction in `_buildRenderer`; charts
+     * already built keep their mode until torn down and rebuilt).
+     *
+     * - `"direct"` — each chart owns a GL context 1:1 with its visible
+     *   canvas (lowest latency; bounded by the browser's ~16-context
+     *   cap).
+     * - `"blit"` — charts render off-screen and share a pool of GL
+     *   contexts ([webgl/context-pool.ts]), so a page can exceed the cap.
+     */
+    static setBlitMode(mode: "direct" | "blit") {
+        BLIT_MODE = mode;
     }
 
     get_static_config(): PluginStaticConfig {
@@ -499,7 +555,15 @@ export class HTMLPerspectiveViewerWebGLPluginElement
 
     private async _drawImpl(view: View): Promise<void> {
         const gen = ++this._generation;
-        const renderer = await this._ensureRenderer(view);
+        let renderer: RendererTransport;
+        try {
+            renderer = await this._ensureRenderer(view);
+        } catch {
+            // Renderer was disposed mid-init (element disconnected
+            // during the async build) — nothing to draw.
+            return;
+        }
+
         if (this._generation !== gen) {
             return;
         }

packages/viewer-charts/src/ts/render/scheduler.ts

@@ -177,6 +177,41 @@ export function deferIfDraining(
     ops.push(op);
 }
 
+/**
+ * Drop every scheduler reference to `glManager` — called from
+ * `WebGLContextManager`'s owning renderer at teardown, *before*
+ * `glManager.destroy()` loses the GL context.
+ *
+ * Without this, a frame queued for the next RAF (an `Entry` in
+ * `pending`) or a `deferIfDraining` op still parked in `deferred`
+ * would survive the destroy and, on the next `drain()`, drive
+ * `fullRender` / `present` against a context-lost manager. In blit
+ * mode that surfaces as `endFrame`'s `transferToImageBitmap` throwing
+ * "Cannot transfer to ImageBitmap because WebGL context is lost" —
+ * logged as "scheduler: present failed". In direct mode it's a wasted
+ * paint against a dead context.
+ *
+ * Outstanding waiters for a destroyed manager are resolved (not
+ * rejected): the caller asked to tear the chart down, so its awaited
+ * `draw()` should observe a clean no-op rather than an error it would
+ * have to suppress. An in-flight `present()` (manager in `inFlight`)
+ * is left to unwind on its own — its `finally` clause will not find a
+ * `deferred` entry to flush, and the manager is already gone from
+ * `pending`, so it cannot re-enqueue.
+ */
+export function unregister(glManager: WebGLContextManager): void {
+    const entry = pending.get(glManager);
+    if (entry) {
+        pending.delete(glManager);
+        for (const w of entry.waiters) {
+            w.resolve();
+        }
+    }
+
+    deferred.delete(glManager);
+    inFlight.delete(glManager);
+}
+
 /**
  * Test-only: clear pending state. Production callers must not use
  * this — outstanding waiters are silently dropped.
@@ -230,31 +265,39 @@ export function _resetForTest(): void {
 async function drain(): Promise<void> {
     const snapshot = Array.from(pending.values());
     pending.clear();
-    const ready: Entry[] = [];
+
+    // Group ready entries by the GL context behind each manager. Under
+    // pooled blit mode many managers share one context (one drawing
+    // buffer): their renders MUST serialize — interleaving two charts'
+    // paints into one canvas would corrupt the bitmap the first ships.
+    // In the default 1:1 mode every manager has its own backend, so
+    // every group has exactly one entry and all groups run in parallel,
+    // exactly as before pooling.
+    const groups = new Map<number, Entry[]>();
     for (const entry of snapshot) {
-        try {
-            // Apply any dimension change recorded by
-            // `glManager.requestResize` *before* the paint, in the
-            // same un-yielded synchronous Phase 1 loop. This pairs
-            // the canvas-clearing `canvas.width = N` assignment
-            // with the immediately-following `_fullRender`, so the
-            // browser's compositor only ever observes the canvas
-            // post-paint. In direct/in-process modes the visible
-            // canvas IS the GL canvas, and a clear-without-matching-
-            // paint in the previous task would otherwise present an
-            // empty frame to the user.
-            entry.glManager.applyPendingResize();
-            entry.fullRender();
-            ready.push(entry);
-        } catch (err) {
-            console.error("scheduler: fullRender threw", err);
+        // The browser force-loses the oldest context when a page
+        // exceeds its per-agent WebGL context cap (~16). A frame queued
+        // before that eviction would paint + present against a dead
+        // context; skip it and settle its waiters cleanly rather than
+        // letting `endFrame`'s `transferToImageBitmap` throw.
+        if (entry.glManager.isContextLost()) {
             for (const w of entry.waiters) {
-                w.reject(err);
+                w.resolve();
             }
+
+            continue;
+        }
+
+        const key = entry.glManager.backendId;
+        const group = groups.get(key);
+        if (group) {
+            group.push(entry);
+        } else {
+            groups.set(key, [entry]);
         }
     }
 
-    await Promise.all(ready.map(present));
+    await Promise.all(Array.from(groups.values()).map(presentGroup));
 
     // Now (and only now) clear rafId. If new requests landed during
     // this drain, schedule the next RAF.
@@ -264,16 +307,51 @@ async function drain(): Promise<void> {
     }
 }
 
+/**
+ * Render every entry sharing one GL context, sequentially: each chart
+ * gets the shared drawing buffer to itself for a full
+ * `beginFrame` → `fullRender` → `awaitGpuFence` → `endFrame` (present)
+ * cycle before the next chart touches it. Groups for *different*
+ * contexts run concurrently (via `Promise.all` in `drain`), so K pooled
+ * contexts give K-way parallelism and the 1:1 default keeps full
+ * cross-chart overlap.
+ *
+ * The synchronous prefix of each group (`beginFrame` + `fullRender`)
+ * runs before its first `awaitGpuFence` yields, so when groups run
+ * concurrently all first-chart draws are still submitted before any
+ * fence wait — preserving the GPU overlap the old two-phase drain had.
+ */
+async function presentGroup(entries: Entry[]): Promise<void> {
+    for (const entry of entries) {
+        await present(entry);
+    }
+}
+
 async function present(entry: Entry): Promise<void> {
-    // Mark this glManager as in-flight *synchronously*, before the
-    // first await. `Promise.all(ready.map(present))` calls each
-    // `present` synchronously to collect its returned promise, so
-    // every entry's glManager is registered in `inFlight` before
-    // any fence-wait yields and before any sibling message handler
-    // can run. Mutations posted by sibling handlers (resize, clear)
-    // route through `deferIfDraining` and queue into `deferred`
-    // until the `finally` block flushes them.
+    // Mark this glManager as in-flight *synchronously*, before the first
+    // await, so sibling message handlers' canvas mutations (resize,
+    // clear) route through `deferIfDraining` into `deferred` until the
+    // `finally` flushes them.
     inFlight.add(entry.glManager);
+    try {
+        // Apply the dimension change and (in pooled mode) reset shared
+        // GL state in the same un-yielded step as the paint that fills
+        // the buffer. Pairing the canvas-clearing `canvas.width = N`
+        // with `_fullRender` keeps the compositor from ever observing a
+        // cleared-but-unpainted canvas (visible flicker in direct/in-
+        // process modes, where the visible canvas IS the GL canvas).
+        entry.glManager.beginFrame();
+        entry.fullRender();
+    } catch (err) {
+        console.error("scheduler: fullRender threw", err);
+        for (const w of entry.waiters) {
+            w.reject(err);
+        }
+
+        flushDeferred(entry.glManager);
+        return;
+    }
+
     try {
         await entry.glManager.awaitGpuFence();
         entry.glManager.endFrame();
@@ -296,21 +374,26 @@ async function present(entry: Entry): Promise<void> {
             w.reject(err);
         }
     } finally {
-        // Bitmap shipped (or error reported). Re-open the canvas to
-        // mutations and flush any deferred ops in arrival order.
-        // Deferred ops may call `requestRender`; the resulting
-        // entry queues into `pending` and the drain's tail check
-        // picks it up for the next RAF.
-        inFlight.delete(entry.glManager);
-        const ops = deferred.get(entry.glManager);
-        if (ops) {
-            deferred.delete(entry.glManager);
-            for (const op of ops) {
-                try {
-                    op();
-                } catch (err) {
-                    console.error("scheduler: deferred op threw", err);
-                }
+        flushDeferred(entry.glManager);
+    }
+}
+
+/**
+ * Re-open a glManager's canvas to mutations and flush any ops deferred
+ * during its present, in arrival order. Deferred ops may call
+ * `requestRender`; the resulting entry queues into `pending` and the
+ * drain's tail check picks it up for the next RAF.
+ */
+function flushDeferred(glManager: WebGLContextManager): void {
+    inFlight.delete(glManager);
+    const ops = deferred.get(glManager);
+    if (ops) {
+        deferred.delete(glManager);
+        for (const op of ops) {
+            try {
+                op();
+            } catch (err) {
+                console.error("scheduler: deferred op threw", err);
             }
         }
     }