Skip to content

#269: assembling a target held three copies of it, not one — 3.06x peak to 1.13x - #271

Merged
Polichinel merged 3 commits into
developmentfrom
fix/269-bound-the-shard-fetch
Aug 14, 2026
Merged

#269: assembling a target held three copies of it, not one — 3.06x peak to 1.13x#271
Polichinel merged 3 commits into
developmentfrom
fix/269-bound-the-shard-fetch

Conversation

@Polichinel

Copy link
Copy Markdown
Collaborator

Measured before changing anything, and the issue's diagnosis turned out to be a third of the story.

What the measurement showed

A synthetic run through the real TargetLease.load — producer-format .tap.zip shards, real read_shard, tracemalloc and peak RSS agreeing within 2% — at 36 shards x 20,000 cells x 200 samples:

before after
peak, tracemalloc 3.06x the delivered frame 1.13x
peak, RSS delta 3.02x 1.06x

The 3x was three roughly equal thirds, and #269 named only the first:

  1. every shard's bytes, resident together — the dict comprehension completed before the first was decoded;
  2. every decoded per-shard frame, held for the stack;
  3. the np.concatenate result, allocated while (2) was still alive.

The ratio held at both 12 and 36 shards, so it is the shape and not the scale. Fixing only the part the issue described would have reached about 2x.

Two false starts, recorded because both would have produced a confident wrong number

My first harness had the fake store return pre-built objects, so the download allocation fell outside the measurement window and peak read 2.04x. The obvious correction — return bytes(blob) — allocates nothing, because bytes(b) is b when b is already bytes; and so does b"" + b. It took a bytearray to get a genuine copy. The 3.06x only appeared once the store actually allocated, which is what a real download does.

The fix is one loop

frames_for_target takes fetch_shard_bytes(name) instead of a filled dict, drops each shard's bytes the moment they are decoded, and writes each shard into a manifest-sized buffer instead of stacking and concatenating. Peak is now the finished frame plus about one shard — the residual 0.13x is 2/n_shards, which is why 12 shards measures 1.36x and 36 measures 1.13x.

The buffer's slots are sized from expected_cell_count, a declaration this function already enforced per shard, and the enforcement runs before anything is written — so a shard whose row count disagrees is refused rather than straddling two months' slots.

Nothing about the product changed

The wire is byte-frozen (ADR-013) and tests/test_wire_fixture.py compares delivered artifacts against checked-in bytes. It passes unchanged, which is the whole claim: an assembly change with no output change.

At production scale

64,742 cells x 36 months, at ADR-013 §5's "~1000 samples per cell" — the one input taken from the contract rather than measured — one target's frame is 8.68 GB, so peak falls from about 26.6 GB to 9.8 GB per target, roughly 16.8 GB saved. Run-0's OOM kill recorded anon-rss:23778224kB; that incident's root cause was pandas on the historical leg and is not this, but the magnitude says this leg alone would have exhausted the same box.

The manager's historical frame is not the elephant

#269 also notes _historical_frame is held from _read through _save. By its declared dimensions — 64,742 cells x 438 months = 28,356,996 rows — that is about 108 MB at one float32 column, 1.2% of a single forecast target frame. Recorded in C-101 rather than filed as its own issue: a separate issue implying comparable cost would misdirect whoever picked it up.

Guarded

test_shards_are_fetched_one_at_a_time_not_all_up_front asserts the fetch/decode interleaving, not a byte count — a memory threshold in a test is a flake on a busy machine, while "fetch, decode, fetch, decode" is exactly the property that bounds the peak. Mutation-proven: restoring the up-front dict yields ['fetch','fetch','fetch','decode','decode','decode'] and it fails.

The register's own reference guard also caught me inventing a cross-reference to a register entry C-126 that does not exist — the 126 is an issue number.

Verification

ruff clean; 450 passed, 1 skipped, 39 xfailed.

Closes #269.

Polichinel and others added 2 commits August 14, 2026 23:23
Measured before changing anything, and the issue's diagnosis was a third of the story.

WHAT THE MEASUREMENT SHOWED. A synthetic run through the real TargetLease.load —
producer-format .tap.zip shards, real read_shard, tracemalloc and peak RSS agreeing
within 2% — at 36 shards x 20,000 cells x 200 samples:

  peak, tracemalloc   3.06x the delivered frame  ->  1.13x
  peak, RSS delta     3.02x                      ->  1.06x

The 3x was three roughly equal thirds, and #269 named only the first: every shard's
bytes resident together; every decoded per-shard frame held for the stack; and the
np.concatenate result allocated while those were still alive. The ratio held at both 12
and 36 shards, so it is the shape and not the scale. Fixing only the part the issue
described would have reached about 2x.

Two false starts worth recording, because both would have produced a confident wrong
number. My first harness had the fake store return pre-built objects, so the download
allocation fell outside the measurement window and the peak read 2.04x. The obvious fix
— return bytes(blob) — allocates nothing, because bytes(b) is b when b is already bytes;
so does b"" + b. It took a bytearray to get a genuine copy. The 3.06x only appeared once
the store actually allocated, which is what a real download does.

THE FIX IS ONE LOOP. frames_for_target takes fetch_shard_bytes(name) instead of a filled
dict, drops each shard's bytes the moment they are decoded, and writes each shard into a
manifest-sized buffer instead of stacking and concatenating. Peak is now the finished
frame plus about one shard; the residual 0.13x is 2/n_shards, which is why 12 shards
measures 1.36x and 36 measures 1.13x.

The buffer's slots are sized from expected_cell_count, which this function already
enforced per shard — and the enforcement runs BEFORE anything is written, so a shard
whose row count disagrees is refused rather than straddling two months' slots.

NOTHING ABOUT THE PRODUCT CHANGED. The wire is byte-frozen (ADR-013) and
tests/test_wire_fixture.py compares delivered artifacts against checked-in bytes. It
passes unchanged, which is the whole claim: an assembly change with no output change.

AT PRODUCTION SCALE, 64,742 cells x 36 months at ADR-013 §5's "~1000 samples per cell"
(the one input taken from the contract rather than measured), one target's frame is
8.68 GB, so peak falls from about 26.6 GB to 9.8 GB per target.

THE MANAGER'S HISTORICAL FRAME IS NOT THE ELEPHANT. #269 also notes _historical_frame is
held from _read through _save. By its declared dimensions — 64,742 cells x 438 months =
28,356,996 rows — that is about 108 MB at one float32 column, 1.2% of a single forecast
target frame. Recorded in C-101 rather than filed as its own issue: a separate issue
implying comparable cost would misdirect whoever picked it up.

GUARDED by asserting the fetch/decode interleaving rather than a byte count — a memory
threshold in a test is a flake on a busy machine, while "fetch, decode, fetch, decode"
is exactly the property that bounds the peak. Mutation-proven: restoring the up-front
dict yields ['fetch','fetch','fetch','decode','decode','decode'] and it fails.

The register guard also caught me inventing a cross-reference to a register entry C-126
that does not exist; the 126 is an issue number.

Suite 450 passed / 1 skipped / 39 xfailed, ruff clean.

Closes #269.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the assembly change: sizing the buffer from the first shard MOVED a
constraint. A run whose shards disagree on draws per cell used to fail inside
np.concatenate; it now fails inside the assignment. Both are bare numpy ValueErrors —

  new: could not broadcast input array from shape (6,2) into shape (6,4)
  old: all the input array dimensions except for the concatenation axis must match
       exactly, but along dimension 1, the array at index 0 has size 4 ...

neither names the shard, neither says "draws", and the second is only wordier. That is
the C-99 shape exactly, caught before it could bite rather than after, and the constraint
is now mine rather than incidental, so the refusal should be too.

Mutation-proven: deleting the check brings the bare numpy error straight back.

Suite 451 passed / 1 skipped / 39 xfailed, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… false claims

Five reviewers on the assembly change. The memory result held — one reproduced it
independently at 3.084x -> 1.158x against my 3.06x -> 1.13x, and separately confirmed
that `del shard_bytes` really does free the bytes (np.load on a BytesIO copies;
OWNDATA=True, base=None), that ordering is equivalent, and that array flags match
np.concatenate exactly. What did not hold was the evidence I claimed for it.

NOTHING IN THE SUITE ASSEMBLED MORE THAN ONE SHARD. I wrote that
tests/test_wire_fixture.py passing unchanged proved the product had not changed. That
file does not reference the assembly at all — zero mentions of frames_for_target,
TargetLease or track_a_source. The real end-to-end proof is test_hop_b_sink_e2e.py, and
its fixture has ONE shard, so `position * expected_cell_count` never ran above zero. The
interleaving guard could not have covered it either: its three shards are byte-identical
copies, so any ordering bug survives it.

So the core of the rewrite shipped unverified. There is now a test that assembles three
shards with distinct values, months and units and asserts equality with np.concatenate in
manifest order. Mutation-proven three ways: reversing the slot index, an off-by-one in
stop, and leaving the identifiers unwritten each fail it.

A STORE FAULT WAS BEING BLAMED ON THE MANIFEST. frames_for_target reads a KeyError from
the fetch callback as "this shard was never pinned", and my lambda put
`self.store.download(...)` inside the same try. A KeyError from anywhere in the client —
a response indexed with [] rather than .get(), which is exactly how C-99 happened one
layer down — would have surfaced as "manifest lists shard X but its bytes were not
provided", with `from None` discarding the traceback that said otherwise. Verified by
mutation: the old lambda produces that false diagnosis verbatim. The lookup and the
download are now separate, and a store KeyError raises a named SourceSelectionError
chained to its cause.

expected_cell_count SIZES AN ARRAY NOW. It used to sit on one side of a `!=`, where 6.0
compares equal to 6 and passed. It now reaches np.empty and raises a bare "TypeError:
'float' object cannot be interpreted as an integer", naming neither the field nor the
manifest — from a document that crossed a repository boundary and whose read side only
checks that the key is present. Guarded, with bool and non-positive covered.

FOUR FALSE CLAIMS IN MY OWN PROSE.
- "the residual is 2/n_shards": fits none of its own numbers. 2/12 is 0.17 against a
  measured 0.36. Re-measured at 12, 24 and 36 shards: the overhead is CONSTANT at about
  4.5 shard-widths (~70 MB), so the ratio falls as 1/n — 1.36x, 1.19x, 1.13x.
- "ADR-013 §5's ~1000 samples per cell": that line is under §0. §5 is the GAUL sidecar.
- the test_wire_fixture citation, above.
- a docstring quoting the numpy error as "into shape (12,4)" when the failing assignment
  touches a (6,4) slice.

THE HISTORICAL FRAME IS FILED, NOT GLOSSED. #269 lists "the historical frame is released,
or not held" as an acceptance criterion; this change does not meet it. It is now #273,
carrying the measurement (108 MB, ~1.2% of a target frame) so nobody picks it up thinking
it is comparable. Closing #269 with that quietly unmet was the alternative.

Suite 457 passed / 1 skipped / 39 xfailed, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Polichinel
Polichinel merged commit 70f25f0 into development Aug 14, 2026
4 checks passed
@Polichinel
Polichinel deleted the fix/269-bound-the-shard-fetch branch August 14, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant