From 0ece94b50db76fa88572ea6ee6a880b8cc04ddde Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 16:28:52 +0100 Subject: [PATCH 1/5] Evict rejected parquet table promises from the cache (ADR 0005 rung 2b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parquetTableCache` caches the table promise before it settles — genuine in-flight dedup — but never cleaned up a rejection, so one transient fetch failure pinned a rejected promise at that path for the lifetime of the source and every later read replayed a network error that had cleared long ago. The entry now evicts itself on rejection, and only if it is still current, so a retry that superseded it is not clobbered by the earlier promise's late rejection — the same discipline `evictIfCurrent` already applies to the dataset metadata and part-path caches. The rejection still propagates unchanged to the caller that provoked it, so the skip-vs-fail policy in `docs/plans/parquet-io-error-handling.md` is untouched. Tests cover the retry as well as the two behaviours that must survive it: concurrent dedup, and successful tables staying cached. Co-Authored-By: Claude Opus 5 --- .../parquet-table-cache-rejection-cleanup.md | 27 ++++++ packages/core/src/models/VTableSource.ts | 29 ++++-- packages/core/tests/parquetTableCache.spec.ts | 94 +++++++++++++++++++ 3 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 .changeset/parquet-table-cache-rejection-cleanup.md create mode 100644 packages/core/tests/parquetTableCache.spec.ts diff --git a/.changeset/parquet-table-cache-rejection-cleanup.md b/.changeset/parquet-table-cache-rejection-cleanup.md new file mode 100644 index 00000000..b1f93f84 --- /dev/null +++ b/.changeset/parquet-table-cache-rejection-cleanup.md @@ -0,0 +1,27 @@ +--- +'@spatialdata/core': patch +--- + +Stop a transient parquet fetch failure from poisoning `loadParquetTable` for the +lifetime of the source. + +`parquetTableCache` stores the table promise *before* it settles. That is +deliberate and correct — it is what makes concurrent callers for the same file +share one `readParquet` + `tableFromIPC` decode instead of racing two WASM +parses of the same bytes. What was missing is the other half: nothing ever +removed a promise that settled as a *rejection*. A single failed read — a +dropped connection, a 503, a store not yet warm — left a rejected promise +parked at that path forever, and every subsequent read of that element replayed +a network error that had long since cleared. The only recovery was to construct +a new source. + +The cached promise now evicts itself on rejection, and only if it is still the +current entry for that path, so a retry that already superseded it is not +clobbered by the earlier promise's late rejection. This is the same +`evictIfCurrent` discipline `loadParquetDatasetMetadata` and +`discoverMultipartPartPaths` already use. + +In-flight dedup and the caching of successful tables are unchanged, and so is +the deliberate skip-vs-fail policy in `docs/plans/parquet-io-error-handling.md`: +the rejection still propagates unchanged to the caller that provoked it. It just +stops being the answer given to the next one. diff --git a/packages/core/src/models/VTableSource.ts b/packages/core/src/models/VTableSource.ts index efa18201..a220aa71 100644 --- a/packages/core/src/models/VTableSource.ts +++ b/packages/core/src/models/VTableSource.ts @@ -1049,12 +1049,29 @@ export default class SpatialDataTableSource extends AnnDataSource { return this.parquetTableCache[parquetPath]; } - const tablePromise = this._loadParquetTableUncached(parquetPath, columns); - - if (!columns?.length) { - this.parquetTableCache[parquetPath] = tablePromise; - } - + const uncached = this._loadParquetTableUncached(parquetPath, columns); + if (columns?.length) { + return uncached; + } + + // Cached BEFORE it settles, so concurrent callers share one WASM decode — but + // what they share must not be a rejection kept forever. Uncleaned, a single + // transient fetch failure pins a rejected promise at this path for the + // lifetime of the source, and every later read replays a network error that + // cleared long ago. + // + // This is a cache-liveness fix, not a change to the skip-vs-fail policy in + // `docs/plans/parquet-io-error-handling.md`: the rejection still propagates + // unchanged to this caller, it just stops being the answer for the next one. + const tablePromise = uncached.catch((error: unknown) => { + // Only if still current — a retry that superseded this entry must not be + // clobbered by this promise's late rejection (see {@link evictIfCurrent}). + if (this.parquetTableCache[parquetPath] === tablePromise) { + delete this.parquetTableCache[parquetPath]; + } + throw error; + }); + this.parquetTableCache[parquetPath] = tablePromise; return tablePromise; } diff --git a/packages/core/tests/parquetTableCache.spec.ts b/packages/core/tests/parquetTableCache.spec.ts new file mode 100644 index 00000000..5f0255ca --- /dev/null +++ b/packages/core/tests/parquetTableCache.spec.ts @@ -0,0 +1,94 @@ +import { tableFromArrays, tableToIPC } from 'apache-arrow'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import SpatialDataTableSource from '../src/models/VTableSource.js'; + +const PARQUET_PATH = 'points/cells/points.parquet'; + +function createParquetBytes() { + return new Uint8Array([0x50, 0x41, 0x52, 0x31, 0x00, 0x00, 0x00, 0x00, 0x50, 0x41, 0x52, 0x31]); +} + +/** + * A parquet module whose `readParquet` returns a fixed three-row table. + * + * `readMetadata` is deliberately absent: without it `loadParquetDatasetMetadata` + * short-circuits to `null`, so these tests exercise the single-file path through + * `loadParquetBytes` without needing range support or real WASM. + */ +function createParquetModuleStub() { + const ipcBytes = tableToIPC(tableFromArrays({ x: Float32Array.from([1, 2, 3]) })); + const wasmTable = { intoIPCStream: () => ipcBytes }; + return { + readParquet: vi.fn(() => wasmTable), + readSchema: vi.fn(() => wasmTable), + // biome-ignore lint/suspicious/noExplicitAny: test double for the WASM module surface + } as any; +} + +/** A store that fails `failures` times on the main parquet path, then succeeds. */ +function createFlakyStore(failures: number) { + const parquetBytes = createParquetBytes(); + let remainingFailures = failures; + const get = vi.fn(async (path: string) => { + if (path === `/${PARQUET_PATH}`) { + if (remainingFailures > 0) { + remainingFailures -= 1; + throw new Error('transient network failure'); + } + return parquetBytes; + } + return null; + }); + return { get }; +} + +function createSource(store: { get: ReturnType }) { + return new SpatialDataTableSource({ + // biome-ignore lint/suspicious/noExplicitAny: minimal zarr.Readable test double + store: store as any, + fileType: '.zarr', + }); +} + +describe('SpatialDataTableSource parquet table cache', () => { + beforeEach(() => { + SpatialDataTableSource.parquetModulePromise = Promise.resolve(createParquetModuleStub()); + }); + + it('does not poison the cache with a rejected promise after a transient failure', async () => { + const store = createFlakyStore(1); + const source = createSource(store); + + await expect(source.loadParquetTable(PARQUET_PATH)).rejects.toThrow( + 'Failed to load parquet data from store.' + ); + + // The store works now. A retry must reach it again rather than replaying the + // cached rejection for the lifetime of the source. + const table = await source.loadParquetTable(PARQUET_PATH); + expect(table.numRows).toBe(3); + expect(store.get).toHaveBeenCalledWith(`/${PARQUET_PATH}`); + }); + + it('still dedupes concurrent unfiltered reads onto one decode', async () => { + const store = createFlakyStore(0); + const source = createSource(store); + + const [first, second] = await Promise.all([ + source.loadParquetTable(PARQUET_PATH), + source.loadParquetTable(PARQUET_PATH), + ]); + + expect(first).toBe(second); + }); + + it('keeps a successful table cached across sequential reads', async () => { + const store = createFlakyStore(0); + const source = createSource(store); + + const first = await source.loadParquetTable(PARQUET_PATH); + const second = await source.loadParquetTable(PARQUET_PATH); + + expect(first).toBe(second); + }); +}); From a031d781fd80b42678231e908086f634e31e7e5a Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 16:30:19 +0100 Subject: [PATCH 2/5] Adopt the MemoryReporting scalar (ADR 0005 rung 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{ readonly byteLength: number }`, exported from `@spatialdata/core`. One number, named so that every `TypedArray`, `ArrayBuffer` and `DataView` satisfies it structurally — no wrapper, no import — which is what makes it cheap enough to put on every cache rather than a chosen few. No policy, no eviction, no tiers: the ADR gates rungs 2 and 3 on this existing, and is emphatic that this rung cannot be over-engineered. The tiered `ResidencyReport` and the global ceiling stay deferred. Co-Authored-By: Claude Opus 5 --- .changeset/memory-reporting-scalar.md | 20 ++++++++++ packages/core/src/index.ts | 2 + packages/core/src/memory/index.ts | 8 ++++ packages/core/src/memory/memoryReporting.ts | 42 +++++++++++++++++++++ 4 files changed, 72 insertions(+) create mode 100644 .changeset/memory-reporting-scalar.md create mode 100644 packages/core/src/memory/index.ts create mode 100644 packages/core/src/memory/memoryReporting.ts diff --git a/.changeset/memory-reporting-scalar.md b/.changeset/memory-reporting-scalar.md new file mode 100644 index 00000000..242b6000 --- /dev/null +++ b/.changeset/memory-reporting-scalar.md @@ -0,0 +1,20 @@ +--- +'@spatialdata/core': minor +--- + +Add `MemoryReporting` — `{ readonly byteLength: number }` — the first rung of +[ADR 0005](https://github.com/Taylor-CCB-Group/SpatialData.js/blob/main/docs/adr/0005-memory-accounting-before-management.md). + +The library had a memory *policy* and no memory *accounting*: `DEFAULT_POINTS_MEMORY_CAP` +is a row count applied to one element kind, and nothing anywhere could answer +"how many bytes are resident right now?". This is that answer, and only that +answer — no tiers, no eviction, no ceiling. + +The name is doing the work. `byteLength` is what `TypedArray`, `ArrayBuffer` and +`DataView` already call this, so every payload we actually hold satisfies the +interface structurally, with no wrapper and no import. That is what makes it +cheap enough to put on every cache rather than on a chosen few. + +Implementors take on one obligation: keep the number cheap to read — a running +total maintained on insert and evict, not a scan of the residents per read — so +that callers can poll it freely. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b4787243..0d1b2353 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,6 +6,8 @@ // Resource Resolver contracts (ADR 0004). export * from './engine/index.js'; +// Memory accounting (ADR 0005). +export * from './memory/index.js'; export * from './models/index.js'; // The semantics that need the tree guards, not just the guards themselves: a // consumer enumerating obs columns has to know that a group might be a diff --git a/packages/core/src/memory/index.ts b/packages/core/src/memory/index.ts new file mode 100644 index 00000000..f1f9ce71 --- /dev/null +++ b/packages/core/src/memory/index.ts @@ -0,0 +1,8 @@ +/** + * Memory accounting for resident caches. + * + * See [ADR 0005](../../../../docs/adr/0005-memory-accounting-before-management.md): + * accounting first, management only where something is already unbounded. + */ + +export type { MemoryReporting } from './memoryReporting.js'; diff --git a/packages/core/src/memory/memoryReporting.ts b/packages/core/src/memory/memoryReporting.ts new file mode 100644 index 00000000..d89ff755 --- /dev/null +++ b/packages/core/src/memory/memoryReporting.ts @@ -0,0 +1,42 @@ +/** + * Memory accounting — the scalar, and nothing else. + * + * See [ADR 0005](../../../../docs/adr/0005-memory-accounting-before-management.md). + * Deliberately one number: no tiers, no policy, no eviction, no ceiling. Those + * are later rungs, and the ADR gates every one of them on this existing first. + */ + +/** + * Anything holding resident host memory can report it in bytes. + * + * The name is the design. `byteLength` is what `TypedArray`, `ArrayBuffer` and + * `DataView` already call this, so all of them satisfy this interface + * structurally — for free, with no wrapper and no import: + * + * ```ts + * const resident: MemoryReporting = new Uint8Array(1024); // 1024 + * ``` + * + * That is what makes it cheap enough to put on every cache: the payloads we + * actually hold are mostly typed arrays already, and the containers around them + * only have to keep a running total. + * + * ### The obligation this creates + * + * Keep it cheap. A cache implementing this must maintain a running total across + * insert and evict rather than scanning its residents on every read — callers + * are expected to be free to poll it (a HUD, a test assertion, a decision about + * what to drop next) without that being a performance question. + * + * ### What it deliberately cannot express + * + * A scalar cannot distinguish an encoded tier from a decoded one, and it cannot + * see a worker heap that a synchronous getter has no access to. Both are real + * for SpatialData.ts, and both are deferred: the tiered `ResidencyReport` is + * ADR 0005 rung 5, and the ADR's position is that it should not be built until + * something needs to *act* on the difference between tiers. + */ +export interface MemoryReporting { + /** Resident bytes held by this object, right now. */ + readonly byteLength: number; +} From 19561379bd188b43bec813dbbdb6903bacd25d6b Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 16:35:47 +0100 Subject: [PATCH 3/5] Bound both parquet caches by resident bytes (ADR 0005 rung 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parquetTableBytes` and `parquetTableCache` were plain `Record`s with no eviction: a source held both the compressed and the decoded tier of every parquet file any caller had ever touched, simultaneously, until the source was discarded. Double memory, zero eviction benefit. Adds `ByteLruCache` — framework-free, byte-bounded, `MemoryReporting`, with a dispose hook — and puts both tiers behind it. Two semantics the tests pin: - An oversized value is admitted and left sole resident rather than refused. `loadParquetBytes` runs ~20 times per points load, so a value that can never be admitted becomes ~20 refetches of the file too big to fetch once. - Sizes need not be known at insertion. The decoded cache holds the in-flight promise (that is the dedup), so it enters at zero bytes and is recounted when the table lands. Arrow's `Data.byteLength` walks the child tree, so it is asked once per table and the total is incremental from there. Ceilings default to 128 MB encoded / 256 MB decoded, overridable per source via `DataSourceParams.parquetCacheLimits`. The numbers bound a leak; they are not a measured working set, which is why they are an option and not a constant. Breaking for direct readers of those two fields — `Record` index access becomes `.get()`. Nothing outside `VTableSource` touched either. Co-Authored-By: Claude Opus 5 --- .changeset/bound-parquet-caches.md | 39 ++++ packages/core/src/Vutils.ts | 16 ++ packages/core/src/memory/byteLruCache.ts | 190 ++++++++++++++++++ packages/core/src/memory/index.ts | 2 + packages/core/src/models/VTableSource.ts | 126 +++++++++--- packages/core/tests/byteLruCache.spec.ts | 151 ++++++++++++++ packages/core/tests/parquetTableCache.spec.ts | 67 +++++- 7 files changed, 556 insertions(+), 35 deletions(-) create mode 100644 .changeset/bound-parquet-caches.md create mode 100644 packages/core/src/memory/byteLruCache.ts create mode 100644 packages/core/tests/byteLruCache.spec.ts diff --git a/.changeset/bound-parquet-caches.md b/.changeset/bound-parquet-caches.md new file mode 100644 index 00000000..029f0cb1 --- /dev/null +++ b/.changeset/bound-parquet-caches.md @@ -0,0 +1,39 @@ +--- +'@spatialdata/core': minor +--- + +Bound the two parquet caches on `SpatialDataTableSource` by resident bytes +([ADR 0005](https://github.com/Taylor-CCB-Group/SpatialData.js/blob/main/docs/adr/0005-memory-accounting-before-management.md) +rung 2), and add the `ByteLruCache` they are built on. + +`parquetTableBytes` (compressed file bytes) and `parquetTableCache` (decoded +Arrow tables) were plain `Record`s with no eviction of any kind. A source held +**both tiers of every parquet file any caller had ever touched**, simultaneously, +until the source itself was discarded — double memory for zero eviction benefit. +That is a leak, and this fixes it rather than building an architecture around it: +both are now byte-bounded LRUs that report `byteLength`, and memory is +assertable in a test for the first time. + +**Breaking for anyone reading those two fields directly.** They are no longer +plain objects: `source.parquetTableBytes[path]` becomes +`source.parquetTableBytes.get(path)`, with `peek` for a read that should not +count as a use, plus `has`, `delete`, `clear`, `size` and `byteLength`. Nothing +in this repository outside `VTableSource` touched either one. + +Ceilings default to 128 MB encoded and 256 MB decoded per source, overridable +via the new `parquetCacheLimits` field on `DataSourceParams`. The numbers are +guesses that bound a leak, not a measured working set — the ADR is explicit that +they stay guesses until something measures them, so they are a constructor +option rather than a constant you would have to fork the library to change. + +Two semantics worth knowing: + +- **A value larger than the whole budget is admitted, not refused**, and left as + the sole resident. Refusing it would be the worse failure: `loadParquetBytes` + runs roughly twenty times per points load, so a file that can never be admitted + becomes twenty refetches of the file that was already too big to fetch once. +- **Entries are inserted before their size is known.** The decoded cache holds + the in-flight promise — that is what dedupes concurrent callers onto one WASM + decode — so it is sized at zero until the table lands, then recounted. Arrow's + `Data.byteLength` walks the whole child tree, so it is asked exactly once per + table and the total is maintained incrementally from there. diff --git a/packages/core/src/Vutils.ts b/packages/core/src/Vutils.ts index a3eb8f5e..c53fd477 100644 --- a/packages/core/src/Vutils.ts +++ b/packages/core/src/Vutils.ts @@ -27,6 +27,20 @@ export function basename(path: string) { return result; } +/** + * Resident-byte ceilings for the two parquet caches a source holds. + * + * Both default to values chosen to bound a leak, not to fit a measured working + * set — ADR 0005 is explicit that the numbers stay guesses until something + * measures them. Raise or lower them per source when you know better. + */ +export type ParquetCacheLimits = { + /** Ceiling for cached compressed parquet file bytes. */ + encodedMaxBytes?: number; + /** Ceiling for cached decoded Arrow tables. */ + decodedMaxBytes?: number; +}; + export type DataSourceParams = { url?: string; /** Options to pass to fetch calls. */ @@ -35,4 +49,6 @@ export type DataSourceParams = { store?: Readable; /** The file type. */ fileType: string; // '.zip' | '.h5ad' etc... + /** Optional overrides for the parquet cache byte ceilings. */ + parquetCacheLimits?: ParquetCacheLimits; }; diff --git a/packages/core/src/memory/byteLruCache.ts b/packages/core/src/memory/byteLruCache.ts new file mode 100644 index 00000000..3759d6f1 --- /dev/null +++ b/packages/core/src/memory/byteLruCache.ts @@ -0,0 +1,190 @@ +import type { MemoryReporting } from './memoryReporting.js'; + +export interface ByteLruCacheOptions { + /** + * Resident-byte ceiling. Enforced after every insertion and every + * {@link ByteLruCache.recount}, by dropping least-recently-used entries. + * + * Not an absolute guarantee — see {@link ByteLruCache} on oversized values. + */ + maxBytes: number; + /** + * Bytes held by a value. + * + * Called once per insertion and once per {@link ByteLruCache.recount}, never + * per read: the cache keeps a running total instead, which is the obligation + * {@link MemoryReporting} takes on. + */ + sizeOf: (value: V) => number; + /** + * Called as an entry leaves — evicted, overwritten, deleted or cleared — + * exactly once per departure, after the cache's own bookkeeping is settled. + * + * For releasing something a garbage collector will not: a GPU buffer, a worker + * handle. Plain typed arrays need nothing here. + */ + onDispose?: (value: V, key: string) => void; +} + +interface Entry { + value: V; + bytes: number; +} + +/** + * A byte-bounded LRU cache that reports what it is holding. + * + * Framework-free and deliberately small — [ADR 0005](../../../../docs/adr/0005-memory-accounting-before-management.md) + * rung 2 is *fixing a leak, not building an architecture*. It exists because the + * two parquet caches in `VTableSource` were plain `Record`s that grew until the + * source was discarded, and because filling fizarrita's chunk-cache seam needs + * something bounded to fill it with. + * + * Recency is `Map` insertion order: re-inserting a key moves it to the tail, and + * eviction takes from the head. {@link get} and {@link recount} count as uses; + * {@link peek} and {@link has} do not. + * + * ### Oversized values are admitted, not refused + * + * If a single value exceeds `maxBytes`, it is stored anyway and left as the sole + * resident — so the effective bound is *`maxBytes`, or one entry, whichever is + * larger*. Refusing it would be the worse failure: `loadParquetBytes` is called + * roughly twenty times per points load, so a value that can never be admitted + * becomes twenty refetches of the very file that was too big to fetch once. + * + * ### Sizes that are not known at insertion + * + * A cache of in-flight promises cannot be sized when the entry goes in. Insert + * it at whatever `sizeOf` can say (zero), and call {@link recount} once the real + * size is knowable. That is why sizing is a callback the cache re-runs on demand + * rather than a number captured at insertion. + */ +export class ByteLruCache implements MemoryReporting { + readonly maxBytes: number; + private readonly sizeOf: (value: V) => number; + private readonly onDispose?: (value: V, key: string) => void; + private readonly entries = new Map>(); + private residentBytes = 0; + + constructor(options: ByteLruCacheOptions) { + this.maxBytes = options.maxBytes; + this.sizeOf = options.sizeOf; + this.onDispose = options.onDispose; + } + + /** Resident bytes, maintained incrementally — never a scan of the residents. */ + get byteLength(): number { + return this.residentBytes; + } + + /** Number of resident entries. */ + get size(): number { + return this.entries.size; + } + + /** Read a value, counting the read as a use. */ + get(key: string): V | undefined { + const entry = this.entries.get(key); + if (!entry) { + return undefined; + } + this.entries.delete(key); + this.entries.set(key, entry); + return entry.value; + } + + /** Read a value *without* counting it as a use. */ + peek(key: string): V | undefined { + return this.entries.get(key)?.value; + } + + /** Whether a key is resident. Not a use. */ + has(key: string): boolean { + return this.entries.has(key); + } + + /** Resident keys, least-recently-used first. */ + keys(): IterableIterator { + return this.entries.keys(); + } + + /** + * Insert or replace a value, then evict down to budget. + * + * Replacing a key disposes the value it displaced, so `set` is safe to use as + * an upsert without leaking the previous payload. + */ + set(key: string, value: V): void { + const previous = this.entries.get(key); + if (previous) { + this.entries.delete(key); + this.residentBytes -= previous.bytes; + } + const bytes = this.sizeOf(value); + this.entries.set(key, { value, bytes }); + this.residentBytes += bytes; + if (previous) { + this.onDispose?.(previous.value, key); + } + this.evictToBudget(); + } + + /** + * Re-ask `sizeOf` for a resident value whose size has changed since insertion, + * then evict down to budget. A no-op for a key that is no longer resident. + * + * Counts as a use: the size becoming knowable — a decode landing, a payload + * being filled in — is the moment the entry is most worth keeping, and the + * eviction pass this triggers would otherwise be liable to drop it. + */ + recount(key: string): void { + const entry = this.entries.get(key); + if (!entry) { + return; + } + const bytes = this.sizeOf(entry.value); + this.residentBytes += bytes - entry.bytes; + entry.bytes = bytes; + this.entries.delete(key); + this.entries.set(key, entry); + this.evictToBudget(); + } + + /** Drop a key. Returns whether it was resident. */ + delete(key: string): boolean { + const entry = this.entries.get(key); + if (!entry) { + return false; + } + this.entries.delete(key); + this.residentBytes -= entry.bytes; + this.onDispose?.(entry.value, key); + return true; + } + + /** Drop everything, disposing each entry. */ + clear(): void { + const disposing = [...this.entries]; + this.entries.clear(); + this.residentBytes = 0; + if (this.onDispose) { + for (const [key, entry] of disposing) { + this.onDispose(entry.value, key); + } + } + } + + /** + * Evict from the least-recently-used end until within budget, stopping while + * one entry remains so that an oversized value is kept rather than refused. + */ + private evictToBudget(): void { + while (this.residentBytes > this.maxBytes && this.entries.size > 1) { + const oldest = this.entries.keys().next(); + if (oldest.done) { + return; + } + this.delete(oldest.value); + } + } +} diff --git a/packages/core/src/memory/index.ts b/packages/core/src/memory/index.ts index f1f9ce71..3e64ae91 100644 --- a/packages/core/src/memory/index.ts +++ b/packages/core/src/memory/index.ts @@ -5,4 +5,6 @@ * accounting first, management only where something is already unbounded. */ +export type { ByteLruCacheOptions } from './byteLruCache.js'; +export { ByteLruCache } from './byteLruCache.js'; export type { MemoryReporting } from './memoryReporting.js'; diff --git a/packages/core/src/models/VTableSource.ts b/packages/core/src/models/VTableSource.ts index a220aa71..22e96d24 100644 --- a/packages/core/src/models/VTableSource.ts +++ b/packages/core/src/models/VTableSource.ts @@ -1,6 +1,7 @@ // this is a direct copy of the Vitessce implementation, with changes mostly to make it more normal TypeScript. import { type Table as ArrowTable, tableFromIPC } from 'apache-arrow'; +import { ByteLruCache } from '../memory/byteLruCache.js'; import { getParquetModule, type ParquetModule, @@ -14,6 +15,42 @@ import AnnDataSource from './VAnnDataSource'; export type { ParquetRowGroupReadOptions }; +/** + * Default ceiling for the encoded (compressed parquet bytes) tier, per source. + * + * A guess that bounds a leak, not a measured working set — ADR 0005 defers the + * numbers until something measures them. It is deliberately the smaller of the + * two: compressed bytes are only needed until they have been decoded, whereas a + * decoded table is what callers actually hold on to. + */ +export const DEFAULT_PARQUET_ENCODED_MAX_BYTES = 128 * 1024 * 1024; + +/** Default ceiling for the decoded (Arrow table) tier, per source. */ +export const DEFAULT_PARQUET_DECODED_MAX_BYTES = 256 * 1024 * 1024; + +/** + * A decoded-table cache entry. + * + * The promise is cached before it settles — that is the in-flight dedup — so the + * byte count starts at zero and is filled in once the table exists. Holding it + * on the entry rather than re-deriving it keeps {@link ByteLruCache.byteLength} + * a running total: `Data.byteLength` in Arrow walks the whole child tree on every + * read, so it is asked exactly once per table. + */ +export interface CachedParquetTable { + readonly promise: Promise; + byteLength: number; +} + +/** Resident bytes of a decoded Arrow table, children included. */ +function arrowTableByteLength(table: ArrowTable): number { + let bytes = 0; + for (const data of table.data) { + bytes += data.byteLength; + } + return bytes; +} + function parquetColumnValueToNumber(value: unknown): number | null { if (typeof value === 'number' && Number.isFinite(value)) { return value; @@ -176,15 +213,28 @@ export default class SpatialDataTableSource extends AnnDataSource { rootAttrs: { softwareVersion: string; formatVersion: string } | null; // biome-ignore lint/suspicious/noExplicitAny: elementAttrs type should be a tree-ish thing elementAttrs: Record; - parquetTableBytes: Record; + /** + * Cache of compressed parquet file bytes — the encoded tier. + * + * Byte-bounded (ADR 0005 rung 2): it was a plain `Record` that grew for the + * lifetime of the source, holding every parquet file any caller ever touched, + * *simultaneously with* the decoded tier below. Read `.byteLength` for what it + * is currently holding. + */ + parquetTableBytes: ByteLruCache; /** * Cache of fully-parsed Arrow tables for paths requested without a column * filter. Avoids repeating the WASM `readParquet` + `tableFromIPC` decode * when the same parquet file is needed by multiple callers in sequence (e.g. * `inferShapesGeometryKindFromParquet`, `loadShapesIndex`, and * `loadPolygonShapes` all target the same file). + * + * Byte-bounded like the encoded tier, with one wrinkle: an entry is inserted + * while its decode is still in flight — that is what makes the dedup work — so + * it is sized at zero until the table exists and {@link ByteLruCache.recount} + * can be told the real number. */ - parquetTableCache: Record>; + parquetTableCache: ByteLruCache; /** * Remembers parquet part layout per path — single file vs. `part.N.parquet` * directory, and the per-part metadata — so the probe sequence (see @@ -217,8 +267,14 @@ export default class SpatialDataTableSource extends AnnDataSource { this.elementAttrs = {}; // TODO: change to column-specific storage. - this.parquetTableBytes = {}; - this.parquetTableCache = {}; + this.parquetTableBytes = new ByteLruCache({ + maxBytes: params.parquetCacheLimits?.encodedMaxBytes ?? DEFAULT_PARQUET_ENCODED_MAX_BYTES, + sizeOf: (bytes) => bytes.byteLength, + }); + this.parquetTableCache = new ByteLruCache({ + maxBytes: params.parquetCacheLimits?.decodedMaxBytes ?? DEFAULT_PARQUET_DECODED_MAX_BYTES, + sizeOf: (entry) => entry.byteLength, + }); this.parquetDatasetMetadataCache = new Map(); this.parquetPartPathsCache = new Map(); this.rowGroupColumnExtentCache = new Map(); @@ -325,9 +381,10 @@ export default class SpatialDataTableSource extends AnnDataSource { } async loadParquetBytes(parquetPath: string) { - if (this.parquetTableBytes[parquetPath]) { + const cachedBytes = this.parquetTableBytes.get(parquetPath); + if (cachedBytes) { // Return the cached bytes. - return this.parquetTableBytes[parquetPath]; + return cachedBytes; } for (const candidatePath of await this.orderedParquetCandidatePaths(parquetPath)) { @@ -340,7 +397,7 @@ export default class SpatialDataTableSource extends AnnDataSource { continue; } // Cache the parquet bytes. - this.parquetTableBytes[parquetPath] = normalizedBytes; + this.parquetTableBytes.set(parquetPath, normalizedBytes); return normalizedBytes; } catch { // Keep probing candidate parquet paths. @@ -1045,8 +1102,11 @@ export default class SpatialDataTableSource extends AnnDataSource { async loadParquetTable(parquetPath: string, columns?: string[]): Promise { // When no column filter is requested, return a shared promise so that // concurrent or sequential callers for the same file share one WASM decode. - if (!columns?.length && parquetPath in this.parquetTableCache) { - return this.parquetTableCache[parquetPath]; + if (!columns?.length) { + const cached = this.parquetTableCache.get(parquetPath); + if (cached) { + return cached.promise; + } } const uncached = this._loadParquetTableUncached(parquetPath, columns); @@ -1054,25 +1114,39 @@ export default class SpatialDataTableSource extends AnnDataSource { return uncached; } - // Cached BEFORE it settles, so concurrent callers share one WASM decode — but - // what they share must not be a rejection kept forever. Uncleaned, a single - // transient fetch failure pins a rejected promise at this path for the - // lifetime of the source, and every later read replays a network error that - // cleared long ago. + // Cached BEFORE it settles, so concurrent callers share one WASM decode. + const entry: CachedParquetTable = { promise: uncached, byteLength: 0 }; + this.parquetTableCache.set(parquetPath, entry); + + // Bookkeeping, deliberately kept OFF the caller's chain: this branch settles + // the entry's byte count, or drops a poisoned entry, and can do neither to + // what the caller sees. It swallows the rejection rather than rethrowing for + // the same reason — rethrowing here would surface as an unhandled rejection + // on a promise nobody awaits, while the caller's own `uncached` still throws. // - // This is a cache-liveness fix, not a change to the skip-vs-fail policy in - // `docs/plans/parquet-io-error-handling.md`: the rejection still propagates - // unchanged to this caller, it just stops being the answer for the next one. - const tablePromise = uncached.catch((error: unknown) => { - // Only if still current — a retry that superseded this entry must not be - // clobbered by this promise's late rejection (see {@link evictIfCurrent}). - if (this.parquetTableCache[parquetPath] === tablePromise) { - delete this.parquetTableCache[parquetPath]; + // Dropping on rejection is the point: uncleaned, a single transient fetch + // failure pins a rejected promise at this path for the lifetime of the + // source, and every later read replays a network error that cleared long + // ago. It is a cache-liveness fix, not a change to the skip-vs-fail policy in + // `docs/plans/parquet-io-error-handling.md`. + void uncached.then( + (table) => { + entry.byteLength = arrowTableByteLength(table); + // Only if still current — a retry, or an eviction, may already have + // replaced us, and that entry must not be resized or dropped by ours + // (the same reasoning as {@link evictIfCurrent}). + if (this.parquetTableCache.peek(parquetPath) === entry) { + this.parquetTableCache.recount(parquetPath); + } + }, + () => { + if (this.parquetTableCache.peek(parquetPath) === entry) { + this.parquetTableCache.delete(parquetPath); + } } - throw error; - }); - this.parquetTableCache[parquetPath] = tablePromise; - return tablePromise; + ); + + return uncached; } /** diff --git a/packages/core/tests/byteLruCache.spec.ts b/packages/core/tests/byteLruCache.spec.ts new file mode 100644 index 00000000..baf7bc6c --- /dev/null +++ b/packages/core/tests/byteLruCache.spec.ts @@ -0,0 +1,151 @@ +import { describe, expect, it, vi } from 'vitest'; +import { ByteLruCache } from '../src/memory/byteLruCache.js'; +import type { MemoryReporting } from '../src/memory/memoryReporting.js'; + +/** The ergonomic claim ADR 0005 makes for `MemoryReporting`, as a compile-time check. */ +const _typedArrayReportsMemory: MemoryReporting = new Uint8Array(8); + +function bytesCache(maxBytes: number, onDispose?: (value: Uint8Array, key: string) => void) { + return new ByteLruCache({ + maxBytes, + sizeOf: (value) => value.byteLength, + onDispose, + }); +} + +describe('ByteLruCache', () => { + it('reports resident bytes as a running total', () => { + const cache = bytesCache(1000); + expect(cache.byteLength).toBe(0); + + cache.set('a', new Uint8Array(100)); + cache.set('b', new Uint8Array(250)); + + expect(cache.byteLength).toBe(350); + expect(cache.size).toBe(2); + }); + + it('evicts least-recently-used entries until it fits', () => { + const cache = bytesCache(300); + cache.set('a', new Uint8Array(100)); + cache.set('b', new Uint8Array(100)); + cache.set('c', new Uint8Array(100)); + expect(cache.byteLength).toBe(300); + + cache.set('d', new Uint8Array(100)); + + expect(cache.has('a')).toBe(false); + expect([...cache.keys()]).toEqual(['b', 'c', 'd']); + expect(cache.byteLength).toBe(300); + }); + + it('counts a read as a use, so the read entry survives the next eviction', () => { + const cache = bytesCache(300); + cache.set('a', new Uint8Array(100)); + cache.set('b', new Uint8Array(100)); + cache.set('c', new Uint8Array(100)); + + expect(cache.get('a')).toBeDefined(); + cache.set('d', new Uint8Array(100)); + + expect(cache.has('a')).toBe(true); + expect(cache.has('b')).toBe(false); + }); + + it('does not count a peek as a use', () => { + const cache = bytesCache(300); + cache.set('a', new Uint8Array(100)); + cache.set('b', new Uint8Array(100)); + cache.set('c', new Uint8Array(100)); + + expect(cache.peek('a')).toBeDefined(); + cache.set('d', new Uint8Array(100)); + + expect(cache.has('a')).toBe(false); + }); + + it('replaces rather than double-counts when a key is overwritten', () => { + const onDispose = vi.fn(); + const cache = bytesCache(1000, onDispose); + const first = new Uint8Array(100); + cache.set('a', first); + cache.set('a', new Uint8Array(400)); + + expect(cache.size).toBe(1); + expect(cache.byteLength).toBe(400); + expect(onDispose).toHaveBeenCalledWith(first, 'a'); + }); + + it('disposes evicted, deleted and cleared entries exactly once', () => { + const onDispose = vi.fn(); + const cache = bytesCache(200, onDispose); + const evicted = new Uint8Array(100); + cache.set('evicted', evicted); + cache.set('deleted', new Uint8Array(100)); + cache.set('cleared', new Uint8Array(100)); + expect(onDispose).toHaveBeenCalledExactlyOnceWith(evicted, 'evicted'); + + cache.delete('deleted'); + expect(cache.byteLength).toBe(100); + + cache.clear(); + expect(cache.byteLength).toBe(0); + expect(cache.size).toBe(0); + expect(onDispose).toHaveBeenCalledTimes(3); + }); + + it('picks up a size that only became known after insertion', () => { + // The parquet table cache's shape: the entry is inserted while the decode is + // still in flight, so its byte count is zero until the table exists. + const cache = new ByteLruCache<{ byteLength: number }>({ + maxBytes: 300, + sizeOf: (value) => value.byteLength, + }); + const pending = { byteLength: 0 }; + cache.set('pending', pending); + cache.set('other', { byteLength: 100 }); + expect(cache.byteLength).toBe(100); + + pending.byteLength = 250; + cache.recount('pending'); + + expect(cache.byteLength).toBe(250); + // 350 would have been over budget, so the older entry went. + expect(cache.has('other')).toBe(false); + expect(cache.has('pending')).toBe(true); + }); + + it('ignores a recount for a key it no longer holds', () => { + const cache = bytesCache(1000); + cache.set('a', new Uint8Array(100)); + cache.delete('a'); + + expect(() => cache.recount('a')).not.toThrow(); + expect(cache.byteLength).toBe(0); + }); + + it('admits a value larger than the whole budget rather than reporting a miss', () => { + // Refusing it would be worse than exceeding the bound: `loadParquetBytes` is + // called ~20 times per points load, and a permanent miss means ~20 refetches + // of a file too big to fetch even once cheaply. + const cache = bytesCache(300); + cache.set('small', new Uint8Array(100)); + const huge = new Uint8Array(5000); + cache.set('huge', huge); + + expect(cache.get('huge')).toBe(huge); + expect(cache.size).toBe(1); + expect(cache.byteLength).toBe(5000); + + // ...and it is the first thing to go once anything else arrives. + cache.set('next', new Uint8Array(100)); + expect(cache.has('huge')).toBe(false); + expect(cache.byteLength).toBe(100); + }); + + it('satisfies MemoryReporting', () => { + const cache: MemoryReporting = bytesCache(100); + expect(cache.byteLength).toBe(0); + expect(_typedArrayReportsMemory.byteLength).toBe(8); + }); +}); diff --git a/packages/core/tests/parquetTableCache.spec.ts b/packages/core/tests/parquetTableCache.spec.ts index 5f0255ca..20fe167a 100644 --- a/packages/core/tests/parquetTableCache.spec.ts +++ b/packages/core/tests/parquetTableCache.spec.ts @@ -1,8 +1,12 @@ import { tableFromArrays, tableToIPC } from 'apache-arrow'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ParquetCacheLimits } from '../src/Vutils.js'; import SpatialDataTableSource from '../src/models/VTableSource.js'; const PARQUET_PATH = 'points/cells/points.parquet'; +const OTHER_PARQUET_PATH = 'points/nuclei/points.parquet'; +/** Length of the stub file bytes below — the unit the encoded-tier tests count in. */ +const PARQUET_BYTE_LENGTH = 12; function createParquetBytes() { return new Uint8Array([0x50, 0x41, 0x52, 0x31, 0x00, 0x00, 0x00, 0x00, 0x50, 0x41, 0x52, 0x31]); @@ -25,9 +29,11 @@ function createParquetModuleStub() { } as any; } -/** A store that fails `failures` times on the main parquet path, then succeeds. */ -function createFlakyStore(failures: number) { - const parquetBytes = createParquetBytes(); +/** + * A store serving parquet bytes at both test paths, failing the first `failures` + * reads of {@link PARQUET_PATH}. + */ +function createFlakyStore(failures = 0) { let remainingFailures = failures; const get = vi.fn(async (path: string) => { if (path === `/${PARQUET_PATH}`) { @@ -35,18 +41,22 @@ function createFlakyStore(failures: number) { remainingFailures -= 1; throw new Error('transient network failure'); } - return parquetBytes; + return createParquetBytes(); + } + if (path === `/${OTHER_PARQUET_PATH}`) { + return createParquetBytes(); } return null; }); return { get }; } -function createSource(store: { get: ReturnType }) { +function createSource(store: { get: ReturnType }, limits?: ParquetCacheLimits) { return new SpatialDataTableSource({ // biome-ignore lint/suspicious/noExplicitAny: minimal zarr.Readable test double store: store as any, fileType: '.zarr', + parquetCacheLimits: limits, }); } @@ -62,6 +72,7 @@ describe('SpatialDataTableSource parquet table cache', () => { await expect(source.loadParquetTable(PARQUET_PATH)).rejects.toThrow( 'Failed to load parquet data from store.' ); + expect(source.parquetTableCache.size).toBe(0); // The store works now. A retry must reach it again rather than replaying the // cached rejection for the lifetime of the source. @@ -71,8 +82,7 @@ describe('SpatialDataTableSource parquet table cache', () => { }); it('still dedupes concurrent unfiltered reads onto one decode', async () => { - const store = createFlakyStore(0); - const source = createSource(store); + const source = createSource(createFlakyStore()); const [first, second] = await Promise.all([ source.loadParquetTable(PARQUET_PATH), @@ -83,12 +93,51 @@ describe('SpatialDataTableSource parquet table cache', () => { }); it('keeps a successful table cached across sequential reads', async () => { - const store = createFlakyStore(0); - const source = createSource(store); + const source = createSource(createFlakyStore()); const first = await source.loadParquetTable(PARQUET_PATH); const second = await source.loadParquetTable(PARQUET_PATH); expect(first).toBe(second); }); + + it('reports resident bytes for both tiers', async () => { + const source = createSource(createFlakyStore()); + expect(source.parquetTableBytes.byteLength).toBe(0); + expect(source.parquetTableCache.byteLength).toBe(0); + + await source.loadParquetTable(PARQUET_PATH); + + expect(source.parquetTableBytes.byteLength).toBe(PARQUET_BYTE_LENGTH); + // Three float32 values plus Arrow's own buffers — the exact figure is Arrow's + // business; that it is counted at all is the point of ADR 0005 rung 1. + expect(source.parquetTableCache.byteLength).toBeGreaterThan(0); + }); + + it('bounds the encoded tier, and refetches what it evicted', async () => { + const store = createFlakyStore(); + const source = createSource(store, { encodedMaxBytes: PARQUET_BYTE_LENGTH }); + + await source.loadParquetBytes(PARQUET_PATH); + expect(source.parquetTableBytes.byteLength).toBe(PARQUET_BYTE_LENGTH); + + await source.loadParquetBytes(OTHER_PARQUET_PATH); + expect(source.parquetTableBytes.byteLength).toBe(PARQUET_BYTE_LENGTH); + expect(source.parquetTableBytes.has(PARQUET_PATH)).toBe(false); + + store.get.mockClear(); + await expect(source.loadParquetBytes(PARQUET_PATH)).resolves.toBeInstanceOf(Uint8Array); + expect(store.get).toHaveBeenCalledWith(`/${PARQUET_PATH}`); + }); + + it('bounds the decoded tier', async () => { + // One byte: every table is oversized, so each admission evicts the last. + const source = createSource(createFlakyStore(), { decodedMaxBytes: 1 }); + + await source.loadParquetTable(PARQUET_PATH); + await source.loadParquetTable(OTHER_PARQUET_PATH); + + expect(source.parquetTableCache.size).toBe(1); + expect(source.parquetTableCache.has(OTHER_PARQUET_PATH)).toBe(true); + }); }); From b521c46de8f42541bd078e447213fa25da21b100 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 16:43:00 +0100 Subject: [PATCH 4/5] Fill fizarrita's chunk-cache seam (ADR 0005 rung 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureCodecWorkers()` called `enableWorkerChunkDecode()` with no options, so `cache` was undefined and fizarrita fell back to its no-op — the seam was exported, documented, typed end to end, and empty. There was no chunk cache at all, so every tile paid a network round-trip and a re-decode each time it came back into view. Filling it is a pure win: nothing to trade against a cache that did not exist. It now takes a `ByteLruCache`, which satisfies fizarrita's `ChunkCache` structurally — `get` is the lookup and the recency update in one, which is exactly what that interface asks for. Default 256 MB, overridable on the first call; `getChunkCache()` exposes it for inspection and `clear()`. `RasterElement.getStore()` is memoized, and that is the load-bearing half. fizarrita keys chunks `store_N:{path}:{chunkKey}` with `N` from a `WeakMap` on the store instance, while `createPrefixedStore` returns a fresh object literal per call — so a view per caller would key one chunk differently per view and fill the cache with duplicates that never hit. Two upstream gaps left standing and documented: absent chunks are cached as full zero-filled arrays (bounded now, but still real bytes), and the chunk path still has no in-flight dedup. Co-Authored-By: Claude Opus 5 --- .changeset/fill-chunk-cache-seam.md | 38 ++++++++++ packages/core/src/models/index.ts | 12 ++- packages/core/tests/parquetTableCache.spec.ts | 2 +- .../core/tests/rasterElementStore.spec.ts | 69 +++++++++++++++++ packages/vis/src/codecWorkers.ts | 76 ++++++++++++++++++- packages/vis/src/index.ts | 7 +- packages/vis/tests/codecWorkers.spec.ts | 74 ++++++++++++++++-- 7 files changed, 267 insertions(+), 11 deletions(-) create mode 100644 .changeset/fill-chunk-cache-seam.md create mode 100644 packages/core/tests/rasterElementStore.spec.ts diff --git a/.changeset/fill-chunk-cache-seam.md b/.changeset/fill-chunk-cache-seam.md new file mode 100644 index 00000000..91d54e0f --- /dev/null +++ b/.changeset/fill-chunk-cache-seam.md @@ -0,0 +1,38 @@ +--- +'@spatialdata/vis': minor +'@spatialdata/core': patch +--- + +Give zarr imagery a decoded chunk cache +([ADR 0005](https://github.com/Taylor-CCB-Group/SpatialData.js/blob/main/docs/adr/0005-memory-accounting-before-management.md) +rung 3). There was not one before — not an undersized one, none at all. + +fizarrita has always accepted a `{ get, set }` cache on `getWorker`, and +`zarrextra` has always plumbed it through `enableWorkerChunkDecode({ cache })`. +`ensureCodecWorkers()` called that with no options, so `cache` was `undefined` +and fizarrita fell back to its no-op. The seam was exported, documented, typed +end to end, and empty. Every tile therefore paid a network round-trip *and* a +re-decode every time it came back into view. + +It is now filled with a byte-bounded LRU, default 256 MB, overridable with +`ensureCodecWorkers({ chunkCacheMaxBytes })` on the first call. `getChunkCache()` +returns it for inspection (`byteLength` is what it currently holds) or for +`clear()`. + +`RasterElement.getStore()` is now memoized, and that is load-bearing rather than +tidiness: fizarrita keys chunks as `store_N:{array path}:{chunk key}`, where `N` +comes from a `WeakMap` on the **store instance**, and `createPrefixedStore` +returns a fresh object literal on every call. Handing out a new view per caller +would give one chunk a different key per view, so the cache would fill with +duplicates and never hit. One stable view per element is what makes it a cache. + +Two limits worth stating plainly: + +- **Absent chunks are cached as data.** fizarrita materialises a full zero-filled + typed array for a missing chunk and caches it like any other, so a sparse array + can spend real bytes on nothing. The byte bound makes that survivable; it does + not make it free. +- **In-flight requests are still not deduped.** fizarrita reads the cache while + building its task list and writes back only after the worker returns, so two + concurrent requests for the same chunk both fetch and both decode. That is an + upstream gap this seam cannot close. diff --git a/packages/core/src/models/index.ts b/packages/core/src/models/index.ts index 0fd2949c..d5041047 100644 --- a/packages/core/src/models/index.ts +++ b/packages/core/src/models/index.ts @@ -442,6 +442,7 @@ abstract class RasterElement extends AbstractSpat RasterAttrs > { readonly attrs: RasterAttrs; + private prefixedStore?: zarr.AsyncReadable; constructor(params: ElementParams) { super(params); @@ -487,9 +488,18 @@ abstract class RasterElement extends AbstractSpat * * Consumers that need codec-aware array loading should use this instead of * reconstructing a URL and letting downstream libraries create their own store. + * + * Memoized, and that matters beyond saving an object allocation: the chunk + * cache downstream is keyed **by store instance**. fizarrita assigns each store + * object an id from a `WeakMap` and builds keys as `store_N:{path}:{chunkKey}`, + * while `createPrefixedStore` returns a fresh object literal every call — so + * handing out a new view per caller would give the same chunk a different key + * per view, and the cache would fill with duplicates that never hit. One stable + * view per element is what makes the cache a cache. */ getStore(): zarr.AsyncReadable { - return createPrefixedStore(this.sdata.rootStore.zarritaStore, this.path); + this.prefixedStore ??= createPrefixedStore(this.sdata.rootStore.zarritaStore, this.path); + return this.prefixedStore; } /** diff --git a/packages/core/tests/parquetTableCache.spec.ts b/packages/core/tests/parquetTableCache.spec.ts index 20fe167a..508d1f0b 100644 --- a/packages/core/tests/parquetTableCache.spec.ts +++ b/packages/core/tests/parquetTableCache.spec.ts @@ -1,7 +1,7 @@ import { tableFromArrays, tableToIPC } from 'apache-arrow'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import type { ParquetCacheLimits } from '../src/Vutils.js'; import SpatialDataTableSource from '../src/models/VTableSource.js'; +import type { ParquetCacheLimits } from '../src/Vutils.js'; const PARQUET_PATH = 'points/cells/points.parquet'; const OTHER_PARQUET_PATH = 'points/nuclei/points.parquet'; diff --git a/packages/core/tests/rasterElementStore.spec.ts b/packages/core/tests/rasterElementStore.spec.ts new file mode 100644 index 00000000..6f65210a --- /dev/null +++ b/packages/core/tests/rasterElementStore.spec.ts @@ -0,0 +1,69 @@ +import { describe, expect, it, vi } from 'vitest'; +import { ATTRS_KEY } from 'zarrextra'; +import { ImageElement } from '../src/models/index.js'; + +/** + * The store view a raster element hands out has to be *the same object* every + * time, because the decoded chunk cache is keyed by store instance: fizarrita + * assigns each store an id from a `WeakMap` and builds keys as + * `store_N:{array path}:{chunk key}`, while `createPrefixedStore` returns a fresh + * object literal per call. A view per caller would key the same chunk differently + * per view — a cache that fills with duplicates and never hits. See ADR 0005 + * rung 3. + */ +function createTree() { + return { + images: { + morphology: { + [ATTRS_KEY]: { + multiscales: [ + { + datasets: [{ path: '0' }], + axes: [ + { name: 'y', type: 'space' }, + { name: 'x', type: 'space' }, + ], + }, + ], + }, + }, + }, + }; +} + +function createImageElement(zarritaStore: { get: (key: string) => Promise } = fakeStore()) { + return new ImageElement({ + sdata: { + rootStore: { tree: createTree(), zarritaStore }, + // biome-ignore lint/suspicious/noExplicitAny: minimal SDataProps test double + } as any, + name: 'images', + key: 'morphology', + }); +} + +function fakeStore() { + return { get: vi.fn(async () => null) }; +} + +describe('RasterElement.getStore', () => { + it('hands out one stable store view per element', () => { + const element = createImageElement(); + + expect(element.getStore()).toBe(element.getStore()); + }); + + it('gives different elements different views', () => { + expect(createImageElement().getStore()).not.toBe(createImageElement().getStore()); + }); + + it('still resolves keys under the element path', async () => { + const store = fakeStore(); + const element = createImageElement(store); + + // biome-ignore lint/suspicious/noExplicitAny: zarrita brands absolute paths + await element.getStore().get('/0/c/0/0' as any); + + expect(store.get).toHaveBeenCalledWith('/images/morphology/0/c/0/0', undefined); + }); +}); diff --git a/packages/vis/src/codecWorkers.ts b/packages/vis/src/codecWorkers.ts index 3abadec9..5a3f97d1 100644 --- a/packages/vis/src/codecWorkers.ts +++ b/packages/vis/src/codecWorkers.ts @@ -1,6 +1,46 @@ +import { ByteLruCache } from '@spatialdata/core'; import { enableWorkerChunkDecode } from 'zarrextra/workers'; +import type { Chunk, DataType } from 'zarrita'; + +/** + * Default ceiling for the decoded zarr chunk cache, shared across every element. + * + * Sized to hold a working set rather than a history: a few screenfuls of tiles + * across the scale levels a pan touches. Like the parquet ceilings this is a + * guess that bounds a cache, not a measured working set — see ADR 0005. + */ +export const DEFAULT_CHUNK_CACHE_MAX_BYTES = 256 * 1024 * 1024; + +export type EnsureCodecWorkersOptions = { + /** Override {@link DEFAULT_CHUNK_CACHE_MAX_BYTES}. Only read on the first call. */ + chunkCacheMaxBytes?: number; +}; let enabled = false; +let chunkCache: ByteLruCache> | undefined; + +/** Bytes a decoded chunk holds. */ +function chunkByteLength(chunk: Chunk): number { + // Numeric dtypes give a typed array, which reports `byteLength` for free — + // that structural match is the whole point of `MemoryReporting`. String dtypes + // give a plain array instead, and `length` is the conservative floor there: one + // byte per element at minimum. Reporting zero would make such entries invisible + // to the budget and so unevictable by size, which is the one way a bounded + // cache quietly goes back to being unbounded. + const data: { byteLength?: number; length: number } = chunk.data; + return typeof data.byteLength === 'number' ? data.byteLength : data.length; +} + +/** + * The decoded chunk cache, once {@link ensureCodecWorkers} has built it. + * + * Exposed for inspection — `byteLength` is what it is holding right now — and for + * `clear()`, which is how a host releases imagery it knows it is done with. + * `undefined` before the workers are enabled. + */ +export function getChunkCache(): ByteLruCache> | undefined { + return chunkCache; +} /** * Enable the bundled zarrextra codec worker once for browser-based vis components. @@ -8,13 +48,45 @@ let enabled = false; * This is called automatically by SpatialCanvas renderer paths. It is exported for * hosts that want to opt in before mounting UI, without risking repeated worker * pool replacement. + * + * ### The chunk cache + * + * fizarrita has always accepted a `{ get, set }` cache here and we always passed + * nothing, so it fell back to its no-op and *there was no chunk cache at all* — + * every tile paid a network round-trip and a re-decode on every pan back across + * ground already covered. Filling the seam is ADR 0005 rung 3, and a pure win: + * there is nothing to trade off against a cache that did not exist. + * + * `ByteLruCache` satisfies fizarrita's `ChunkCache` structurally, so it is passed + * as-is — its `get` is the lookup *and* the recency update, which is exactly what + * that interface wants. + * + * Two things worth knowing about what ends up in here: + * + * - **Absent chunks are cached as data.** fizarrita materialises a full + * zero-filled typed array for a missing chunk and caches that like any other, + * so a sparse array can spend real bytes on nothing. The byte bound is what + * makes that survivable rather than a leak. + * - **This does not dedupe in-flight requests.** fizarrita consults the cache + * while building its task list and writes back only after the worker returns, + * so two concurrent requests for one chunk still both fetch and both decode. + * That is an upstream gap, not something this seam can close. + * + * Cache keys are `store_N:{array path}:{chunk key}`, where `N` identifies the + * **store instance**. `RasterElement.getStore()` memoizes its prefixed view for + * exactly that reason; a fresh view per caller would key the same chunk + * differently per view and fill the cache with duplicates that never hit. */ -export function ensureCodecWorkers(): boolean { +export function ensureCodecWorkers(options?: EnsureCodecWorkersOptions): boolean { if (enabled || typeof Worker === 'undefined') { return enabled; } - enableWorkerChunkDecode(); + chunkCache = new ByteLruCache>({ + maxBytes: options?.chunkCacheMaxBytes ?? DEFAULT_CHUNK_CACHE_MAX_BYTES, + sizeOf: chunkByteLength, + }); + enableWorkerChunkDecode({ cache: chunkCache }); enabled = true; return enabled; } diff --git a/packages/vis/src/index.ts b/packages/vis/src/index.ts index 1205cbfd..ffa30796 100644 --- a/packages/vis/src/index.ts +++ b/packages/vis/src/index.ts @@ -21,7 +21,12 @@ export { SpatialLayer, spatialLayerPropsSchema, } from '@spatialdata/layers'; -export { ensureCodecWorkers } from './codecWorkers'; +export { + DEFAULT_CHUNK_CACHE_MAX_BYTES, + type EnsureCodecWorkersOptions, + ensureCodecWorkers, + getChunkCache, +} from './codecWorkers'; export { default as ImageView } from './ImageView'; export { default as Shapes } from './Shapes'; export { default as Sketch } from './Sketch'; diff --git a/packages/vis/tests/codecWorkers.spec.ts b/packages/vis/tests/codecWorkers.spec.ts index 33691a71..509efcdc 100644 --- a/packages/vis/tests/codecWorkers.spec.ts +++ b/packages/vis/tests/codecWorkers.spec.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Chunk, DataType } from 'zarrita'; const enableWorkerChunkDecode = vi.hoisted(() => vi.fn()); @@ -6,6 +7,27 @@ vi.mock('zarrextra/workers', () => ({ enableWorkerChunkDecode, })); +function installWorker() { + Object.defineProperty(globalThis, 'Worker', { + value: class TestWorker {}, + configurable: true, + }); +} + +/** A decoded chunk of `bytes` bytes, shaped like whatever fizarrita hands back. */ +function chunkOf(bytes: number): Chunk { + return { + data: new Uint8Array(bytes), + shape: [bytes], + stride: [1], + } as Chunk; +} + +/** The cache handed to fizarrita on the most recent call. */ +function passedCache() { + return enableWorkerChunkDecode.mock.calls.at(-1)?.[0]?.cache; +} + describe('ensureCodecWorkers', () => { beforeEach(() => { vi.resetModules(); @@ -14,22 +36,62 @@ describe('ensureCodecWorkers', () => { }); it('does nothing outside browser worker environments', async () => { - const { ensureCodecWorkers } = await import('../src/codecWorkers'); + const { ensureCodecWorkers, getChunkCache } = await import('../src/codecWorkers'); expect(ensureCodecWorkers()).toBe(false); expect(enableWorkerChunkDecode).not.toHaveBeenCalled(); + expect(getChunkCache()).toBeUndefined(); }); it('enables the bundled codec worker once', async () => { - Object.defineProperty(globalThis, 'Worker', { - value: class TestWorker {}, - configurable: true, - }); + installWorker(); const { ensureCodecWorkers } = await import('../src/codecWorkers'); expect(ensureCodecWorkers()).toBe(true); expect(ensureCodecWorkers()).toBe(true); expect(enableWorkerChunkDecode).toHaveBeenCalledOnce(); - expect(enableWorkerChunkDecode).toHaveBeenCalledWith(); + }); + + it('fills fizarrita chunk-cache seam, which was previously left empty', async () => { + installWorker(); + const { ensureCodecWorkers, getChunkCache, DEFAULT_CHUNK_CACHE_MAX_BYTES } = await import( + '../src/codecWorkers' + ); + ensureCodecWorkers(); + + const cache = passedCache(); + expect(cache).toBeDefined(); + expect(cache).toBe(getChunkCache()); + expect(cache.maxBytes).toBe(DEFAULT_CHUNK_CACHE_MAX_BYTES); + // fizarrita's `ChunkCache` contract is exactly these two. + expect(typeof cache.get).toBe('function'); + expect(typeof cache.set).toBe('function'); + }); + + it('accounts for the decoded bytes it holds, and bounds them', async () => { + installWorker(); + const { ensureCodecWorkers, getChunkCache } = await import('../src/codecWorkers'); + ensureCodecWorkers({ chunkCacheMaxBytes: 100 }); + + const cache = getChunkCache(); + expect(cache?.byteLength).toBe(0); + + cache?.set('store_0:/0:c/0/0', chunkOf(60)); + expect(cache?.byteLength).toBe(60); + + cache?.set('store_0:/0:c/0/1', chunkOf(60)); + expect(cache?.byteLength).toBe(60); + expect(cache?.has('store_0:/0:c/0/0')).toBe(false); + expect(cache?.get('store_0:/0:c/0/1')).toBeDefined(); + }); + + it('reads the ceiling only on the call that builds the cache', async () => { + installWorker(); + const { ensureCodecWorkers, getChunkCache } = await import('../src/codecWorkers'); + + ensureCodecWorkers({ chunkCacheMaxBytes: 100 }); + ensureCodecWorkers({ chunkCacheMaxBytes: 999 }); + + expect(getChunkCache()?.maxBytes).toBe(100); }); }); From bfbf793a75455c48c58aaf7d9fe0b5e65298d2a6 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 17:42:05 +0100 Subject: [PATCH 5/5] Review: guard the byte accounting, contain disposal errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ByteLruCache` trusted every number handed to it. A `NaN` ceiling or a `sizeOf` that returned `NaN` made every `resident > max` comparison false, silently disabling eviction for the life of the cache — a bounded cache quietly becoming an unbounded one, which is the exact failure it exists to prevent. Both are now refused at the one place a bad number can enter, along with Infinity and negatives. Disposal errors no longer abandon the operation that triggered them. Removal and byte bookkeeping already completed before `onDispose` ran, but a throwing callback could abort an eviction pass halfway — leaving the cache over its ceiling — or skip the rest of a `clear`. Eviction and clearing now run to completion and rethrow the first error afterwards. Test-only, from the same review: the "different elements different views" case now shares one backing store, so it asserts something about the per-element view rather than about two elements holding different stores; and the `as any` casts are gone from both new specs, verified with tsc rather than assumed (the specs are outside the package tsconfig, so the suite alone would not have caught it). Co-Authored-By: Claude Opus 5 --- packages/core/src/memory/byteLruCache.ts | 108 +++++++++++++++--- packages/core/tests/byteLruCache.spec.ts | 48 ++++++++ .../core/tests/rasterElementStore.spec.ts | 29 +++-- packages/vis/src/codecWorkers.ts | 25 +++- packages/vis/tests/codecWorkers.spec.ts | 2 +- 5 files changed, 181 insertions(+), 31 deletions(-) diff --git a/packages/core/src/memory/byteLruCache.ts b/packages/core/src/memory/byteLruCache.ts index 3759d6f1..640fac43 100644 --- a/packages/core/src/memory/byteLruCache.ts +++ b/packages/core/src/memory/byteLruCache.ts @@ -31,6 +31,24 @@ interface Entry { bytes: number; } +/** + * Reject a byte count that would corrupt the accounting rather than merely be wrong. + * + * `NaN` is the dangerous one: every `residentBytes > maxBytes` comparison against + * it is false, so a single `NaN` — from a ceiling passed through an unparsed + * config value, or from a `sizeOf` that met an unexpected payload — silently + * disables eviction for the lifetime of the cache. A bounded cache quietly + * becomes an unbounded one, which is the exact failure it exists to prevent. + * Infinity and negatives are the same class of mistake and equally cheap to + * refuse here, at the one place a bad number can enter. + */ +function assertByteCount(value: number, label: string): number { + if (!Number.isFinite(value) || value < 0) { + throw new RangeError(`${label} must be a finite, non-negative byte count; received ${value}`); + } + return value; +} + /** * A byte-bounded LRU cache that reports what it is holding. * @@ -67,11 +85,43 @@ export class ByteLruCache implements MemoryReporting { private residentBytes = 0; constructor(options: ByteLruCacheOptions) { - this.maxBytes = options.maxBytes; + this.maxBytes = assertByteCount(options.maxBytes, 'maxBytes'); this.sizeOf = options.sizeOf; this.onDispose = options.onDispose; } + /** `sizeOf`, with the result checked before it can reach the running total. */ + private measure(value: V): number { + return assertByteCount(this.sizeOf(value), 'sizeOf(value)'); + } + + /** + * Run `onDispose`, returning what it threw instead of throwing. + * + * Disposal is a courtesy at the end of a removal that has already happened; + * letting it propagate mid-loop would abandon an eviction pass partway and + * leave the cache over budget, or skip the rest of a `clear`. Callers finish + * the structural work, then rethrow. + */ + private disposeQuietly(value: V, key: string): unknown { + if (!this.onDispose) { + return undefined; + } + try { + this.onDispose(value, key); + } catch (error) { + return error; + } + return undefined; + } + + /** Drop an entry and its bytes, returning any error `onDispose` threw. */ + private removeEntry(key: string, entry: Entry): unknown { + this.entries.delete(key); + this.residentBytes -= entry.bytes; + return this.disposeQuietly(entry.value, key); + } + /** Resident bytes, maintained incrementally — never a scan of the residents. */ get byteLength(): number { return this.residentBytes; @@ -120,13 +170,14 @@ export class ByteLruCache implements MemoryReporting { this.entries.delete(key); this.residentBytes -= previous.bytes; } - const bytes = this.sizeOf(value); + const bytes = this.measure(value); this.entries.set(key, { value, bytes }); this.residentBytes += bytes; - if (previous) { - this.onDispose?.(previous.value, key); - } + const disposeError = previous ? this.disposeQuietly(previous.value, key) : undefined; this.evictToBudget(); + if (disposeError !== undefined) { + throw disposeError; + } } /** @@ -142,7 +193,7 @@ export class ByteLruCache implements MemoryReporting { if (!entry) { return; } - const bytes = this.sizeOf(entry.value); + const bytes = this.measure(entry.value); this.residentBytes += bytes - entry.bytes; entry.bytes = bytes; this.entries.delete(key); @@ -156,35 +207,62 @@ export class ByteLruCache implements MemoryReporting { if (!entry) { return false; } - this.entries.delete(key); - this.residentBytes -= entry.bytes; - this.onDispose?.(entry.value, key); + const disposeError = this.removeEntry(key, entry); + if (disposeError !== undefined) { + throw disposeError; + } return true; } - /** Drop everything, disposing each entry. */ + /** + * Drop everything, disposing each entry. + * + * Every entry is disposed even if one of them throws; the first error is + * rethrown once the cache is empty, so a single bad payload cannot strand the + * rest. + */ clear(): void { const disposing = [...this.entries]; this.entries.clear(); this.residentBytes = 0; - if (this.onDispose) { - for (const [key, entry] of disposing) { - this.onDispose(entry.value, key); + let firstError: unknown; + for (const [key, entry] of disposing) { + const error = this.disposeQuietly(entry.value, key); + if (firstError === undefined) { + firstError = error; } } + if (firstError !== undefined) { + throw firstError; + } } /** * Evict from the least-recently-used end until within budget, stopping while * one entry remains so that an oversized value is kept rather than refused. + * + * A throwing `onDispose` does not stop the pass — abandoning it halfway would + * leave the cache over its ceiling, which is worse than the failed disposal. + * The first error surfaces once the cache is back within budget. */ private evictToBudget(): void { + let firstError: unknown; while (this.residentBytes > this.maxBytes && this.entries.size > 1) { const oldest = this.entries.keys().next(); if (oldest.done) { - return; + break; + } + const entry = this.entries.get(oldest.value); + if (!entry) { + break; } - this.delete(oldest.value); + const error = this.removeEntry(oldest.value, entry); + if (firstError === undefined) { + firstError = error; + } + } + if (firstError !== undefined) { + throw firstError; } } } diff --git a/packages/core/tests/byteLruCache.spec.ts b/packages/core/tests/byteLruCache.spec.ts index baf7bc6c..9318f8c3 100644 --- a/packages/core/tests/byteLruCache.spec.ts +++ b/packages/core/tests/byteLruCache.spec.ts @@ -143,6 +143,54 @@ describe('ByteLruCache', () => { expect(cache.byteLength).toBe(100); }); + it('refuses a ceiling that would disable eviction', () => { + // NaN is the one that matters: every `resident > max` comparison against it + // is false, so the cache would report nonsense and never evict again. + expect(() => bytesCache(Number.NaN)).toThrow(RangeError); + expect(() => bytesCache(Number.POSITIVE_INFINITY)).toThrow(RangeError); + expect(() => bytesCache(-1)).toThrow(RangeError); + }); + + it('refuses a size that would corrupt the running total', () => { + const cache = new ByteLruCache({ maxBytes: 100, sizeOf: () => Number.NaN }); + + expect(() => cache.set('a', 'x')).toThrow(RangeError); + expect(cache.byteLength).toBe(0); + }); + + it('finishes evicting even when disposal throws', () => { + const disposed: string[] = []; + const cache = bytesCache(200, (_value, key) => { + disposed.push(key); + throw new Error(`dispose failed for ${key}`); + }); + cache.set('a', new Uint8Array(100)); + cache.set('b', new Uint8Array(100)); + + // Admitting 500 bytes has to evict both 100-byte entries to get under 200. + expect(() => cache.set('c', new Uint8Array(500))).toThrow('dispose failed for a'); + + expect(disposed).toEqual(['a', 'b']); + expect(cache.size).toBe(1); + expect(cache.byteLength).toBe(500); + }); + + it('finishes clearing even when disposal throws', () => { + const disposed: string[] = []; + const cache = bytesCache(1000, (_value, key) => { + disposed.push(key); + throw new Error(`dispose failed for ${key}`); + }); + cache.set('a', new Uint8Array(10)); + cache.set('b', new Uint8Array(10)); + + expect(() => cache.clear()).toThrow('dispose failed for a'); + + expect(disposed).toEqual(['a', 'b']); + expect(cache.size).toBe(0); + expect(cache.byteLength).toBe(0); + }); + it('satisfies MemoryReporting', () => { const cache: MemoryReporting = bytesCache(100); expect(cache.byteLength).toBe(0); diff --git a/packages/core/tests/rasterElementStore.spec.ts b/packages/core/tests/rasterElementStore.spec.ts index 6f65210a..8be424bb 100644 --- a/packages/core/tests/rasterElementStore.spec.ts +++ b/packages/core/tests/rasterElementStore.spec.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import { ATTRS_KEY } from 'zarrextra'; +import type * as zarr from 'zarrita'; import { ImageElement } from '../src/models/index.js'; /** @@ -31,20 +32,27 @@ function createTree() { }; } -function createImageElement(zarritaStore: { get: (key: string) => Promise } = fakeStore()) { +/** The narrowest thing satisfying `ConsolidatedStore['zarritaStore']`. */ +function fakeStore() { + return { + get: vi.fn(async (_key: zarr.AbsolutePath): Promise => undefined), + contents: vi.fn((): { path: zarr.AbsolutePath; kind: 'array' | 'group' }[] => []), + }; +} + +function createImageElement(zarritaStore: ReturnType = fakeStore()) { return new ImageElement({ sdata: { + source: 'test://sdata.zarr', rootStore: { tree: createTree(), zarritaStore }, - // biome-ignore lint/suspicious/noExplicitAny: minimal SDataProps test double - } as any, + }, name: 'images', key: 'morphology', }); } -function fakeStore() { - return { get: vi.fn(async () => null) }; -} +/** A chunk key rooted at the element, in zarrita's branded absolute-path form. */ +const CHUNK_KEY: zarr.AbsolutePath = '/0/c/0/0'; describe('RasterElement.getStore', () => { it('hands out one stable store view per element', () => { @@ -54,15 +62,18 @@ describe('RasterElement.getStore', () => { }); it('gives different elements different views', () => { - expect(createImageElement().getStore()).not.toBe(createImageElement().getStore()); + // One backing store, so the assertion is about the per-element *view* rather + // than about the two elements happening to hold different stores. + const shared = fakeStore(); + + expect(createImageElement(shared).getStore()).not.toBe(createImageElement(shared).getStore()); }); it('still resolves keys under the element path', async () => { const store = fakeStore(); const element = createImageElement(store); - // biome-ignore lint/suspicious/noExplicitAny: zarrita brands absolute paths - await element.getStore().get('/0/c/0/0' as any); + await element.getStore().get(CHUNK_KEY); expect(store.get).toHaveBeenCalledWith('/images/morphology/0/c/0/0', undefined); }); diff --git a/packages/vis/src/codecWorkers.ts b/packages/vis/src/codecWorkers.ts index 5a3f97d1..01408f49 100644 --- a/packages/vis/src/codecWorkers.ts +++ b/packages/vis/src/codecWorkers.ts @@ -12,7 +12,14 @@ import type { Chunk, DataType } from 'zarrita'; export const DEFAULT_CHUNK_CACHE_MAX_BYTES = 256 * 1024 * 1024; export type EnsureCodecWorkersOptions = { - /** Override {@link DEFAULT_CHUNK_CACHE_MAX_BYTES}. Only read on the first call. */ + /** + * Override {@link DEFAULT_CHUNK_CACHE_MAX_BYTES}. + * + * Read on the first call that actually enables the workers, and ignored + * afterwards — calls that no-op because `Worker` is unavailable read nothing, + * so a later call in a worker-capable context still gets to set it. Must be a + * finite, non-negative byte count; `ByteLruCache` rejects anything else. + */ chunkCacheMaxBytes?: number; }; @@ -22,11 +29,17 @@ let chunkCache: ByteLruCache> | undefined; /** Bytes a decoded chunk holds. */ function chunkByteLength(chunk: Chunk): number { // Numeric dtypes give a typed array, which reports `byteLength` for free — - // that structural match is the whole point of `MemoryReporting`. String dtypes - // give a plain array instead, and `length` is the conservative floor there: one - // byte per element at minimum. Reporting zero would make such entries invisible - // to the budget and so unevictable by size, which is the one way a bounded - // cache quietly goes back to being unbounded. + // that structural match is the whole point of `MemoryReporting`. And numeric + // is all this cache ever sees: the sole route in is zarrextra's + // `ZarrPixelSource`, i.e. OME-Zarr pixel data, which is rejected long before + // here if it is not a numeric raster. + // + // The `length` arm is therefore a floor for a payload that should not arrive + // rather than a real measurement of one — element count, not UTF-8 bytes. It + // exists because reporting zero would make such entries invisible to the + // budget and so unevictable by size, which is the one way a bounded cache + // quietly goes back to being unbounded. If string chunks ever do reach this + // cache, this needs to become a real measurement. const data: { byteLength?: number; length: number } = chunk.data; return typeof data.byteLength === 'number' ? data.byteLength : data.length; } diff --git a/packages/vis/tests/codecWorkers.spec.ts b/packages/vis/tests/codecWorkers.spec.ts index 509efcdc..f80f6594 100644 --- a/packages/vis/tests/codecWorkers.spec.ts +++ b/packages/vis/tests/codecWorkers.spec.ts @@ -20,7 +20,7 @@ function chunkOf(bytes: number): Chunk { data: new Uint8Array(bytes), shape: [bytes], stride: [1], - } as Chunk; + }; } /** The cache handed to fizarrita on the most recent call. */