Repository navigation
Add operation-scoped RAM statistics across supported backends - #144
Conversation
tavateva
left a comment
tavateva
left a comment
There was a problem hiding this comment.
Early, non-final review of f732917 (draft), graded against #139. Requested by the owner as a COMMENT review. Explicitly out of scope: all performance results, including the native Windows +4.10% diagnostic, which await the real-data runs and an owner decision. Nothing below evaluates those numbers.
Verification so far
- Ran: the full Linux Python suite, 327 passed and 3 platform skips. A focused Linux shared-memory probe (below).
- Not run: Windows/macOS readers (relied on CI and the body), and a Docker run.
- Read in full:
memory_statistics.py,memory_observer.py,memory_mappings.py, and the lifecycle wiring inrun_statistics.py/server_handlers.py.
The lifecycle integration looks right:
- Collection starts and stops around
Pipeline.process()inside_process, beforecapture_volumes. - Startup failure degrades to explicit unavailability.
KeyboardInterruptduring startup cleans up and propagates.- Disabled reporting starts no collector.
mem_monitoris retired.
Blocking (as of this draft)
1. Linux/Docker headline misses resident shared segments that no sampled process has touched
process_tree_pss_bytes sums each process's PSS. PSS only counts pages a process has faulted into its own page tables. A shared segment that is resident in /dev/shm but not currently touched by any sampled process contributes nothing.
- Probe: a child fills a 32 MiB
SharedMemorysegment and exits, while the parent keeps the segment mapped but never touches it./dev/shmusage: 32 MiB.- Process-tree PSS delta: about 4 MiB (noise).
- Where slicing creates this state:
download_volumeallocates each chunk withTermedNPArrayand fills it in the download worker. That worker's handle closes once the result is pickled back.dl_completedstores the unpickledSharedNPArrayinvolume_cache.volumes[index]. The coordinator maps it but does not touch it.- The pages are attributed only once a slice worker starts reading.
- Every downloaded chunk waiting for a free slice worker is therefore resident workload memory that the headline does not see. That queue is a main contributor to slicing's peak.
- Not covered by the tests:
test_linux_tree_pss_counts_a_live_shared_mapping_oncecovers a mapping that is touched in both processes, but not this state. - Not covered by the docs: nothing in
run-statistics.mddeclares the exclusion. - I did not measure the in-pipeline magnitude. This reviewer's container has a 64 MB
/dev/shm, which cannot run the 384³ slice. - Other backends, unverified:
- Windows working-set accounting likely has the same blind spot by the same mechanism.
- macOS backing-residency (
mincore) appears to cover it by construction.
This is the default deployment's metric, and #139 requires measuring the workload, including shared-memory state, without silently partial coverage.
- Closure condition: the Linux headline either covers operation-owned shared segments that are resident while untouched, or the record and docs name this exclusion explicitly as a coverage limitation and the owner accepts it against #139.
- Narrowest known option: per-process USS (
memory_full_info().uss, from the same read) plus the allocated bytes (st_blocks × 512) of the operation's live/dev/shmsegments.SharedNPManageralready enumerates those segments through its termed-segment list.- USS excludes shared pages, so nothing is counted twice.
- It is a different quantity from PSS and needs its own metric name.
2. Outstanding #139 validation (already acknowledged in the body)
Not new findings; listed so the draft's remaining acceptance is in one place:
- Default Docker deployment validated on a Linux host and a macOS host (only Windows/WSL so far, and only CLI-in-container rather than volume-server delivery).
- A real native macOS collector/pipeline run beyond the focused CI tests.
Non-blocking
- Pre-implementation disclosure is not on the record. #139 requires the backend matrix, headline metrics, sampling strategy, lifecycle, reset/concurrency assumptions and expected cost to be documented before implementation. The body says these were "disclosed before implementation", but #139 has no comments and its only body edit is the overhead amendment. Minimum response: link where they were disclosed, or post them on #139.
- Shape question for the owner's Windows decision, independent of any numbers. The page-level union on Windows/macOS is the largest and most complex part of the change: per-process address manifests, a process-global weakref registry, JSON IPC,
ctypesaddress derivation, and full working-set snapshots. It is also the part tied to the unresolved Windows question. A segment-level metric is materially leaner: per-process private bytes plus the size or residency of the operation's live shared segments, which the manager already knows. With its own name, it would also close finding 1 on all backends. Minimum response: none for this draft. Worth weighing before the Windows decision commits the PR to the page-union design. - No closing keyword. The body says "for #139" rather than "Fixes #139", so the merge will not close the issue. Ignore this if that is intentional.
e8fd4fd to
be87951
Compare
|
Full real-data benchmark results for all three annotation paths, including both short paths and the long path, on Linux and native Windows. Revision and comparison scope: Linux was measured at Method
Linux / WSL2: all path medians (f732917)
Full raw results, including external pipeline times, output headers and all memory summaries Native Windows: all path medians (be87951)
Full raw results, including external pipeline times, output headers and all memory summaries Interpretation
Every paired data-operation timing, including both short paths (seconds)Pair numbers identify matched trials. Actual chronological order was off/on for pair 1 and on/off for pair 2.
All enabled-operation RAM peaks, coverage and collector costsRequested interval: 1 second. Peaks below are integer bytes. C/I/D = complete/incomplete/discarded sweep counts; incomplete sweeps do not contribute to a peak. CPU time is collector CPU, not additive wall-time overhead. Observed interval and maximum sweep are seconds. Linux reports Linux / WSL2
Native Windows
All 12 Windows reports are Coverage limitation remains open: queued resident shared backing can be absent from Linux PSS and Windows working-set scans after the producer closes when no remaining process has touched the pages. An untouched manager mapping keeps the allocation alive but does not fix that accounting gap. Successful sampling does not establish complete backing-residency coverage. Warmups (excluded from paired overhead calculations)Each is the one planned off-mode slicing warmup for its path; all succeeded. Times are the data-operation step, not the whole warmup process.
Provenance and preserved earlier attempts The new Windows batch is a fresh complete batch, not a selection of successful retries from the old one. The original Windows batch completed 5 workflows, failed 7 slicing trials and skipped their 7 dependent backprojections; 2 of its 4 completed enabled operations had unavailable RAM. Those outcomes remain recorded separately. The preliminary Linux attempt used a broader timing boundary and was stopped; it is not included in the Linux table above.
Benchmark method and interpretation. Remaining native macOS pipeline / Docker host-delivery validation and the post-rebase Linux rerun are not supplied by these results. |
14078d1 to
60e0dd9
Compare
|
Fresh real-data benchmarks for the current simplified implementation at 10145cd, on Linux and native Windows. This supersedes the performance interpretation of the earlier working-set/page-tracking benchmark, which does not measure this implementation. Method: all three original Salivary Duct annotation paths; 120 × 120 uint16 slices at mip 0; adaptive spacing 1 / ratio 0.5; 128 target slices per box; backprojection uses 160³ chunks, zlib and separate output planes. Short 2 = Linux and Windows ran sequentially on the same host. Each platform ran two off/on pairs per path after one off-mode slicing warmup: pair 1 off → on, pair 2 on → off. Each treatment backprojects its own sliced output. Twelve workers, one BLAS/OpenMP thread, shared cached chunks; no pixel hashing/comparison. On enables reporting and memory collection together; off disables both. The Windows runtime retains the previously disclosed temporary CloudFiles zero-width-interval workaround identically in both treatments; it is not a shipped dependency change. Timing boundary: the tables use the existing slicing/backprojection data-processing step, excluding earlier parsing, geometry and source/cache setup, sampler startup/shutdown, and final statistics-report formatting/writing/delivery. The timed step includes its worker/manager startup and shutdown, scientific downloads, computation, output writes and intermediate cleanup. RAM collection covers the broader Median change is 100 × (on median / off median − 1); MAD is median absolute deviation within each treatment. Both come from the benchmark's existing summary fields; Δ seconds is on median minus off median. Two pairs on a shared development host do not establish a 1% upper bound or identical behavior across platforms. Negative differences indicate measurement variation, not an instrumentation-induced speedup. Linux / WSL2 — Python 3.11.13,
Native Windows — Python 3.11.9,
Acceptance update after review: the owner decision on #139 limits 1% engineering acceptance to the measured longer runs; the short-run costs and qualifications below remain unchanged. Interpretation
What memory can be missed:
Every pair: data-operation timings in seconds
All 24 enabled-operation memory reports, coverage and collector costC/I/D = complete/incomplete/discarded sweeps. Only complete in-window samples contribute to the peak; partial/unavailable reports are shown explicitly. Peak MiB uses 2²⁰ bytes. Window is the broader pipeline window, not the data-step timing above. Each requested interval is one second. Complete-sample counts include setup: a non-null peak does not prove coverage of a short operation's maximum.
Pipeline failures or unavailable reports: none recorded; all 48 measured operations have timing records and all 24 enabled operations have non-null sampled memory peaks. Recorded incomplete-sweep errors by kind (examples are representative; each occurrence remains represented in C/I/D above):
Provenance: both batches require production SHA-256 Committed raw evidence: Linux, native Windows, and methods/provenance. The table now includes each run's maximum process count and observed interval. Separate short Docker delivery test; native macOS workflow validation is deferred on #139. |
tavateva
left a comment
tavateva
left a comment
There was a problem hiding this comment.
Review of 10145cd, graded against #139 (body last edited 2026-10-17 15:28) together with the PR body and both benchmark comments. This supersedes my early review of f732917.
Verdict: changes requested. The verdict rests on two acceptance items that Closes #139 would close while they are unmet: the overhead criterion and deployment validation. Neither is a code defect. Each closes with either evidence or an owner decision recorded on #139.
The redesign itself is a substantial improvement:
- Measurement now uses process counters plus one contribution per live manager-owned segment, derived from
_OwnedSharedMemoryTrackerunder its own lock. - The weak mapping registry, worker-context plumbing, TIFF wrappers and Windows page unions are gone.
- The production diff is now mostly the collector itself.
Verification
- Full Linux suite: 352 passed, 2 skipped. CI: 10/10 green.
- The early-review blocker is closed. I repeated the probe against the live manager and
TreeSampler: a pool worker fills a 32 MiBTermedNPArray, then closes it, while the parent holds an untouched attachment.- The sampled total rose about 32 MiB above the baseline once the segment was queued: +35.3 MiB, less the 3.6 MiB of worker growth that remains after release.
- It dropped back after
remove_termed, and/dev/shmwas empty afterwards. - So
Pss − Pss_Shmemplus managedmincoreresidency counts a queued, untouched buffer once, with no double count from the readers.
- Relied on: the Windows/macOS readers (CI) and the author's real-data benchmark comment.
Blocking
1. The combined overhead criterion is not met, and no owner decision is recorded
#139 applies "the same combined 1% in-operation engineering target, interpreted alongside median absolute deviation and the owner-approved variability-aware assessment". Its last criterion says that if the collector cannot meet both scope and overhead, the PR should "revise the design or obtain an explicit scope decision". The current-implementation benchmark (comment) reports:
- Linux:
- Short 2 slicing: +8.24%, Δ +0.379 s.
- Short 2 backprojection: +7.63%, Δ +0.306 s.
- Short 3 slicing: +11.69%, Δ +0.498 s.
- All three were slower in both pairs, with median differences larger than both treatment MADs.
- Native Windows: Short 3 slicing, +6.15% (Δ +1.320 s), was slower in both pairs and exceeded both MADs.
Under the #138 rule ("an apparent increase smaller than the observed MAD … is accepted"), these do not qualify. The PR body states this itself ("they do not support a blanket negligible-overhead claim"), but still closes #139. #139 has no comments, and its body has not been amended to accept the result.
- Closure condition: either an owner decision on #139 that accepts these costs (or scopes the target, for example to the long path or to a duration threshold), or a rerun that meets the criterion as written.
- Narrowest known option: the owner decision. The data-step Δ on the short paths is a roughly fixed 0.3–0.5 s against a 4–5 s step, so the percentage falls as data size grows. That has the same shape as the #138 fixed-cost discussion.
- Note: two pairs give a weak MAD. That argues for more pairs or an owner judgement, but it does not satisfy the criterion as written.
2. Deployment validation required by #139 is absent for this head
#139: "Validate the default Docker deployment on Linux, Windows, and macOS hosts and the supported native backends." For 10145cd:
- There is no Docker run on any host. The body does not claim one, and the earlier WSL CLI-in-container run predates the redesign.
- There is no native macOS pipeline run; the body says "Native macOS pipeline cost remains unmeasured".
The Linux metric now depends on smaps_rollup carrying Pss_Shmem and on mincore of /dev/shm segments. Inside the default container both are plausible but unexercised. A missing Pss_Shmem raises NotImplementedError, so the failure would be explicit, but it is still untested on the default deployment.
- Closure condition: for this head, a Docker slice and backprojection on each host OS with memory reports present and delivered, plus one native macOS slice and backprojection. Alternatively, an owner decision on #139 that defers named items to a follow-up, with
Closeschanged to match.
Non-blocking
- The current-implementation raw results are not committed, while superseded ones are.
- None of the 11 files in
benchmarks/results/carries the production digest66ec3675…that the comment cites. The README's #139 sections describe former collectors. - The data for the measurement this PR relies on exists only in a comment table. That table also omits per-run process counts, which #139 asks for and which the previous table included.
- Minimum response: commit the
10145cdartifact and point the README at it. Delete the superseded artifacts, or link them at their original revisions as was done for #140.
- None of the 11 files in
- Pre-implementation disclosure is still not on #139. The body again says the load-bearing decisions were "disclosed before implementation", but #139 has no comments. Minimum response: link where they were disclosed.
- Leftover diff noise from the removed plumbing:
helpers/mem.pygains two blank-line-only additions.backproject_pipeline.py(+6 −2) andslice_parallel_pipeline.py(+2 −2) change only formatting, such as trailing commas and line splits around executor construction. Neither pipeline file has a functional change any more.- Minimum response: revert those hunks.
- The Windows managed term is "allocated", so untouched pages can be overcounted. This is declared, and named distinctly. No action is needed; it is noted because the Windows peaks (1.6–2.6 GiB) are above Linux on the same paths, and readers should not take the gap as a real difference.
|
In response to the review of
|
tavateva
left a comment
tavateva
left a comment
There was a problem hiding this comment.
Re-review of 3e91580, diffed against the previously reviewed 10145cd, together with the new PR comment and the #139 body edit and comment (2026-10-17 23:23–23:24).
Verdict: approve. Both blockers are closed on the record.
- Overhead criterion: closed by owner decision.
- The #139 body now limits the combined 1% engineering acceptance to longer runs and accepts the long Salivary Duct results (checked item).
- The short-path slowdowns remain disclosed, and the issue explicitly says no blanket negligible-overhead claim follows from them.
- The issue and the PR now describe the same acceptance, which was the closure condition.
- Deployment validation: closed by evidence plus a recorded deferral.
- A Windows/WSL2 Docker slice → backprojection through the real volume server delivered both reports. The raw evidence is in
issue-139-docker-review.json. - Native-Linux-host Docker, macOS-host Docker and native macOS remain open as named checklist items on #139.
Closes #139has been removed, so the merge leaves those items open, which matches the second closure option.
- A Windows/WSL2 Docker slice → backprojection through the real volume server delivered both reports. The raw evidence is in
All four non-blocking notes are resolved:
- Current results committed: the
10145cdLinux/Windows raw results and provenance are in the repo and carry production digest66ec3675…. The README now points to them, superseded artifacts are linked at their historical revision, and the benchmark comment includes process counts and intervals. - Design disclosure: recorded on #139, and explicitly labelled as a retrospective record.
- Churn removed: both pipeline files now match
mainexactly, and the two blank lines inmem.pyare gone.10145cd..3e91580makes no other production-code change. - Windows "allocated" term: documented on #139.
Verification on 3e91580: full Linux suite 352 passed, 2 skipped. CI is green (10/10). The runtime evidence carried forward from the previous review still applies, because the production code is unchanged apart from the reverted formatting: the queued, untouched 32 MiB segment counted once and released cleanly.
2 participants
Adds operation-scoped memory measurements to the existing run-statistics reports for #139. Slicing and backprojection report a sampled peak, coverage and collection cost through the same
--verbosefiles and server/Docker delivery paths as timing statistics.The collector now uses process counters plus one contribution per live manager-owned shared buffer. It no longer tracks worker mappings or TIFF mappings, enumerates Windows working-set pages, or adds resident TIFF backing absent from process counters.
Implementation
Pss − Pss_Shmem, plus each live managed segment's resident bytes frommincorePrivateWorkingSetSize, plus each live managed segment's allocated sizemincoreLinux's subtraction avoids double counting shared pages even with one reader; the managed residency term includes queued resident buffers after their producer closes. Windows uses the fast
PROCESS_MEMORY_COUNTERS_EX2counter and explicitly labels its managed term allocated, not resident: unused or paged-out managed pages remain included. It requires 64-bit Windows 10/11 22H2 with September 2023 updates or later. macOS retains a region walk; excluding the manager's own ranges prevents sole-owner double counting.Pipeline.process(), excluding itself. The default interval is one second. Disabled reporting starts no collector._OwnedSharedMemoryTrackerpublishes derived allocation snapshots at its existing allocation/registration/release boundaries. Manager initialization receives the shared observation session; workers and array attachments need no reporting hooks.There is no extra resident file-cache accounting. Ordinary mapped-file pages can still contribute through the OS process counter. Platform metrics have different shared-memory semantics and are named accordingly; they are not interchangeable measurements of exactly the same RAM. Only live manager-owned segments are added. Overlapping operations share a process-tree scope, not isolated per-job accounting; managers created before an observation session are not retroactively enrolled. Production pipelines create their managers inside that window.
Shared-basis audit
Taskowns run identity/outcome;RunStatisticsowns reporting and the collector window. Memory extends that record and its existing output paths, with no parallel run/report authority.PipelineandPipelineStepretain execution, dispatch and timing ownership; their observation-context fields and executor initializer forwarding were removed._OwnedSharedMemoryTracker._segmentsexclusively owns managed identities, live handles and release policy. Snapshots derive names/sizes and, on macOS only, owner addresses directly under that owner's lock. The previous attachment/TIFF weak registry, cached address records, constructor/close hooks and per-worker manifests were removed. Snapshot JSON is a one-way observation boundary; execution never reads it and no persistent allocation mirror is maintained. Native residency queries take temporary read-only views without unlink authority.memory_ipc.observation_file_ioremains the single bounded retry facility for snapshots and collector control/result files. The existing shared-session seam now serves only manager observations on all supported backends.Validation
Review follow-up
3e91580: committed the current10145cdLinux/Windows raw results, provenance and all per-run process counts; historical artifacts remain linked at their original revision. Evidence and methods. Removed formatting-only changes frommem.pyand the two data pipelines; all 46 production Python ASTs still match the benchmarked revision.Short Docker delivery check: Short 2 slicing then backprojection through the existing Windows/WSL2 development container and real Electron-hosted volume server. Both tasks completed and both reports reached the host as readable UID 1000 / mode 0600 files. Slicing: 1,448.38 MiB, partial (29 complete / 1 incomplete sweep); backprojection: 1,751.84 MiB, sampled (10 complete). Both use the final Linux metric; source digests match the host, scientific scratch/report staging cleared, and no managed shared segments remained. This is a delivery check, not an overhead comparison or a newly built release-image test.
Final full Linux CI suite: 352 passed, 2 platform skips in 23.23 s; the configured helper coverage check passed at 99%. Includes reporting lifecycle and the actual fork-pool regression for released slice buffers. The existing Python 3.12 threaded-fork deprecation warning from that retained Fix fork-inherited shared-buffer retention during slicing #148 regression remains visible; this change does not introduce it. Local validation before the added clock regression was 351 passed, 2 skipped.
Native Windows focused suite: 115 passed, 6 platform skips in 36.36 s, with
ResourceWarningtreated as an error. Covers the native private counter, managed owner lifecycle, snapshot publication, collector subprocess and pipeline statistics; server HTTP tests are covered by the Linux suite (the isolated Windows runtime lacks theirhttpxtest dependency).After CI exposed coarse Windows clock timestamps, snapshot publication now uses a strictly advancing counter. The deterministic allocation/release regression and collector checks passed on Linux (42 passed) and native Windows (39 passed, 3 platform skips). The control-file test also permits additional native Windows sharing retries within the existing half-second bound.
Focused ownership tests verify that attachments/views/serialization create no extra allocation observations, buffers remain observable after producer closure, release removes them, and POSIX residency queries do not fault untouched pages. Native Windows private/shared allocation checks verify that shared mappings do not enter the private counter.
Final native CI and coverage passed: Linux and macOS each returned 111 passed, 2 skipped on both Python 3.11 and 3.12; Windows returned 92 passed, 5 skipped on each version. Native macOS pipeline cost remains unmeasured; no cross-platform 1% overhead bound is claimed.
Current-implementation Linux and native Windows benchmarks cover all three Salivary Duct paths, two off/on pairs each, with actual data-step timings, MAD, memory peaks, partial coverage and collector cost. All 48 measured operations completed. The results show repeatable short-path slowdowns. The owner acceptance decision scopes the combined 1% engineering target to longer runs, with inter-run variability and fixed startup/shutdown costs considered. The measured long path is accepted (Linux −1.764% slicing / −6.737% backprojection; Windows −7.260% / +0.288%). Short costs remain disclosed; this is neither a blanket negligible-overhead claim nor a statistical 1% bound. The comment also distinguishes sampling/lifecycle omissions from OS-controlled TIFF cache.
Full historical Linux and Windows benchmark results and raw reports remain available. Those overhead figures describe the former collectors, not this replacement. The subsequent Linux simple-versus-complete probes informed the chosen accounting; isolated query costs are not pipeline overhead. The benchmark guide now marks that distinction explicitly.
Advances #139. Per the owner decision, native macOS workflow/performance and macOS-host Docker delivery are deferred. A separate native-Linux-host Docker check also remains unperformed. #139 stays open for those named follow-up checks; the Windows/WSL2 Docker test is not presented as a complete three-host matrix.