-
Notifications
You must be signed in to change notification settings - Fork 0
Memory accounting before memory management: ADR 0005 rungs 1-3 #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
xinaesthete
wants to merge
5
commits into
main
Choose a base branch
from
claude/memory-accounting-handoff-adr-5a4377
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0ece94b
Evict rejected parquet table promises from the cache (ADR 0005 rung 2b)
xinaesthete a031d78
Adopt the MemoryReporting scalar (ADR 0005 rung 1)
xinaesthete 1956137
Bound both parquet caches by resident bytes (ADR 0005 rung 2)
xinaesthete b521c46
Fill fizarrita's chunk-cache seam (ADR 0005 rung 3)
xinaesthete bfbf793
Review: guard the byte accounting, contain disposal errors
xinaesthete File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.