Viewer transfer efficiency: v3 opens, probe sizing, chunk dedup, cache write-through - #271
Merged
Merged
Conversation
Measured against zarr.nemar.org (129-ch EEG stores): - Pin every zarr.open to open.v3: generic open tries v2 first on a fresh store, costing two serial 404s (.zattrs, .zgroup) before zarr.json on every viewer open (~1.9 s wall measured) and tripling every miss (view-level probes, missing events). Open of a 40-min store: 36 -> 15 requests; 172-s store: 5 -> 3 requests, zero 404s. - Size the first view-level probe batch from n_samples via new predictedViewLevelCount (producer decimates x4, stops under 250 samples; verified against live stores). Prediction only sizes the batch; discovery still verifies every level, so a producer change costs a couple of extra probes, never a wrong list. - dedupingFetch: share one in-flight fetch per URL+Range between the interactive path and the background preloader. Level-0 reads are 206 Range responses the browser cache never stores (~140 KB per chunk otherwise re-transferred). Shared request aborts only when every subscriber has aborted. Unit tests for both new functions (fake fetch at the transport boundary per repo testing policy); vitest/typecheck/lint/build green.
An interactive read that lands on the preloader's segment grid IS the segment the background walk would fetch for that index. Store it under the walk's own cache key so cache.has skips it, instead of the walk re-transferring the identical bytes right after the user viewed them (~430 KB per 10 s level-0 page on a 129-channel store, starting with the very first window of every open while preload is on). The renderer DC-removes on copies (cw.line.slice() / removeBandDc returns new arrays), so cached windows stay pristine -- same invariant the existing cache-hit path relies on.
A browser-level reduce-data preference (Save-Data / Chromium Data Saver) outranks a stored opt-in from a previous session: background preloading a whole recording (up to the configured cap, default 500 MB) is exactly what Save-Data asks sites not to do. Progressive enhancement: navigator.connection is Chromium-only; absence changes nothing. The gear toggle still enables preload for the current mount.
Deploying nemar-website with
|
| Latest commit: |
3526a44
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://286d1744.nemar-website.pages.dev |
| Branch Preview URL: | https://perf-viewer-transfer-audit.nemar-website.pages.dev |
Contributor
Author
|
Producer/edge-side companion filed: nemarOrg/nemar-cli#1178 (view-level chunk shape, declared pyramid, Range edge-caching, tokened-metadata TTL, index.json weight). |
neuromechanist
changed the base branch from
feature/issue-256-epic-viewer-workbench
to
staging
September 1, 2026 19:14
Closed
5 tasks
Review-panel fixes on the transfer work (PR #271): - openNode: v3-first open with a v2 legacy escape hatch. The fast path stays pinned to open.v3; a NotFound falls back to open.v2 only until the store's first successful v3 read (a store is entirely one format), so misses on a working v3 store never pay the v2 404 pair again. - discoverViewLevels: a non-404 probe failure (retry-exhausted 5xx, 403 from an expired token) now throws ViewLevelDiscoveryError carrying the levels that DID open; openGroup keeps them and sets a new viewLevelsDegraded flag on the GroupHandle, so callers can distinguish 'discovery broke' from a genuinely short pyramid. Fulfilled sibling probes in a failed batch are kept instead of discarded. - discoverViewLevels takes openGroup's already-resolved nSamples (attrs with level-0-shape fallback) instead of re-deriving with a NaN fallback, removing the 12-probe worst case. - retryingFetch's aborted-under-transient-status error now states that the status may be a real backend error, not an effect of the cancellation (dedup's refcount abort can race a real 429/5xx). - Comment fixes: VIEW_PROBE_FOLLOWUP's job is confirming the real end (it runs on exact predictions too); discoverViewLevels docstring moved onto the function; isNotFound documented (tagged error preferred, regex fallback); dedup doc notes the useSuffixRequest coupling. New tests: fake v3 store harness driving the real reader pipeline (exact/undershot prediction probe counts, degraded discovery keeps siblings + sets the flag, no v2 requests on a v3 store), dedup fresh-fetch-after-rejection, and dedup(retry) composition sharing one retry cycle between concurrent callers.
Review-panel CRITICAL fix (PR #271): the write-through key was built from chanStart/chanCount/windowLengthS re-read AFTER the awaited network read -- live closure state the channel scrollbar or a window-length change can mutate mid-flight, which would store the OLD window's data under the NEW range's key and make a later cache hit render the wrong traces. The gate + key derivation now live in a pure exported helper, writeThroughKey (prefetch.ts, beside prefetchCacheKey which moved there so the walk's keyFor and the write-through provably share one key scheme). readFrame captures every geometry input into locals before the await and passes only those. The put()'s boolean is no longer discarded: a refusal (single window larger than the whole cache cap) logs a console.warn instead of silently disabling the optimization. Tests: writeThroughKey boundary/off-grid/partial-trailing/disabled cases, key equality with prefetchCacheKey for the same segment, and an oversized put leaving resident cache entries intact.
Review-panel HIGH fix (PR #271): hardened/privacy browsers can make the navigator.connection accessor itself throw (fingerprinting countermeasures); unguarded, that exception propagated out of loadPreloadEnabled and took down the whole viewer mount. saveDataRequested now try/catches the access and treats a throw as 'no signal'. Both settings readers are exported and covered by new boundary-fake tests (real-shape navigator/localStorage stand-ins, restored after each test): Save-Data on beats a stored opt-in, Save-Data off honors it, a throwing accessor degrades to the stored value, and absent storage defaults off.
Contributor
Author
|
Review-panel dispositions — all 16 findings addressed in e0e371a, 2c6ed05, 3526a44 (gates green on the tip: 1379/1379 tests, typecheck 0 errors, lint clean, build green; live re-verification against zarr.nemar.org shows identical request counts to the pre-review after-numbers). CRITICAL
HIGH
MEDIUM
TESTS
COMMENTS
Verified-sound items (refcounted abort lifecycle, Range keying, dedup map self-heal, misprediction degradation, Save-Data precedence design, byte accounting, VERSION_COUNTER analysis): no changes, per the panel. |
This was referenced Sep 1, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Data-transfer and smoothness audit of the signal viewer's zarr read path, with the website-side wins implemented. Producer/edge-side findings are filed separately in nemar-cli (link below once filed) — the biggest lever (view-level chunk granularity) lives there, not here.
All numbers are real measurements against live
zarr.nemar.org, driving the actualstore.tsreader through an instrumented fetch (bun; no browser HTTP cache, which matches the worst case — level-0 Range responses are never browser-cached anyway).Audit numbers (before)
Store A:
on005514sub-NDARAC589YMB_task-DespicableMe_eeg.zarr— 129 ch, 250 Hz, 172 s, 4 view levels.Store B:
on005514sub-NDARXZ850KHQ_task-contrastChangeDetection_run-1_eeg.zarr— 129 ch, 250 Hz, 2430 s (40.5 min), 6 view levels..zattrs+.zgroup404 serially beforezarr.json(zarrita's genericopentries v2 first on a fresh store); 3.6 s wall from heremax-age=86400)The request storms in the last three rows are producer chunk-shape issues (constant seconds per chunk instead of constant columns) — filed upstream, not fixable in the reader.
What this PR changes (with after numbers)
openNodeinstore.ts). The fast path isopen.v3: the store contract is Zarr v3 only, and zarrita's version-sniffingopencost 2 serial 404 round-trips before first metadata on every viewer open (~1.9 s wall measured) and tripled every probe miss. A NotFound falls back toopen.v2only until the store's first successful v3 read (a store is entirely one format), so the legacy safety net never re-adds the 404 pairs on a working v3 store. Caveat on scope: the v3-only observation was measured on two stores from one dataset (on005514), both from the same converter generation — hence the escape hatch rather than a bare pin.n_samples(new purepredictedViewLevelCount;openGrouppasses its already-resolved sample count, so the 12-probe fallback is unreachable in practice). The constants encode producer conventions verified on the two measured stores; prediction only sizes the batch — discovery still verifies every level, so a producer change degrades to a couple of extra probes, never a wrong level list. A non-404 probe failure (retry-exhausted 5xx, expired-token 403) now throwsViewLevelDiscoveryErrorcarrying the levels that did open; the group keeps them and setsviewLevelsDegraded, so "discovery broke" is distinguishable from a genuinely short pyramid.dedupingFetch, wrapped outsideretryingFetchinmakeStore): one shared fetch per URL+Range across the interactive path and the Viewer: optional full-recording background streaming with capped cache #254 preloader. Level-0 chunks are 206 responses the browser cache never stores, so an overlap was a full ~140 KB re-transfer per chunk. Shared request aborts only when every subscriber has aborted; each subscriber gets an independently consumable clone.writeThroughKeygate (prefetch.ts, sharingprefetchCacheKeywith the walk'skeyFor): a grid-aligned interactive read is the segment the background walk would fetch — without this, enabling preload re-fetched the exact window the user was already looking at on every (re)target (~430 KB per 10 s level-0 page, starting with the first window of every open).readFramecaptures every geometry input before the awaited read so mid-flight scrolling can never store one window's data under another window's key, and a refusedput(window larger than the whole cap) warns instead of silently disabling the optimization.loadPreloadEnabled()returns false whennavigator.connection.saveDatais set — a browser-level "reduce data" preference outranks a stored opt-in from a previous session. Chromium-only signal; absence — or a hardened browser making the accessor itself throw — degrades to "no signal".A side effect of (1): the hover/click metadata warmup (
prefetchZarrStoreMetadata) now actually shortens the critical path — before, the open's first two requests (.zattrs/.zgroup) were unwarmed, uncacheable 404s sitting in front of the warmed URLs.Evaluated and deliberately not changed
pickViewLeveltuning: measurements show it already picks the coarsest level satisfying ≥1 sample/px, and level-0 selection is bounded to <16 s windows at 250 Hz (~439 KB) by the existingLEVEL0_MAX_SAMPLEScap. Reducing level-0 over-fetch further (2.5–4 samples/px worst case) needs denser pyramid spacing — a producer decision, noted upstream.Test plan
bun run test1379/1379,typecheck0 errors,lintclean,buildgreen.it()blocks, all boundary fakes per repo testing policy (no business-logic mocks):predictedViewLevelCount— 5 (pinned to the two measured stores' real pyramids plus edge cases);dedupingFetch— 10 (clone independence, partial/total abort lifecycle, post-abort and post-rejection rejoin, pre-aborted caller, anddedupingFetch(retryingFetch(...))composition sharing one retry cycle between concurrent callers);viewLevelsDegraded); also asserts zero v2 fallback requests on a v3 store;writeThroughKey— 6 (key equality withprefetchCacheKeyfor the same segment, off-grid/partial-trailing/disabled nulls, oversized put leaves resident entries intact);saveDataRequested/loadPreloadEnabledprecedence both ways, throwingnavigator.connectionaccessor degrades instead of crashing).zarr.nemar.orgafter the changes (numbers above re-verified on the review-fix tip: open 3 req / no 404s, dedup confirmed at 4 requests for two concurrent identical level-0 window reads).