diff --git a/CONTEXT.md b/CONTEXT.md index c94439c6..94eafe08 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -25,12 +25,12 @@ A reserved **Stack Entry** that names ordered children for future blending or ag _Avoid_: framebuffer layer until the rendering behavior exists **Resource Resolver**: -The store-agnostic boundary that turns structural **Render Stack** inputs into stable loaded resources for renderers. -_Avoid_: viewer-local cache, periodic snapshotter +The store-agnostic **and renderer-agnostic** boundary that turns structural **Render Stack** inputs into stable loaded resources for renderers. Owns the cache, request supersession, cancellation, streaming partials, eviction, and world bounds. Lives in `@spatialdata/core`: it depends on neither deck.gl nor React, and is consumed by every **Renderer Adapter** ([ADR 0004](docs/adr/0004-resource-resolver-owned-by-core.md)). +_Avoid_: viewer-local cache, periodic snapshotter, deck-coupled engine **Renderer Adapter**: -The code that turns resolved resources plus entry props into Viv/deck layer instances. -_Avoid_: state store +The code that turns resolved resources plus entry props into renderer output — deck.gl `Layer`s, Viv image props, or a three.js/WebGPU pass. Pure and synchronous: it is handed resolved state and cannot start a load. +_Avoid_: state store, "the renderer" (there is more than one) **Runtime Attachment**: An unsaved function or object supplied by the host application alongside a **Render Stack**, such as `hostLayerResolver`, `onFeatureHover`, `onFeatureClick`, raw deck handlers, DOM portal targets, or deck layer factories. @@ -60,10 +60,35 @@ _Avoid_: calling the points tooltip a "feature tooltip"; pulling tooltip values Transient, interactive emphasis of *all* points belonging to one chosen **Points Feature** (e.g. hovering a gene in the catalog panel brightens its points and de-emphasises the rest). It is ephemeral **runtime** state — a cheap recolor/re-emphasis of the already-resident batch with **no reload or refilter** — not part of the serializable colour encoding or **Render Stack** config. Distinct from deck.gl `autoHighlight`, which emphasises a single *picked point*, not a whole feature. _Avoid_: persisting highlight into the Stack Entry; conflating with per-object `autoHighlight`; reloading geometry on highlight change +**Resolution**: +The state of one loaded resource of a **Spatial Entry**, as a value: `idle | loading | ready | failed`. `loading` carries `partial` (what *this* load has produced so far — the streaming scan's growing buffer) and `stale` (the last good value from the *previous* load). `failed` may also carry `stale`. Resolutions are **per-resource, not per-entry** — a shapes entry with a broken tooltip column must still draw its geometry. + +`stale` is a **retention, not a guarantee**: *while it is retained*, a failed or in-flight refine keeps drawing rather than blanking. It is released on eviction and on non-retryable failure (see **Resource Ceiling**), after which the resource is simply not renderable and the UI shows the **Spatial Entry Error** instead. Callers must handle the no-stale case; they may not assume a previously-ready resource stays drawable. +_Avoid_: a status enum beside a value field; a tri-state (`undefined | null | T`) plus a `loaded` boolean; a per-entry `Result`; treating `stale` as a permanent fallback + +**Spatial Entry Error**: +A structured, typed **domain failure** of a resource — not an exception, not a missing layer, not a `console.error`. Every case carries what the UI needs to explain itself (`coordinate-system-not-found` carries `availableCoordinateSystems`; `points-preload-too-large` carries `rowCount` and `maxRows`) plus a `retryable` flag that gates a Retry affordance. `retryable` — not the union — is what prevents a failed scan settling permanently. +_Avoid_: a bare `Error`; swallowing into `console.error`; modelling *absence* as failure (a points element with no `feature_key` is `ready(null)`, a settled fact, not a failure) + +**Entry Notice**: +A non-fatal domain fact about a **successfully** resolved entry — preload truncated, selection served from memory, catalog is a resident-subset preview, image channel defaults fell back. A channel distinct from **Spatial Entry Error**, so healthy data never renders as an error. +_Avoid_: overloading the failure channel; a `degraded` resolution status + +**Encoded Tier / Decoded Tier**: +The two forms a loaded payload takes: compressed bytes as fetched (parquet file or row-group bytes; a zarr chunk before its codec), and the materialized form (an Arrow table; a typed-array chunk). Both ingest paths — zarr and parquet — have both tiers, and a cache may be bounded independently at each. Dropping a **Decoded Tier** entry while retaining its **Encoded Tier** trades memory for a re-decode; the trade is only sound where decode is off the main thread ([ADR 0005](docs/adr/0005-memory-accounting-before-management.md)). +_Avoid_: "the cache" (there are four); conflating a parquet whole-file cache with a per-chunk one + +**Resource Ceiling**: +The byte bound a working set must fit. When it would be exceeded, the policy is *degrade to fit* — coarsen, cap, or evict — not crash and not silent truncation. Distinct from the current points **memory cap**, which is a row count with no accounting behind it. Deferred until an actual out-of-memory case can be provoked; measurement comes first ([ADR 0005](docs/adr/0005-memory-accounting-before-management.md)). +_Avoid_: quota, limit, budget-as-a-guess + ## Relationships - A **Render Stack** contains zero or more ordered **Stack Entries**. - A **Spatial Entry** is resolved by a **Resource Resolver** before a **Renderer Adapter** creates Viv/deck output. +- A **Spatial Entry**'s resources are each held as a **Resolution**; a failed one carries a **Spatial Entry Error**, a successful one may carry **Entry Notices**. +- A **Resource Resolver** is shared across **Renderer Adapters** (deck.gl, Viv, three.js/WebGPU, headless); it knows about none of them. +- A cached payload exists in an **Encoded Tier**, a **Decoded Tier**, or both; a **Resource Ceiling** bounds them. - A **Host Overlay** is saved as a descriptor and materialized by the host application at runtime. - A **Group Entry** may order child entries, but does not yet imply framebuffer or blending behavior. - A **Runtime Attachment** may observe or materialize stack entries, but is not part of saved **Render Stack** config. diff --git a/docs/adr/0001-render-stack-owned-by-layers.md b/docs/adr/0001-render-stack-owned-by-layers.md index 9e9be7f5..8e5455cd 100644 --- a/docs/adr/0001-render-stack-owned-by-layers.md +++ b/docs/adr/0001-render-stack-owned-by-layers.md @@ -1,5 +1,12 @@ # Render Stack Owned By Layers -The canonical ordered render description lives in `@spatialdata/layers` as `RenderStack`, while `@spatialdata/vis` adapts that stack into React, Viv, and deck.gl rendering. Host overlays are saved as descriptors and resolved by the host application at runtime, so MDV can interleave scatter, gates, selections, and SpatialData entries without storing raw deck layer instances or reintroducing parallel `layerOrder` / `stackOrder` state. +> **Amended by [ADR 0004](0004-resource-resolver-owned-by-core.md) (2026-07-14).** +> The **package-placement** claim below is superseded: `RenderStack` lives in +> `@spatialdata/core`, not `@spatialdata/layers`. The **Resource Resolver** takes a +> Render Stack as input and is renderer-agnostic, so dependency direction forces the +> move. Everything else in this ADR stands unchanged — host overlays as descriptors, +> no parallel `layerOrder` state, MobX outside the contract. + +The canonical ordered render description lives in `@spatialdata/layers` as `RenderStack` *(**historical** — superseded by ADR 0004: it lives in `@spatialdata/core`; `layers` and `vis` retain re-exports as compatibility shims)*, while `@spatialdata/vis` adapts that stack into React, Viv, and deck.gl rendering. Host overlays are saved as descriptors and resolved by the host application at runtime, so MDV can interleave scatter, gates, selections, and SpatialData entries without storing raw deck layer instances or reintroducing parallel `layerOrder` / `stackOrder` state. MobX may be used by MDV-facing control UI, but it is not part of the `@spatialdata/layers` contract or the default renderer API. MobX-controlled panels should be explicit control islands, especially while adopting React Compiler, because observable direct editing and automatic memoization have different assumptions. diff --git a/docs/adr/0004-resource-resolver-owned-by-core.md b/docs/adr/0004-resource-resolver-owned-by-core.md new file mode 100644 index 00000000..e53d84f7 --- /dev/null +++ b/docs/adr/0004-resource-resolver-owned-by-core.md @@ -0,0 +1,257 @@ +# Resource Resolver Owned By Core + +**Status:** proposed +**Amends:** [ADR 0001](0001-render-stack-owned-by-layers.md) — its *package-placement* claim only; its substance stands. +**Builds on:** [ADR 0002](0002-spatially-aware-vector-loading.md), [ADR 0003](0003-points-render-resource.md) + +The **Resource Resolver** — the module that turns structural **Render Stack** inputs +into stable loaded resources — lives in `@spatialdata/core`, not +`@spatialdata/layers`. `@spatialdata/layers` keeps the deck.gl **Renderer Adapter**. + +## Context + +`CONTEXT.md` already draws this line, and we drifted from it: + +> **Resource Resolver**: The **store-agnostic** boundary that turns structural +> Render Stack inputs into stable loaded resources **for renderers**. +> +> **Renderer Adapter**: The code that turns resolved resources plus entry props +> into Viv/deck layer instances. *Avoid: state store.* +> +> A **Spatial Entry** is resolved by a **Resource Resolver** *before* a +> **Renderer Adapter** creates Viv/deck output. + +Two steps, explicitly, with the state store on the resolver side and explicitly +forbidden on the renderer side. + +Today the Resource Resolver does not exist as a module. Its responsibilities are +split between `useLayerData.ts` (1,873 lines, `@spatialdata/vis`) and +`PointsDataEngine.ts` (940 lines, `@spatialdata/layers`). Neither is where the +domain model says it should be. + +### Consequence 1 — the god-hook + +The four **Spatial Entry** kinds are not separate modules. They are four +implementations braided line-by-line through one body, joined at six shared +mutable mechanisms: a `toLoad` tuple whose six booleans every push site must +spell out, one `layerLoadStates` map, one six-Map `loadedDataRef`, one revision +counter, one `getLayers` loop, and four kind-switches. Points work and shapes +work cannot proceed concurrently without conflicting. + +### Consequence 2 — a second Resource Resolver already exists + +This is the decisive one, and it is not a matter of taste. + +`tgpu-htj2k` renders 1.5 Gpx HTJ2K imagery from a real Xenium SpatialData store +through three.js/WebGPU **today**. It depends on `@spatialdata/core` and +`zarrextra`, and its ADR-0010 excludes the rest by name: + +> "No deck.gl / React enters the render path … `@spatialdata/layers` / +> `@spatialdata/vis` / `@spatialdata/avivatorish` are excluded." + +It reached into `@spatialdata/core` for a resolution layer, found only `readZarr` +plus element discovery, and **hand-rolled the entire thing**: `Select`, +`Selection`, `Resolve`, `Tileset`, `TileCache`, `loadScheduler`, Nyquist LOD, +byte budgets. Roughly ten files, with tests. + +That is a second Resource Resolver, written from scratch because the first one +was locked behind deck.gl. The duplication is being paid for now, in another +repo. + +### The resolver is not deck-shaped + +Every type in the per-kind resource maps is a `core` type — `PointsLoadResult`, +`PointsFeatureCatalog`, row codes, `ShapesRenderData`, `ShapesTooltipMetadata`. +Not one is a deck type. Every case of the failure union is a `core` concept — +coordinate-system-not-found, element-not-found, unsupported-format, +points-preload-too-large, decode-failed, worker-unavailable. Not one is a deck +concept. + +## Decision + +1. **The Resource Resolver lives in `@spatialdata/core`.** Framework-free: no + React, no deck.gl, no Viv. It owns **element-level** resource lifecycle — + preload, feature catalog, row codes, geometry, tooltip metadata, fill colour — + including their cache, request supersession, cancellation, streaming partials, + and eviction. It also owns entry resolution (element + transform to the active + coordinate system) and world bounds. + + **It does not own tile-level lifecycle.** See §"Lifecycle split" below. + +2. **Per-kind resolvers behind one interface** — points, shapes, images, labels — + not one monolithic engine. A single engine would move the four-way kind-switch + down a package rather than dissolve it, and would keep points and shapes in + one file. (This resolves open question 2 of + [`docs/plans/layer-data-engine-decomposition.md`](../plans/layer-data-engine-decomposition.md).) + +3. **`Resolution` and `SpatialEntryError` are `core` types.** Failure is + **per-resource**, not per-entry: a shapes entry with a broken tooltip column + must still draw its geometry. See [ADR 0005](0005-memory-accounting-before-management.md)'s + sibling note and `CONTEXT.md` for the vocabulary. + +4. **The Renderer Adapter stays in `@spatialdata/layers`.** It owns `project()` + (prebuilt datum arrays, feature-state runtime, render-resource identity memos) + and `render()` → `Layer[]`. Identity-stable memoisation is a **deck + requirement** — deck tears a layer down when its data identity changes — so it + belongs on the renderer side, memoising against core's per-entry snapshot + identity. + +5. **`RenderStack` moves to `core`.** Forced by dependency direction: the + Resolver takes a Render Stack as input. This also lands where the schemas + already wanted to be — `renderStack.ts` and `spatialLayerProps.ts` are zod + persistence schemas that `vis` re-exports verbatim because MDV consumes them + as a *data contract*, and `core` already owns `schemas/` and depends on zod. + + **Migration contract:** `@spatialdata/core` becomes canonical. + `@spatialdata/layers` and `@spatialdata/vis` **retain their existing re-exports + as compatibility shims** — `RenderStack`, the render-stack schemas, + `SpatialLayerProps`, and `migrateSpatialLayerProps` keep their current import + paths. No consumer import moves. Removing the shims is a separate, deliberate + deprecation, coordinated with MDV; it is not part of this work. + +6. **The image loader is a port.** `createImageLoader` closes over the React + `VivLoaderRegistry` context. `core` defines the port; `vis` supplies the + adapter. This is the one genuine ports-and-adapters dependency; everything + else is local-substitutable (tests stub elements as plain object literals). + +7. **No runtime dependency enters `core`.** No Effect, no `neverthrow`, no + TanStack in a public signature. `core` is the dependency root for + `tgpu-htj2k` as well as `layers`, and that repo's engine core is deliberately + dependency-free. A library may be used *inside* a resolver's implementation + (see the `RequestSlot` spike) but must not appear in `core`'s interface. + +## Lifecycle split — who reconciles what + +The Resolver is **not** the universal request-lifecycle owner. Tile scheduling +stays with the renderer, deliberately. + +| Concern | Owner | Why | +|---|---|---| +| **Element-level** resource lifecycle — preload, catalog, row codes, geometry, tooltip metadata, fill colour | **Resource Resolver** (`core`) | Identical for every renderer. Duplicating it is what `tgpu-htj2k` was forced to do. | +| **Tile-level** lifecycle — which tiles, at what LOD, when to abort on pan | **Renderer Adapter** | Genuinely renderer-specific. deck's `TileLayer` and `tgpu-htj2k`'s `Select` + `loadScheduler` (Nyquist + frustum + nearest-first) implement *different, correct* policies for *different* viewports and budgets. | +| The **loader** facet — `capabilities`, `loadInBounds()` | **`core`**, shared | Already ADR 0003's decision. This is the seam both tile schedulers call. | +| The **byte-level chunk/table cache** | **`core`**, shared ([ADR 0005](0005-memory-accounting-before-management.md)) | Registered downward at the store layer, so it is shared regardless of who schedules. | + +A renderer-neutral tile scheduler would be a lowest-common-denominator abstraction +serving neither deck nor WebGPU well. **That** is the reinvention risk, and this +ADR declines it. The Renderer Adapter supplies viewport state and drives its own +tile requests through the shared loader; `core` owns everything above the tile. + +If a future kind genuinely needs resolver-level viewport reconcile, the Resolver's +input grows an optional `viewport?` field that existing resolvers ignore. + +## Non-goals + +**A renderer-neutral render abstraction is not a goal.** Deck-agnostic *Resource +Resolver* does not mean deck-agnostic *render path*. The point of the Renderer +Adapter seam is that the deck adapter can lean **all the way into** deck's +ecosystem — `@geoarrow/deck.gl-geoarrow`'s `GeoArrowScatterplotLayer` / +`GeoArrowPolygonLayer`, Viv's `MultiscaleImageLayer` / `XRLayer`. Quarantining deck +behind an adapter means *not leaking deck into `core`*; it is the opposite of +*reimplementing deck*. (ADR 0003 already says as much: *"Layers owns +deck.gl-geoarrow integration … Core must not import deck.gl."*) + +**Batch representation is decided per encoding, not by policy.** Delegate to an +existing deck layer wherever one can consume the encoding directly; hand-roll flat +typed arrays wherever it cannot. Both are expected, and ADR 0003's strategy +registry — dispatching on `loader.capabilities.kind` — is already the mechanism. +Concretely: wild-type shapes are **WKB in parquet** with geopandas `geo` metadata, +*not* GeoArrow, so a decode is unavoidable; and points are x/y **columns**, not +encoded geometry, so `GeoArrowScatterplotLayer` buys nothing over the existing +columnar `ScatterplotLayer` path. The only real requirement on any batch is that it +be **transferable across the worker seam** and **not allocate one JS object per +vertex**. + +**This ADR does not decide Viv-vs-own-renderer for images.** That is a Renderer +Adapter question. The images resolver is thin (loader construction, omero channel +defaults, multi-selection stats); the heavy lifting — multiscale pyramid, tile +fetch, codec — is in `zarrextra`, whose `VivCompatiblePixelSource` **already serves +both Viv and `tgpu-htj2k` today**. The shared seam for images therefore already +exists and sits *below* the Resolver. A renderer-agnostic Resolver buys the 3D +option without spending it: go all-in on WebGPU for 3D and the Resolver does not +change; stay on Viv for 2D and it does not change either. + +## What ADR 0001 retains + +Its substance is unchanged: + +- **Host Overlays** are saved as descriptors and resolved by the host application + at runtime. MDV still interleaves scatter, gates and selections without storing + raw deck layer instances. +- No parallel `layerOrder` / `stackOrder` state. +- MobX is not part of the contract. **MobX Control Islands** remain explicit, + especially under React Compiler. +- `@spatialdata/vis` adapts the stack into React, Viv and deck.gl rendering. + +Only this sentence changes: *"The canonical ordered render description lives in +`@spatialdata/layers`"* → **it lives in `@spatialdata/core`.** + +## Renderer Adapters — why the seam is real + +"One adapter means a hypothetical seam. Two adapters means a real one." + +| Adapter | Status | +|---|---| +| deck.gl (`@spatialdata/layers`) | shipping | +| Viv image props (`@spatialdata/layers`) | shipping — and *already a distinct output shape*: images do not produce `Layer[]`, which is why every interface sketch grew an awkward optional `buildVivProps?` | +| three.js / TSL (`tgpu-htj2k`) | shipping | +| headless (no renderer) | shipping — and today cannot reach any of this logic | + +Four adapters, three of them live. The Viv wart is worth dwelling on: it was the +renderer seam asserting itself through an interface that refused to acknowledge +it. + +## Out of scope + +- **Group Entry compositing / blend rendering ops.** `CONTEXT.md` reserves + **Group Entry** and says plainly: *"Avoid: framebuffer layer until the + rendering behavior exists."* That still holds. `tgpu-htj2k`'s + `splatDensity.ts` is a GPU splat-by-blending **primitive**, not a prototype of + Render Stack compositing, and adopting it would mean adopting the whole WebGPU + stack. A group layer-hierarchy — where blend ops would live — is expected to + become live before long, and may well be built on WebGPU. The Renderer Adapter + seam is what makes that approachable later. **Nothing in this ADR builds it, + and no framebuffer hook is added in anticipation of it.** + +- **Viewport-driven loading needs no resolver change.** Tile scheduling is + renderer-owned by design — see §"Lifecycle split". Points already work this way: + deck's `TileLayer` drives tile requests through `PointsLoader.loadInBounds()`, + exactly as ADRs 0002/0003 decided, and the Resolver never sees a viewport. Shapes + get it the same way. + +- **ADR 0003's "FBO-based render caching"** remains deferred, on its own terms. + +## Cross-repo consequence + +`tgpu-htj2k` ADR-0008's cross-repo layering table assigns `Select` / `Tileset` / +`TileCache` / render backends to `tgpu-htj2k` as their **permanent** home. That +was decided when the only SpatialData.ts equivalent lived behind deck.gl. If the +Resolver moves to `core`, the table should be renegotiated: **resolution to +`core`, render backends stay.** This ADR does not unilaterally amend another +repo's decision; it flags it as owed. + +## Consequences + +- `@spatialdata/core` gains a **stateful, subscribable module**. This is a change + of character for a package that is today models + loaders + workers. It is the + right change — `CONTEXT.md`'s Resource Resolver *is* a store concept — but it + should be made deliberately, not slid into. +- The resolver becomes testable **headless**, with no deck.gl and no GL context. +- `@spatialdata/vis`'s public surface is preserved for MDV via a compat shim; do + not silently relocate an exported type out of `vis`. +- The unmanaged caches already living in `core` (`parquetTableBytes`, + `parquetTableCache` — both unbounded, never evicted) gain an owner. See + [ADR 0005](0005-memory-accounting-before-management.md). +- `useLayerData` collapses to a thin React binding: create, reconcile, + `useSyncExternalStore`, project. The three `eslint-disable react-hooks/refs` + render-phase ref writes and the `'use no memo'` React-Compiler opt-outs go with + it. + +## Provenance + +Surfaced during an architecture review on the +`claude/codebase-architecture-refactor-36013b` branch, 2026-07-14. The interface +was designed four ways in parallel under four different constraints (minimise the +interface; maximise extensibility; optimise for the caller; error-as-value spine). +All four independently produced per-kind resolvers behind a uniform interface with +per-resource resolutions — the convergence is the evidence for decisions 2 and 3. diff --git a/docs/adr/0005-memory-accounting-before-management.md b/docs/adr/0005-memory-accounting-before-management.md new file mode 100644 index 00000000..5a43939e --- /dev/null +++ b/docs/adr/0005-memory-accounting-before-management.md @@ -0,0 +1,225 @@ +# Memory Accounting Before Memory Management + +**Status:** proposed +**Related:** [ADR 0004](0004-resource-resolver-owned-by-core.md), [ADR 0002](0002-spatially-aware-vector-loading.md) + +Adopt byte-level **memory accounting** now; adopt a **Resource Ceiling** only when +measurement justifies it. Land the three rungs that fix things demonstrably broken +today; defer the architecture for a problem we have not yet measured. + +## Context + +SpatialData.ts has a memory *policy* and no memory *accounting*. + +`DEFAULT_POINTS_MEMORY_CAP` is **4,000,000 rows** — a row count, not a byte count, +applied to one element kind. Nothing anywhere measures what is actually resident. +Grepping `packages/*/src` for `lru | evict | maxBytes | memoryBudget | memoryLimit` +returns exactly one hit outside a comment: `PointsDataEngine.evict()`, which is +keyed on **element unload**, not memory pressure. That is the entire eviction +machinery in the repository. + +### The two ingest paths have the same two-tier shape, and neither is managed + +| | **Encoded tier** (compressed bytes) | **Decoded tier** (typed arrays / Arrow) | +|---|---|---| +| **zarr** (images, labels) | *nothing exists* | fizarrita's `ChunkCache` seam — **empty** | +| **parquet** (points, shapes) | `parquetTableBytes` — **unbounded, never evicted** | `parquetTableCache` — **unbounded, never evicted** | + +Parquet holds **both** tiers for the same file, simultaneously, forever: double +memory, zero eviction benefit. Zarr holds **neither**, so every tile pays a network +round-trip *and* a re-decode. + +### fizarrita has no cache + +Its contract is the whole of: + +```ts +interface ChunkCache { + get(key: string): Chunk | undefined + set(key: string, value: Chunk): void +} +``` + +No `delete`, no `size`, no `clear`. It can insert and look up; it can never evict, +enumerate, or measure. And `ensureCodecWorkers()` calls `enableWorkerChunkDecode()` +with **no options**, so `cache` is `undefined` and fizarrita falls back to its +`NULL_CACHE` no-op. We use fizarrita purely as **codec offload** — getting +OpenJPH/OpenJPEG WASM off the main thread. The cache is plumbed through the types +end to end and nothing ever passes one. + +The seam is real, exported, documented, and free: `enableWorkerChunkDecode({ cache })` +will accept any `{get, set}` object today with no changes to fizarrita or +`zarrextra`. + +### Prior art: `tgpu-htj2k` + +```ts +/** Anything holding resident host memory can report it in bytes (mirrors TypedArray). */ +export interface MemoryReporting { + readonly byteLength: number; +} +``` + +One number, named so it **structurally matches `TypedArray`** — every typed array +satisfies it for free, with no import. That ergonomic trick is the whole design. + +Two things to inherit carefully: + +- Over there it is **purely observational**. Every read ends in a HUD string. The + one place bytes drive behaviour (`TileCache.set`'s eviction loop) takes bytes as + a *parameter*, not from the interface. **The interface and the enforcement are + disconnected.** We should not inherit that: whatever we bound, bound it *through* + the numbers we report. +- **A scalar cannot express tiers.** They get away with it because only the GPU + tier is theirs (zarrita owns the compressed tier; the decoded tier is transient, + dropped after upload). SpatialData.ts owns **all** the tiers, plus a worker heap + a synchronous getter cannot see. + +Their `TileCache` — ~100 lines, framework-free, byte-bounded LRU, generic over +payload, with a `dispose` hook — is directly reusable. + +## Decision + +Adopt in rungs. **Land 1–3. Do not build 4–5 until measurement justifies them.** + +### 1. Adopt `MemoryReporting` — the scalar only + +`{ readonly byteLength: number }`. No policy, no eviction, no tiers. Just the +ability to answer *"how many bytes am I holding?"* This is the one thing that +cannot be over-engineered, and everything below is gated on it. + +### 2. Bound the two caches that are already unbounded + +Byte-bounded LRU over `parquetTableBytes` and `parquetTableCache`. This is +**fixing a leak, not building an architecture**. + +Fix in the same pass: `parquetTableCache` stores the promise *before* it settles — +correct, genuine in-flight dedup — but never cleans up a rejection, so a single +transient fetch failure caches a rejected promise for that path **permanently**. + +### 3. Fill the empty chunk-cache seam + +`enableWorkerChunkDecode({ cache })` with a byte-bounded LRU. This is a **pure win +today**: there is currently no chunk cache at all, so every tile re-fetches and +re-decodes. + +--- + +> **Stop here until measurement says otherwise.** + +--- + +### 4. *(deferred)* Encoded tier; evict-decoded-keep-encoded + +Hold compressed bytes, drop decoded payloads, re-decode on demand. + +- **Viable on zarr.** Decode is already on the worker pool; encoded HTJ2K is + roughly 10–50× smaller than the decoded typed array. The trade converts a + network round-trip into a worker task. Note it *requires* adding the encoded + tier first — today, evicting a decoded chunk costs a refetch, because nothing + caches encoded bytes above the store. +- **Dangerous on parquet.** `readParquet` + `tableFromIPC` run **synchronously on + the main thread** for shapes and for every `loadParquetTable` call. Evicting a + decoded table costs a whole-file main-thread WASM decode to restore — that *is* + the jank. Gated on moving shapes decode onto the worker, and on caching at + **row-group** rather than whole-file granularity (the machinery already exists: + `readParquetRowGroupBytesByGroupIndex`). + +### 5. *(deferred)* Tiered `ResidencyReport`, a global Resource Ceiling, degrade-to-fit + +Do not build the tier breakdown until something needs to **act** on the difference +between tiers — which is rung 4. Do not build a ceiling until a real OOM can be +provoked. + +## Rationale for deferring 4–5 + +`tgpu-htj2k` **built and unit-tested** `selectWithinBudget` — a degrade-to-fit +ceiling — and then **never called it in production**. Its ADR-0010 defers it +*"until an actual OOM (e.g. a grazing oblique strip) can be provoked"*, because +the geometry (Nyquist + frustum + LOD gradient) already bounds the working set to +roughly screen size. The budget is for a pathological case they have not hit. + +That is the discipline, in writing, from the people who would most have enjoyed +building the budget solver. Rungs 1–3 fix things that are broken today. Rungs 4–5 +are architecture for a problem we have not measured. + +## Where the authority lives + +In `@spatialdata/core`, with the **Resource Resolver** ([ADR 0004](0004-resource-resolver-owned-by-core.md)). +It needs **no new package and no dependency inversion**: it registers caches +downward through injection points that already exist. + +- zarr decoded → `enableWorkerChunkDecode({ cache })` (exists, unused) +- zarr encoded → a caching `zarr.Readable` wrapper at `createPrefixedStore`'s layer + (rung 4 only) +- parquet, both tiers → direct ownership in `VTableSource` +- points resident batches → the points resolver's own entries + +fizarrita and the parquet sources become **clients** of the authority, never +authorities themselves. + +## Notes for the implementer + +- **An encoded-bytes cache cannot live at fizarrita's seam.** Raw bytes are fetched + on the main thread and then *transferred* — neutered — into the worker; the main + thread loses them. `ChunkCache.set` takes a decoded `Chunk`. The only place + compressed chunk bytes are visible is `arr.store.get(chunkPath)`, i.e. the store + layer, i.e. `createPrefixedStore`. +- **Beware store identity.** fizarrita builds cache keys as + `store_${N}:${arr.path}:${chunkKey}`, where `N` comes from a `WeakMap` on the + **store object instance**. `createPrefixedStore` returns a **fresh object literal + on every call**, so two prefixed views over the same root get different `store_N` + and would double-cache. Hold a stable prefixed-store instance per element. +- **Neither seam gives in-flight dedup.** fizarrita checks the cache while building + its task list and writes it only after the worker returns, so two concurrent + requests for the same chunk both fetch and both decode. Key pending promises + yourself — `parquetTableCache` is the in-repo precedent, and also the cautionary + tale (see the rejection-poisoning bug above). +- **`Resolution.stale` needs a drop policy, and it bounds the `Resolution` + contract.** A `failed` resolution holding `stale: PointsLoadResult` pins roughly + 48 MB indefinitely. Today's code leaks the same memory implicitly; the type makes + it a *named, typed, easy-to-keep-forever* field. + + **Policy:** drop `stale` on eviction, and on a **non-retryable** failure (the + value can never be superseded, so retaining it only helps the current frame). + Retain it across a **retryable** failure and across an in-flight refine. + + This is a deliberate qualification of the `Resolution` contract, and `CONTEXT.md` + states it the same way: **`stale` is a retention, not a guarantee.** "A failed + refine does not blank the view" holds *while `stale` is retained*. Once released, + the resource is not renderable and the UI shows the **Spatial Entry Error** + instead. Consumers must handle the no-stale case; they may not assume a + previously-ready resource stays drawable. +- **Fill-value chunks are cached too.** fizarrita caches a full zero-filled typed + array per *absent* chunk — a memory trap for sparse arrays. + +## Owed upstream to fizarrita + +Worth filing alongside the `zarrextra` asks already listed in +`tgpu-htj2k/docs/zarrextra-worker-decode.md`: + +1. **`probeDecompressedSize` does not recognise `imagecodecs_jpeg2k` or HTJ2K.** Its + compression sniff knows only `gzip|zlib|blosc|zstd|lz4|bz2|lzma|snappy`, so for + JP2K-backed images it takes the "not compressed" branch and returns the + *compressed* byte length as the decompressed size, then feeds that to + `inferChunkShape`. Usually it fails to divide cleanly and falls back to the + metadata shape harmlessly — but it can emit spurious `chunk_shape does not match` + warnings and, worst case, adopt a bogus inferred shape. **This affects our + imagery.** +2. **Two extra store round-trips per tile.** Every `getWorker` call re-reads + `zarr.json`/`.zarray` *and* runs `probeActualChunkShape` (another `store.get`, + plus up to five one-past-the-end probes). The probe runs **before** the cache is + consulted, so even a populated chunk cache would not eliminate it. +3. **No in-flight dedup on the chunk path.** +4. **No `AbortSignal`.** `GetWorkerOptions` has no `signal`, and `zarrextra`'s + `rejectOnAbort` only settles the promise early — the fetch and the worker decode + run to completion regardless. + +## Consequences + +- Memory becomes **test-assertable** for the first time. +- The points "memory cap" can become a real byte ceiling instead of a magic row + count, and a shapes cap becomes possible at all. +- Zarr tiles stop re-fetching on every pan. +- We take on a `MemoryReporting` obligation on new caches. Keep it cheap: maintain + a running total on insert/evict rather than scanning residents per read. diff --git a/docs/plans/layer-data-engine-decomposition.md b/docs/plans/layer-data-engine-decomposition.md index 0d95852a..8d7aa109 100644 --- a/docs/plans/layer-data-engine-decomposition.md +++ b/docs/plans/layer-data-engine-decomposition.md @@ -1,7 +1,23 @@ # LayerDataEngine decomposition — moving orchestration out of the vis god-hook -**Status:** proposed (not started) -**Related:** [ADR 0002](../adr/0002-spatially-aware-vector-loading.md), [ADR 0003](../adr/0003-points-render-resource.md), [points preload & feature filter status](points-preload-feature-filter-status.md) +> **Status: SUPERSEDED (2026-07-14).** The diagnosis in this document stands and is +> still worth reading — the god-hook, the mis-placed orchestration layer, the +> unreachable-headless problem. **Its target home is wrong.** +> +> This plan proposes a `LayerDataEngine` in `@spatialdata/layers`. That is one +> package too high: the orchestration layer is the **Resource Resolver**, it is +> renderer-agnostic, and it belongs in `@spatialdata/core`. A second Resource +> Resolver already exists in `tgpu-htj2k` precisely because `core` did not offer +> one and `layers` is behind deck.gl. +> +> It also leaves open question 2 ("one engine, or per-type sub-engines?"), which is +> now answered: **per-kind**. +> +> Superseded by **[ADR 0004 — Resource Resolver Owned By Core](../adr/0004-resource-resolver-owned-by-core.md)** +> and **[Resource Resolver — implementation handoff](resource-resolver-handoff.md)**. + +**Status:** superseded — see banner above +**Related:** [ADR 0002](../adr/0002-spatially-aware-vector-loading.md), [ADR 0003](../adr/0003-points-render-resource.md), [ADR 0004](../adr/0004-resource-resolver-owned-by-core.md), [points preload & feature filter status](points-preload-feature-filter-status.md) This plan addresses a structural problem surfaced while trying to rebase the oversized points-loading branch onto current `main`: **substantial data-loading, diff --git a/docs/plans/resource-resolver-handoff.md b/docs/plans/resource-resolver-handoff.md new file mode 100644 index 00000000..760daddb --- /dev/null +++ b/docs/plans/resource-resolver-handoff.md @@ -0,0 +1,250 @@ +# Resource Resolver — implementation handoff + +**Status:** ready for implementation +**Decisions:** [ADR 0004 — Resource Resolver Owned By Core](../adr/0004-resource-resolver-owned-by-core.md), [ADR 0005 — Memory Accounting Before Management](../adr/0005-memory-accounting-before-management.md) +**Supersedes:** [layer-data-engine-decomposition.md](layer-data-engine-decomposition.md) +**Vocabulary:** [CONTEXT.md](../../CONTEXT.md) — *Resource Resolver, Renderer Adapter, Spatial Entry, Resolution, Spatial Entry Error, Entry Notice, Encoded/Decoded Tier, Resource Ceiling* + +Read the two ADRs first. This document is sequencing, not rationale. + +--- + +## The shape + +```text +core Resource Resolver reconcile · cache · supersede · stream · evict · bounds + per-kind: Points / Shapes / Images / Labels + Resolution · SpatialEntryError · EntryNotice + RenderStack (moved from layers) + │ + ├──► layers deck Renderer Adapter project() → renderInput · render() → Layer[] + │ │ PointsLayer · shapesLayer · LabelsLayer + │ └──► vis React binding (useSyncExternalStore) · panels · Viv passthrough + │ + ├──► tgpu-htj2k three.js/TSL Renderer Adapter (separate repo, already live) + │ + └──► headless no renderer +``` + +**Phase separation inside a resolver** — this is the load-bearing part: + +| Phase | Purity | When | May start I/O? | +|---|---|---|---| +| `plan(ctx)` | pure, sync | commit only | no — *returns* task descriptors | +| `load(task, ctx)` | async | commit only | **yes — the only place** | +| `project(ctx)` | pure, sync | end of reconcile | no | +| `render(state, opts)` | pure, sync | **during React render** | no — *is handed no engine handle* | + +`render()` receives a frozen resolved state and nothing that can start work. That +makes today's `void engine.ensureMatchingFeaturesLoaded(...)` inside `getLayers()` +**a type error**, not a code-review note. The `queueMicrotask(() => this.notify())` +defence in `PointsDataEngine` disappears with it. + +--- + +## Sequence + +### Step 0 — shared contracts (land first, land alone) + +`packages/core/src/engine/{resolution,errors}.ts`. The types, plus the two small +functions that are inseparable from them. No imports beyond `core`'s own. + +- `Resolution` — `idle | loading{partial?, stale?, progress?} | ready{value} | failed{error, stale?}` +- `SpatialEntryError` — discriminated union; every case carries `message` + + `retryable`, plus its own structured payload +- `EntryNotice` — the non-fatal channel +- `toSpatialEntryError(cause, ctx)` — the single classifier; the *one* place a + throw becomes a value + +Also here: `fromResult()`, a three-liner lifting `getTransformation`'s existing +`Result` into a `Resolution`. This is why +`Resolution` lives in `core` — the `Result` it lifts is already there. + +> **Do not** put `Resolution` in `zarrextra`. Do not push it down into `core`'s leaf +> loaders — they keep throwing, and the resolver classifies at the seam. + +### Step 1 — the resolver interface + four thin adapters (shared; the fork point) + +Extract the interface **from the shape `PointsDataEngine` already has**, generalised. +Write `Shapes` / `Images` / `Labels` resolvers as **thin adapters holding today's +Maps and calling today's functions**. Behaviour-identical. No load changes, no +race fixes, no memory work. + +`useLayerData` becomes a loop over resolvers instead of a switch over kinds. Keep +its 17-member public surface intact behind a compat shim — MDV consumes it. + +**This is the commit that unblocks parallel work.** After it lands, the tracks below +touch different files. + +Move in the same step (mechanical, no behaviour change): +- `renderStack.ts`, `spatialLayerProps.ts` → `core` (zod schemas; `core` already has zod). + Re-export from `layers` and `vis` so MDV's imports don't move. + +### Step 2 — three independent tracks + +Assign these to different people. They do not conflict. + +--- + +#### Track A — Points state model + +1. **`RequestSlot`** in `core`. One module replacing four hand-rolled + dedup/supersede/settle implementations. **Supersession by record identity** + (`if (this.current !== myRecord) return`), never by value comparison. Owns the + `AbortSignal`. `error` is a state, not a `console.error`. +2. `PointsEntry`'s 18 mutable fields become four typed slots: `preload`, `catalog`, + `rowCodes`, `matching`. +3. **`retryable` + `retry()`.** This — not the union — is what fixes the + permanently-settled catalog failure. Do not skip it. +4. Threading the `AbortSignal` to the worker (punchlist D8). The worker protocol has + no cancel message today. + +**Races this must close** (all currently live, none reachable by the existing +845-line spec — write the tests through `RequestSlot`): + +| # | Trigger | Symptom | +|---|---|---| +| R1 | cap drag 4M → 8M → 4M | stale `finally` wipes the live load's markers → second concurrent decode | +| R2 | deselect then re-select a feature mid-scan | two scans, same signature, corrupting each other's progress; loser's result silently dropped | +| R3 | raise the cap during a scan | served by the smaller scan; extra rows never fetched | +| R5 | filter toggled while preload is in flight | 4M row-codes overwrite an 8M preload → **row misalignment** | + +5. **The partial-overlay flash** (punchlist D10), independently landable: one + `GrowingPointsResource` per scan whose **loader identity is fixed for the scan's + lifetime**, plus a `resourceRevision` prop so `PointsLayer` re-reads without + resetting. One deck layer per *(entry, selection)*, not per *(entry, phase)* — so + settling is not a teardown either. Zero teardowns per scan instead of N. + +**Spike, behind the `RequestSlot` interface:** implement the slot twice — plain, and +Effect `Stream` + `Fiber` — for the **matching scan only**. Effect stays *inside* the +implementation; nothing leaks into a public signature (ADR 0004 §7 — `core` is also +`tgpu-htj2k`'s dependency root). **Kill criterion, agreed up front:** drop Effect +unless it wins on *all three* of — supersession correctness under two concurrent +scans; interruption that actually reaches the worker; fewer lines to set up a race in +a test. A tie means the plain slot wins. + +--- + +#### Track B — Shapes + +**Needs nothing from Track A. Can start immediately, in parallel.** Viewport-driven +loading requires no resolver change — it lives behind `loadInBounds()` inside deck's +`TileLayer`, exactly as ADRs 0002/0003 decided. + +1. **The loader seam.** Mirror ADR 0003 for shapes: `CoreShapesLoader` + (`capabilities` + `loadInBounds`), `ShapesBatch`, `ShapesRenderResource = + { element, loader }`, strategy dispatch, and a `ShapesLayer` `CompositeLayer` that + **loads inside itself** the way `PointsLayer` does. + + > Non-blocking is a *consequence*, not a feature. Move the load inside the + > composite and the modal overlay disappears — which is precisely why `isBlocking` + > already treats points differently from shapes. + + **First: delete or fix `loadShapesInBounds`.** It exists, ignores `bounds`, `zoom` + and `columns`, does a full-element load, echoes the bounds back, and stamps + `loadMode: 'full-filter'`. Zero callers, zero tests. It lies to anyone reading the + interface. + + The seam lands with **one** adapter — a full-load loader honestly reporting + `supportsViewportTiles: false`. The tiled loader is the *second* adapter, and is + gated on a GeoParquet artifact (ADR 0002 marks shapes tiling *Future*). + +2. **The batch representation.** `ShapePolygon = Array>` — one + JS array object per vertex — is neither transferable nor cheap, which is *why* + `VShapesSource` contains zero references to the worker while `VPointsSource` + imports the whole worker client. + + **The requirement is only this:** the batch must be **transferable across the + worker seam** and must **not allocate one JS object per vertex**. Nothing more is + mandated. + + **Delegate where you can; hand-roll where you can't** (ADR 0004, Non-goals). The + encoding decides, and ADR 0003's strategy registry — dispatching on + `loader.capabilities.kind` — is already the mechanism: + + - Wild-type shapes are **WKB in parquet** with geopandas `geo` metadata, *not* + GeoArrow. A decode is unavoidable. Whether you decode into GeoArrow buffers + (and hand them to `GeoArrowPolygonLayer`) or into flat `positions` / + `polygonIndices` typed arrays (and hand them to a binary `PolygonLayer`) is an + open call — the decode cost is the same, and GeoArrow's layout *is* flat typed + arrays with a schema. Prefer delegation if `deck.gl-geoarrow` can carry our + feature-state, filtering and picking needs; **hand-rolled flat arrays are a + sanctioned outcome if it cannot.** Establish this before building the batch. + - **Points stay columnar.** They are x/y *columns*, not encoded geometry, so + `GeoArrowScatterplotLayer` buys nothing over the existing `ScatterplotLayer` + path. Do not convert them. (`geoarrow-binary` remains a stub for a reason.) + + This work also unblocks ADR 0005 rung 4 on the parquet path, and shrinks the + picking buffer — which is the same problem as "keep hover live with no settle + delay", not a separate one. + +3. **Close the tooltip ping-pong.** Shapes tooltip data is cached by *element key* but + requested per *layer config*, so two layers over one element with different + `tooltipFields` invalidate each other forever. Labels have the identical shape. + (`shapePrebuiltData` and `shapeFillColorData` were deliberately keyed by layer id to + avoid exactly this; the tooltip cache was missed.) + +--- + +#### Track C — Memory (ADR 0005 rungs 1–3) + +**Leak fixes, not architecture.** Independent of A and B. + +1. `MemoryReporting = { readonly byteLength: number }`. The scalar only. No tiers, no + policy. +2. Byte-bounded LRU over `parquetTableBytes` and `parquetTableCache`. Fix the + rejection-poisoning bug in the same pass. +3. Fill the empty chunk-cache seam: `enableWorkerChunkDecode({ cache })`. It is public, + exported, documented, and **never passed** — so there is no zarr chunk cache at all + today, and every tile re-fetches *and* re-decodes. + +`tgpu-htj2k`'s `TileCache` (~100 lines, framework-free, byte-bounded LRU with a +`dispose` hook, generic over payload) is directly reusable for 2 and 3. + +**Stop at rung 3.** Rungs 4–5 (encoded tier, tiered `ResidencyReport`, Resource +Ceiling) wait for measurement. See ADR 0005 for why. + +**File upstream to fizarrita** (ADR 0005 lists all four): the JP2K/HTJ2K blind spot in +`probeDecompressedSize` is the one that touches our actual imagery. + +--- + +### Step 3 — Renderer Adapter cleanup (after Step 1) + +- `project()` / `render()` formalised in `layers`. Identity-stable memoisation lives + **here**, not in the resolver — it is a deck requirement. +- Delete the three `eslint-disable react-hooks/refs` render-phase ref writes and the + `'use no memo'` React-Compiler opt-outs. If any survive, the snapshot is not + identity-stable and something is wrong. +- The channel-merge ladder (`ch?.X && ch.X.length > 0 ? ch.X : loadedData.X`) is + hand-written **ten times**. `mergeLayerChannelState` already exists in + `avivatorish`, is exported from `vis`, and is unused. +- Dead surface: `reloadElement` (returned, typed, zero call sites), the unused + `_coordinateSystem` parameter, `renderers/pointsRenderer.ts` (zero importers — + punchlist F2). + +--- + +## Definition of done + +- [ ] `packages/core` has no `react`, no `deck.gl`, no `@hms-dbmi/viv` import. Still true. +- [ ] `packages/layers` has no `react` import. Still true. +- [ ] The resolver is exercised by a test that constructs no deck layer and no GL context. +- [ ] R1, R2, R3, R5 each have a failing-before / passing-after test written *through* + `RequestSlot`. +- [ ] No `'use no memo'` remains in `packages/vis`. +- [ ] `useLayerData`'s 17-member surface is intact (compat shim is fine). +- [ ] `parquetTableBytes` and `parquetTableCache` report `byteLength` and are bounded. +- [ ] A failed catalog scan can be retried. + +## Explicitly out of scope + +- **Group Entry / blend compositing.** `CONTEXT.md` reserves it and says *"avoid: + framebuffer layer until the rendering behavior exists."* That holds. `splatDensity.ts` + in `tgpu-htj2k` is a GPU splat-by-blending primitive, **not** a prototype of Render + Stack compositing — using it would mean adopting the whole WebGPU stack. A group + layer-hierarchy is expected before long and may well be built on WebGPU; the Renderer + Adapter seam is what makes it approachable. **Add no framebuffer hook in anticipation.** +- **ADR 0003's FBO-based render caching.** Still deferred, on its own terms. +- **Resource Ceiling / degrade-to-fit.** ADR 0005 rung 5. Measure first. +- **Effect in a public signature.** Anywhere.