Reuse OperatorOutput in pine-cpp (#122); storage_mode docs + column guardrail (#160) - #182
Merged
Merged
Conversation
… buffer Ports pine-go's #119 pooling win to pine-cpp (issue #122). Every node previously constructed a fresh OperatorOutput, so an operator writing N fields re-grew its write log 0->1->2->4->...->N on every single Execute. The flat-vector layout was already in place; only the allocation was missing reuse. pine-go uses a sync.Pool because its scheduler can hand a node to any goroutine. The C++ ready-queue never migrates a half-finished node, so a plain thread_local buffer gets the same capacity reuse without the pool's Get/Put bookkeeping. reset() clears in place rather than reassigning: vector::clear keeps the backing array (the entire point), and object_t / set clear keeps their node pools warm. Reset happens on ACQUIRE, not release, because apply_output move-extracts added_items_ / column_writes_ and leaves same-size husk vectors behind; releasing without reset would replay those husks as ghost items on the next node. Acquiring-side reset also covers the throwing-node case with no unwind handling.
…y loss Two regression gates for issue #122, both mutation-verified: - test_output_pool.cpp mirrors pine-go's scheduler_test pooling tests: an inspect operator records what its output buffer already contained on entry, run repeatedly and across a recall->mark->inspect chain. Deleting reset() on acquire turns the cross-operator case red (the recall's moved-from added_items husks leak into the downstream Transform, which then trips the "Transform must not call AddItem" type check). - test_operator_output.cpp asserts capacity survives reset(). Replacing the clear() calls with a genuine move-assign turns this red (0 == 256). Note that `item_writes_ = {}` would NOT: it binds to operator=(initializer_list) and forwards to assign(), which never shrinks the buffer.
The nightly cross-runtime benchmark has been uploading nothing for months while reporting success. Reports moved to bench-results/report-<ts>.txt in aabf337, but the workflow still read /tmp/bench_cross_runtime/report.txt in all five places: compare, analyze, step summary, artifact upload, and the Bark notification body. Evidence from run 30107050346: ==> Done. Report: .../bench-results/report-20260724-155142.txt ! No files were found with the provided path: /tmp/bench_cross_runtime/report.txt No artifacts will be uploaded. The last two runs each produced 0 artifacts. Nothing failed loudly because upload-artifact only warns and every consumer was guarded by `if [ -f ... ]`, so the whole trend-tracking chain degraded to a green no-op. Rather than teach the workflow to glob a timestamp, the script now also copies the finished report to the fixed name bench-results/report.txt. The timestamped file stays as the local archive; the stable name is what CI and `gh run download` consume, so the two cannot drift apart again. A copy and not a symlink: artifact upload follows the link but archives the target's name, which would land an unpredictable filename on the consumer. Also drops the WORK_DIR/*.csv entry from the artifact list — those are hey's intermediate files, deleted by the cleanup trap before upload runs.
`--modes "row,column"` produced two report rows labelled row and column that were both the mode declared in the fixture config. $mode reached only the progress line and the report's Storage column; start_server always got the untouched config. Since every fixture that declares the field declares "row", any column figure ever read out of a --modes report was row-mode numbers wearing a column label. storage_mode is a root-level config field and no runtime accepts it as a server flag, so honouring the override means rewriting the config rather than passing an argument: config_for_mode writes a copy into WORK_DIR with the field pinned and hands the server that path. Runtime-agnostic, and the same approach benchStorageAB already uses in-process (pine-go/benchmarks/bench_storage_ab_test.go sets cfg["storage_mode"]). When the fixture already declares the requested mode the original path is returned unchanged, so the common no-override run touches nothing. If the rewrite fails the function emits an empty path and the caller skips that run with a message — falling back to the original config would silently reintroduce the mislabelling. Verified on small_010 with --modes "row,column": the two legs now report different throughput, and the materialized config carries "storage_mode": "column".
parse_report only matched 11-column lines, the layout retired when the benchmark became fixture-driven. Against today's 9-column reports it built an empty dict, so every nightly comparison printed "No comparable data found between runs" — a message indistinguishable from a first run, which is why it went unnoticed while the path bug kept artifacts empty anyway. Now accepts both layouts and normalizes them to (runtime, fixture, storage), folding the legacy (nodes, par, op) triple into a synthetic fixture name the way bench-analyze.py already does. The two parsers must stay in sync: a format the analyzer accepts but the comparer drops looks exactly like "no regressions." Report table widened to fit real fixture names, which are far longer than the legacy op labels. Also drops the `re` / `sys` imports, unused since before this change and flagged once the file was staged.
…fixtures Drops the unused `os` import and gets every line under the configured 100-column limit. The five-field item_output list appeared verbatim eight times and accounted for most of the E501s, so it becomes the ITEM_FIELDS constant; the rest are docstring and pipeline-list rewraps. These predate this branch — the pre-commit hook lints the whole staged file, so they surfaced as soon as the file was touched for another reason. Generated fixtures are byte-identical before and after.
Every calibrated fixture declares storage_mode=row — correct for their N~10
production shape, but it leaves the column batch-access path with no fixture
watching it. A regression there (say batch column access silently degrading
to per-element gather) would not show up in anything the nightly matrix runs.
transform_heavy_1000 is that guardian: one recall_static plus a chain of
eight transform_normalize ops, each reading the previous one's output field,
with no removals, reorder or additions after the recall. Shape mirrors
transformHeavyConfig in pine-go/benchmarks/bench_storage_ab_test.go — a
strictly serial DAG that rescans the same column at every step, which is
where column storage is expected to win. Pinned to storage_mode=column;
`--modes "row,column"` now gives a real A/B on the identical shape.
The root-level _comment states plainly that this is synthetic and not a
production proxy, that calibrated fixtures remain the sole referee per
llmdoc/guides/benchmark-hygiene.md, and that +-5-7% build-layout noise
bounds it to catching wholesale collapse rather than fine-grained
regressions. Root level specifically: the operator layer rejects unknown
parameters, the root layer ignores them.
flow_contract projects the chain-tail field rather than being left empty.
An empty item_output projects every item to {} (an empty output list is an
empty output, not a fallback to all fields), which would erase the
serialization cost and turn the whole chain into writes nobody reads.
Fixture discovery is a plain glob, so nothing else needs wiring. Verified:
go, java and cpp all emit byte-identical output on it, and row vs column
agree byte-for-byte.
Selection criteria only existed in llmdoc, so downstream users had no way to discover when column mode pays off — README said "DataFrame supports two storage modes" and stopped there, and doc/ documented no root-level config at all. Adds a "Flow-level configuration" section covering storage_mode, log_prefix, debug and skip_dead_code, with the selection criteria written out: transform-dominated + large N + few structural changes favours column, recall/filter/sort or small N stays on row, plus the mechanical reason. Deliberately states no speedup multiples. The figures floating around (~30% in llmdoc vs ~37% in the PR #155 description) are two measurements of the same experiment that disagree, they come from Go microbenchmarks on Apple silicon while README's Benchmark table is Linux 2C/4G HTTP end-to-end, and #156/#157 have already moved them three times. Points at the A/B entry points instead, so readers measure their own workload. The criteria are stable; the multiples are not. Both documented commands were run before committing: the benchmarks/ module needs its own directory and the pine_bench tag, which the first draft got wrong. The Flow snippet and the compile-time rejection of invalid values were checked too. Says nothing about what happens on an invalid value, because the runtimes disagree — pine-go and pine-java fall back to row, pine-cpp falls back to column. The Apple DSL rejects it at compile time, which is the contract worth documenting; the divergence is filed separately.
…laim
Reflection: memory/reflections/cpp-output-pool-and-storage-mode-guardrail.md
Promoted to stable docs:
- ci-quality-baseline: the two-step mutation criterion. "Test stayed green"
should first be read as "the mutation was inert," not "the test has no
teeth" — reversing that order nearly got a correct assertion rewritten.
Carries the concrete counterexample: `vec = {}` binds
operator=(initializer_list) and forwards to assign(), which never shrinks
capacity, so it is semantically identical to clear().
- standard-workflow: never restore a mutation with git, only with a file-level
cp. `git checkout -- <file>` swallowed uncommitted work here, and this is
the second incident of the shape — the earlier fix (use `git checkout
<base> -- <file>`) only addressed which baseline you get, not what else gets
reverted. Also: `git add <file>` is the hole in single-domain commit
discipline when one file carries two unrelated changes, and touching a
long-untouched file means inheriting its whole lint debt, since pre-commit
gates per staged file.
- benchmark-hygiene: how a synthetic guardrail divides from a calibrated
fixture, and that porting an in-process microbench shape to an e2e fixture
requires re-checking the projection stage — an empty item_output erases the
serialization cost the e2e run exists to measure.
- pine-cpp-runtime: the #122 thread_local reuse, with reset-on-acquire
documented as a contract rather than an implementation detail.
- memory/decisions: user-facing docs quote no performance multiples, with the
reasoning (two disagreeing figures for the same experiment, incomparable
measurement paths, three rounds of churn). Cross-referenced from
conventions' existing "no hardcoded quantitative claims" rule.
Corrects a stale claim rather than only adding: ci-quality-baseline listed
clang-format as the C++ lint tool immediately beside the CI cpp-lint job
description, which reads as coverage that does not exist. Verified against
.github/workflows/ci.yml — cpp-lint runs a -Werror strict build, whitespace
and trailing-newline hygiene, and the adjacent-literal typo guard. Nothing
greps clang-format or fmt-check anywhere under .github/workflows/.
New memory/doc-gaps.md tracks that gap (whether CI should gain an fmt-check
job is an open engineering question, not just a doc fix), plus issue #179,
recorded as a known divergence that is not fixed.
…bench skips Independent review of 7612e18..30a7fc6 found five issues; all five fixed here. Important 1 — pine-cpp/tests/test_output_pool.cpp: the cross-request leak test had no detection power at all. The buffer is thread_local, so a leak from run i is only observable if run i+1 lands on the same worker; this pipeline submits one task per request and the default pool is nproc*4, so 32 requests spread across 32 distinct threads and every run saw a pristine buffer. Confirmed by mutation: with the acquire-side reset() removed the test passed 20/20. Pinning dag_pool_size to 1 makes same-thread reuse certain rather than probabilistic — the same mutant now fails 20/20, and the real code passes 20/20. The sibling cross-operator test was already catching this defect 5/5, so the contract was guarded; this test simply was not the guard its comment claimed. Important 2 — scripts/bench-cross-runtime.sh: config_for_mode folded "probe failed" into "declares row" via `|| echo "row"`, so an unreadable config whose target mode happened to be row was handed to the server as a success. Probe failure now returns a skip like rewrite failure does. Separately, skips silently shrank the sample count while TOTAL_RUNS stayed at the precomputed figure, so missing report rows produced no signal — the same shape as the empty-artifact bug this branch set out to fix. The run now counts completed rows and, on any shortfall, writes an explicit WARNING plus the skip reasons to both the report tail and stderr; the report tail matters because the Bark notification only forwards the analysis Verdict section. Minor 3 — engine.cpp: the comment argued that nothing leaks across engines because the buffer dies with its thread. That does not follow for a function-level thread_local. Rewritten to give the actual reason: dag_pool_ is per-Engine, so a worker only ever runs node bodies for its own engine. Also notes that dag_pool_ is unconditionally constructed, so the null-pool synchronous fallback is reachable only when run_dag is called directly. Minor 4 — test_operator_output.cpp: "the leak tests above" pointed at value semantics tests in the same file; the leak tests are in test_output_pool.cpp, as the file header already said. Minor 5 — bench-compare.py and bench-analyze.py: the `parts[0] == "═══"` header guard never matched, since the banner rule is one 67-character run of box characters. Harmless (the line has neither 9 nor 11 fields and fell through) but it read like a working guard. Now startswith, fixed in both since their parsers are required to stay in sync. Verified: 242 doctests pass, cross-validate 55/55 sections, both report parsers still handle the 9-column, 11-column and N/A rows, and the shortfall warning was exercised end-to-end with a deliberately corrupted fixture.
The round-1 review supplement noted that the WARNING line added by
bench-cross-runtime.sh splits into exactly nine whitespace fields — the same
width as a data row. It is currently discarded only because the fourth field
("of") fails float(), so a harmless rewording could turn the notice into a
phantom data row. The reviewer verified present behavior is correct and
explicitly did not class this as a finding; fixing it costs two lines and
removes the dependency on wording.
Both parsers now skip WARNING:/skipped: prefixes explicitly, alongside the
existing header and banner guards. Verified that a report containing one data
row plus both warning lines yields exactly one parsed row in each parser.
…enchmark runs Final full-range review of 7612e18..ba4d7ad found five issues; all fixed. Important 1 — unbounded capacity retention was not on par with pine-go. A thread_local buffer lives as long as its thread, and DAG pool workers live as long as the engine (nproc*4 of them by default), so whatever capacity one large request needed was pinned on every worker that served it, with no reclamation point. The reviewer measured 478 MB RSS after 300 large requests versus 27 MB for the per-call baseline, with nothing returned after 2000 subsequent small requests. Go does not behave this way: its buffers sit in a sync.Pool that the GC empties. Confirmed independently with a temporary probe in internal/runtime — an output with cap 554598 comes back with cap 0 after two GC cycles. reset() now swap-releases any vector grown past kRetainLimit (65536 elements) instead of clearing it. That keeps the full reuse win for every shape this engine targets (calibrated production N~10, largest synthetic fixture N=5000) while capping the damage from an outlier request. Both directions are pinned by tests: the existing case asserts ordinary sizes retain capacity, a new one asserts oversized buffers release it, and the new assertion goes red when the release branch is mutated away. Important 2 — an all-N/A row was counted as a completed run, so the shortfall warning added earlier stayed silent in the most likely failure mode: a server that answers /health then dies under load. hey exits 0 when it cannot connect, so neither set -e nor pipefail catches it, and both report parsers discard the row — exactly the thinning-data-behind-a-green-job shape the warning exists to catch. N/A rows now count as skips. Minor 1 — hey exits 1 on argument errors (e.g. -c greater than -n), which under set -e aborted the script before the shortfall warning and the report.txt copy. Added `|| true`; parse_hey's N/A fallback now handles it. Minor 2 — the reset() comment described Variant::object_t as node-pool-backed. It is FlatMap, a sorted std::vector, so it retains contiguous array capacity; std::set is the only node-based container in the struct. Minor 3 — llmdoc/index.md's new memory/reflections/ heading left memory/ holding a single entry, reading as though the directory had one file. The section now states it lists top-level files and points at both subdirectories. llmdoc records the retention cost and the Go divergence, which previously documented capacity retention only as a benefit. Verified: 243 doctests pass, cross-validate 55/55, mutation red-before / green-after on the new release assertion, and the N/A skip path exercised.
Both defects here are in the previous commit's fix, found by the reviewer while verifying it. The regression gate covered one container out of three. The test drove set_item only, so the added_items_ and column_writes_ release branches were unguarded: deleting them left the suite fully green at 243/243 while a recall-heavy load still pinned ~478 MB permanently — the exact regression the fix was for, since recall operators grow added_items_, not item_writes_. The test now uses SUBCASEs driving set_item, add_item and set_item_column_double past the limit, and mutating any one of the three branches turns it red (verified individually). reset() delegates to a clear_or_release helper so the decision lives in one place rather than three copies that can drift apart. The stated cost was wrong by an order of magnitude. "Roughly a megabyte per worker" is really 10.00 MiB when one request drives all three containers to the limit — item_writes_ 5.00, column_writes_ 3.50, added_items_ 1.50, from sizeof 80/56/24 at 65536 elements — so about 960 MiB across the default 96 workers, not ~96 MiB. That figure is what a reader uses to judge whether the threshold is safe, so both the comment and llmdoc now carry the measured numbers and note it is a synthetic worst case. The threshold itself stays at 65536. The reviewer independently judged the position defensible: production calibrated N≈10 and the largest synthetic fixture N=5000 sit more than 13x below it, verified retained at N=5000, and it is far from vector's growth-doubling points so no request randomly triggers a release.
Independent review found the capacity ceiling still missed one container, and the miss is the same shape as the previous two rounds — hence routing all four through one helper rather than fixing this instance alone. item_order_ is a capacity-bearing vector<int> that reorder_sort and reorder_shuffle_by_salt both fill to the item count, yet reset() cleared it without any ceiling. Measured: 200000 elements (781 KiB) survive reset() untouched. Retaining it was never even a win — set_item_order move-assigns, so the next call discards whatever block reset() kept (verified: capacity goes 200000 -> 3). It was pure cost, and before this it was unbounded cost. reset() now routes item_order_ through clear_or_release with the other three, and the test grows a fourth SUBCASE driving set_item_order past the limit. Mutating any one of the four release branches now turns the test red, each checked individually. std::set removed_items_ stays out deliberately: clearing it returns nodes to the allocator's free list rather than holding one contiguous block, so there is no single capacity to cap — now stated in the helper's comment, which previously overclaimed by saying "every capacity-bearing vector". Also from the same review: - config_for_mode's contract is now stated explicitly, so the `return 0` on error paths reads as "give up without printing a path" rather than "succeeded" — the exit status is deliberately 0 so a failure cannot trip the caller's set -e; emptiness is the signal. - llmdoc wording: dropped 形态 / 落地 / 守门 / 守住 from text added on this branch, which CLAUDE.md lists among the constructions to avoid. Verified: 243 doctests / 110574 assertions pass, lint clean, cross-validate 55/55.
…f vectors The round-3 reviewer was asked directly whether the omission class was closed and answered no, with a fifth instance: warning_. Measured 4 MiB surviving reset() on a real thread_local buffer. Warning text embeds operator messages carrying request data, so its high-water mark is request-driven exactly like the vectors'. Its root cause is worth more than the fix. clear_or_release accepted only std::vector<T>&, so std::string was excluded from the discussion by type — no amount of care reading the vector list would have surfaced it. The criterion itself was wrong: "which containers have a branch" is a list somebody has to maintain, and every new member is another chance to forget. "Which members hold something that grows with the request" is a question you answer against the member list. So the helper is now overloaded on the container's own capacity()/clear(), and reset()'s comment walks all nine data members and states where each one lands: five routed and tested, common_writes_ routed but deliberately untested, removed_items_ excluded with a reason, two bools holding nothing. common_writes_ is the honest exception. FlatMap exposes no capacity(), so its release cannot be observed through the API and any assertion would pass against a plain clear() too — verified: mutating that branch leaves the suite green, which is why there is no SUBCASE rather than a decorative one. It is also the member least at risk, since its size follows the pipeline config rather than item count. Both facts are recorded at the call site. Mutating any of the other five release branches turns the test red, checked one at a time. 243 doctests / 110579 assertions pass, lint clean, cross-validate 55/55.
BLOCKING, from the fourth full-range review: the capacity ceiling bounds each container's spine and nothing else. capacity() counts slots — but an ItemWrite owns a std::string and a Variant, every added_items_ row is its own heap block, and a DoubleColumnWrite owns a vector<double>. None of that payload is counted, so none of it was bounded. With reset only on acquire, an idle worker held the whole previous request's payload until it happened to run another node. Measured here at N=2000 (far below the 65536 ceiling, so the release branch never fires), 4 KiB per item, 400 requests: 6.32 GB without the fix versus 5.36 GB with it. The reviewer measured 806 MB vs 34 MB retained at the same item count with malloc_trim. Either way, ordinary traffic reaches the magnitude the ceiling comment called a synthetic worst case — the ceiling was measuring the wrong dimension. Fix is one reset() after apply_output on the success path. The acquire-side reset stays: it is what upholds the moved-from-husk contract when a node throws, and the catch block reads out.warning(), so the new call cannot move above it. No test guards it, stated as such at the call site. The retention window sits between one node's apply_output and the next node's acquire reset, and every in-engine observation point is after an acquire reset, so an operator probing the buffer reads zero either way. I wrote that test, found it passed against a mutant with the line deleted, and removed it rather than leave fake coverage. Confirming this needs external RSS measurement. Also from the same review: - --modes now rejects anything but row/column. The runtimes silently fall back on an unknown value and disagree on the direction (#179), so a typo would have produced a whole report labelled with a mode nothing ran — the same mislabelling this script was just fixed for. - The warning_ comment implied a real 4 MiB observation. No built-in operator can approach the limit: all three set_warning sites are key- or error-sized and one truncates at 1024 bytes. The figure is synthetic; now said so. - common_writes_'s exclusion note now mentions both config-bounded routes into it, including recall_static's set_common, whose keys come from its own params rather than declared metadata. - README-en.md was left behind when README.md's storage_mode line was expanded. - bench-analyze.py usage examples still cited the retired /tmp path. - bench-compare.py's legacy 11-column branch is documented as unreachable within the 30-day artifact window, kept for symmetry and archived reports. 243 doctests pass, lint clean, cross-validate 55/55.
BLOCKING, from the fifth full-range review: the release-side reset() sat inside the try block, so a node that throws skipped it — and a throwing node holds the largest payload of all, because apply_output never consumed it. Worse, reset is what applies kRetainLimit, so the throw path escaped the ceiling entirely: the reviewer measured 352 MB retained at N=70000 and 702 MB at N=140000 on pool=24, linear in N with no bound at all. Resetting only on success left precisely the worst case unprotected. The call now sits after the try/catch, so it runs on both paths. It cannot move to the end of the try instead: the catch block reads out.warning() to attach the message to the frame. That makes the acquire-side reset redundant in the strict sense — mutate it away and the suite stays green, because the trailing reset already left the buffer clean. Kept anyway, as a no-op on empty containers, so the invariant "execute() sees a clean buffer" holds locally rather than depending on every exit path in the body staying correct. The comment now says "defence in depth" instead of claiming it is load-bearing. Also corrected two comments that had become false: - test_output_pool.cpp claimed deleting the acquire-side reset fails the cross-request case 20/20. That was true when written, and the release-side reset added later in this same branch invalidated it. Now recorded as a historical note with the reason it no longer bites — the reviewer caught this by mutating the line and finding 243/243 green. - The engine comment claimed the release reset was success-path-only by design. New test: a recall that adds five items then throws, followed by a good request on the same engine and a single-worker pool, asserting the failed request's items never appear in the later response. With both resets removed it fails with 8 items instead of 3, five named "leak*". Two engines would not have worked — each gets its own dag_pool and so its own worker threads, with no shared buffer. Behaviour test, not a gate on one line: either reset satisfies it, and the comment says so. Two tests written for the payload dimension were removed rather than kept, because each passed against a mutant with the fixed line deleted. The retention window lies between one node's reset and the next node's acquire, and every in-engine observation point is after an acquire reset, so an operator probing the buffer reads zero either way. Recorded at the call site, along with the finding that RSS cannot measure this (both variants read within 0.1%; the allocator caches the freed blocks) and an allocator live-bytes probe is needed. 244 doctests pass, lint clean, cross-validate 55/55.
… on wording
Remaining findings from the fifth full-range review.
The shortfall warning is written into the report body, and both parsers were
excluding it by matching "WARNING:"/"skipped:" as the first field. That defends
on wording: the notice splits into exactly nine whitespace fields, the same
width as a data row, and survived only because field 4 happens not to parse as
a float. Reword it and it becomes a phantom data row with runtime "WARNING:",
feeding a bogus verdict while the job stays green — the third instance of that
shape in this file's history. The notice now carries a '#' prefix and both
parsers skip comments structurally. Verified against an adversarial line
("# WARN 4 6 0 0 0 0 0 0", numeric field 4): still one data row.
The reset() cost table listed three containers while the rule right above it
insists on answering for all nine members. Added the two it omitted
(item_order_ 0.25 MiB, warning_ 0.06 MiB) for 10.31 MiB per worker and ~990
MiB across 96, and stated plainly that this is the spine budget only — element
payload is not in it, real footprint runs well above, and 990 MiB must not be
used for capacity planning. That was the more misleading half.
Dropped bench-compare.py's 11-column branch. Nothing has emitted that layout
since 2026-05-28, artifacts live 30 days, and this script only ever compares
two runs, so no input could reach it; it was untested and would have rotted
quietly. bench-analyze.py keeps its 11-column support because it does get run
by hand on archived reports — the asymmetry is now stated rather than implied.
Also: one-off measurements now carry their conditions (N, payload size, pool
size, and whether the figure is RSS or allocator live bytes), since the numbers
are observations rather than gates and absolute values move with the machine.
The FlatMap size proxy is narrowed to the one-way implication it actually is —
reserve() followed by a single insert slips through, acceptable only because
nothing reserves that map. And the reflection's hardcoded test count is replaced
with a pointer to `make cpp-test`, having gone stale within this very branch,
which is exactly what conventions.md warns about.
244 doctests pass, lint clean, cross-validate 55/55.
Sixth full-range review: 0 blocking, and it answered the two questions worth asking. The reuse does pay for itself — paired interleaved runs on pinned cores show +2.02% on large_1000 (10/10 pairs) and +1.76% on transform_heavy_1000 (9/10), with allocation bytes down 4.6% at small_100 rising to 34.3% at large_5000. Memory is bounded on all five paths, the throw path being the one that matters most: 1.2 MB versus 205.5 MB without the trailing reset. Every finding was a comment claiming more than is true, and three of them were mine to begin with — I moved the reset out of the try block and left the justifications describing the old arrangement: - Two comments still said the release-side reset is inside the try / covers only the success path. It sits after the catch and does cover throws, which the reviewer measured. Those sentences were the stated reason for keeping the acquire-side call, so the argument rested on an inverted model. - The engine comment credited the failed-request test with guarding the acquire-side reset. It does not: delete that line and the case still passes 8/8, all pool cases pass, and the suite passes 244/244. Nothing has mutation coverage on it, which the same comment block admitted two paragraphs later. - "Every idle worker holds the full payload" is false for added_items_ on the success path, since apply_output move-extracts it (measured: no difference either way). True for the value-copied containers, and true for everything on the throw path. The byte budget also omitted common_writes_, which is routed and capped like the rest: 4.50 MiB more per worker, so 14.81 MiB and ~1422 MiB across 96 workers rather than 10.31 and 990. Arithmetic for the members it did list was right; the omission was the problem. Minor: "largest synthetic fixture N=5000" now says "benchmark fixture", since unit tests and probes deliberately drive far larger N and a reader could take 5000 as the largest tested anywhere. bench-analyze.py's docstring documents the '#' comment-skip rule. And the generated fixture's _comment drops its two hardcoded figures in favour of pointing at benchmark-hygiene.md — a generated file is exactly where numbers go stale, since nobody edits it when the noise threshold moves. Verified: make cpp-test 244 cases green (the new failed-request case run 40x for flakiness, 40/40), cross-validate 55/55, differential-fuzz 1000/1000, codegen fresh, lint clean, and the regenerated fixture still byte-identical across go/java/cpp.
Seventh full-range review, 0 blocking again. Both important findings were claims that were true in one mode and asserted unconditionally. The engine comment said added_items_ is move-extracted by apply_output "so it is already empty (measured: no difference either way)". That is RowFrame (row_frame.cpp:269, `auto&`). ColumnFrame value-copies it (column_frame.cpp:418, `const auto&`) and leaves the original intact, with column_writes_ going the other way about. Measured at N=2000 with 4 KiB items, added_items_ right after apply_output: 0 KiB on row, 8003 KiB on column. Not a hypothetical difference either — the transform_heavy_1000 fixture added in this range pins storage_mode=column, so the copying side is the one that ships. The code was already correct because the trailing reset is unconditional; only the reasoning was mode-specific. llmdoc still carried three statements the previous commit corrected in engine.cpp but not there: that reset must be on the acquire side, that the trailing reset covers the success path only, and that verification needs RSS. All three describe superseded arrangements — the trailing reset sits outside try/catch and covers both paths, deleting the acquire-side call leaves 244 cases green so "must" is wrong, and RSS was specifically found insufficient (the two variants differ by under 0.1% because the allocator caches freed blocks). The responsibilities of the two resets were effectively swapped. Rewritten to match, including which container each storage mode leaves behind. New test: the recall/mark/inspect reuse case now has a column-storage twin, since the two modes leave the buffer in different states and only row was covered. Goes red with both resets removed. Minor: bench-analyze.py's docstring now states the shortfall notice splits into 9 fields only once the '#' is stripped, which is the premise that makes the structural guard necessary; bench-compare.py's docstring said the same thing about the dropped 11-column branch twice. Verified: make cpp-test 245 cases green via the ctest target, lint clean, cross-validate 55/55, differential-fuzz 1000/1000.
Eighth full-range review: 0 blocking, 0 important, 1 minor — the code itself drew no findings. The minor was mine. The previous commit corrected the reset description in architecture/pine-cpp-runtime.md but left index.md's navigation summaries saying "reset 必须在 acquire 侧", so the same llmdoc contradicted itself on the mechanism a reader meets first. Both index entries now describe the arrangement that exists: resets on both sides, the load-bearing one at the end of the node body outside try/catch, the acquire-side one kept as defence in depth. Found a third instance the review did not flag, in the reflection itself. Left as a process record rather than rewritten to look prescient — it now states that "must be on acquire" was the conclusion at the time and that later rounds overturned it, and points at the architecture doc as authoritative. A reflection recording a superseded belief is fine; one silently asserting it as current is not. Verified: make cpp-test 245 cases green, lint clean. The review independently re-ran cross-validate 55/55, differential-fuzz 200 rounds across go/java/cpp with the C++ binary injected, a -Werror strict build in its own tree, and confirmed the six sizeof values and the 14.81 MiB / 1422 MiB budget by recomputation.
Two items from the round-4 and round-6 supplements. The round-4 reviewer attached a condition to its ship recommendation: the "6.32 GB versus 5.36 GB" figure is un-malloc_trim'd absolute RSS with frame and result memory in the denominator, so quoting it as −15% badly understates the fix and would mislead anyone later judging whether to keep the optimization. The llmdoc entry now carries the caliber alongside the allocator-live-bytes numbers that isolate the buffer: 806 MB → 34 MB on the success path (~23x) and 205.5 MB → 1.2 MB on the throw path (~170x), plus the warning that RSS is the wrong instrument here since the two variants can differ by under 0.1%. Its other condition was already met — the spine/payload distinction sits at the kRetainLimit declaration itself. The round-6 supplement noted residual 守门/守住 in lines this branch added. CLAUDE.md names both, so they are now 质量检查 and 防住. Left the one in an untouched pre-existing line alone rather than widening scope.
Ninth full-range review: APPROVE, 0 blocking, 0 important, 1 minor — this. test_output_pool.cpp still said the acquire-side reset is guarded by the failed-request case "where a throwing node skips the release". Both halves are wrong now. The release-side reset sits outside the try/catch — 003fbf9 moved it there precisely so throws would not skip it — so nothing in this file goes red when the acquire-side line alone is deleted. Verified: 245/245 green, and the failed-request case passes 20/20 on its own. The mutation is genuinely semantic — delete both resets and that case fails with 8 items, five named leak*. engine.cpp already states the honest position at the call site ("defence in depth", not "guarded by a test"); this comment was the last one carrying the superseded model. The reviewer also corrected my framing: I had described the final commits as documentation-only, but 131b73e added a test case and 00f5ddc changed the generated fixture's _comment. Only the last three commits are purely llmdoc. Everything else came back clean, with the disclaimers checked rather than taken on trust: the six release branches and the capacity-retention direction each go red under individual mutation, the three lines documented as unguarded each stay green when deleted, the six sizeof values match measurement, the 14.81 MiB and 1422 MiB arithmetic derives correctly, and the four source line references in the comments are accurate.
Tenth full-range review: APPROVE, 0 blocking, 0 important, 1 minor — this. The reflection cited pine-cpp/include/pine/pine.hpp:493 for OperatorOutput::reset(). That was correct when written at 30a7fc6; two later rounds of comment expansion pushed the function to 529. b4ffe0d updated the prose around it and missed the number. Dropped the line number rather than correcting it. A pinned line in a doc that sits outside the file it points at is the same maintenance trap as a hardcoded count, and conventions.md already prefers symbol-granularity references — a correct line number today would just drift again on the next edit. The review otherwise found no reportable defect. Notably it verified the self-descriptions by mutation rather than trusting them: every place claiming a regression gate went red under individual mutation, and every place claiming to be uncovered stayed green when deleted. It also checked something I had not — that reset() is entirely noexcept, which matters because the trailing out.reset() sits outside the try/catch, so a throwing reset would leave `remaining` never reaching zero and hang run_dag. Independent runs: 245 cases green, differential fuzz 300/300 go-vs-cpp, TSan clean on the reuse paths (the one race it found is pre-existing test scaffolding in a file this range never touched), and the 14.81 MiB / 1422 MiB budget recomputed from real sizeof values.
Eleventh full-range review: APPROVE, 0 blocking, 0 important, 2 minor — both comment text, no behaviour change. "~170x by one measurement below" pointed at figures that cannot produce that ratio: the lines below give only one-sided throw-path numbers (352 MB at N=70000, 702 MB at N=140000). The 205.5 MB versus 1.2 MB pair that does yield ~170x lived only in llmdoc, so a reader following the pointer in this file found nothing to divide. The pair is now stated where it is used, and the earlier sentence no longer claims a ratio it cannot support. Also rewrapped a comment line that broke at column 35 while its neighbours run to ~100. .clang-format sets ColumnLimit 110 with ReflowComments, so this was hand-wrapped residue — and, as the reviewer noted, nothing would have caught it, since clang-format has no CI job. Verified the whole block is now inside the limit. The review independently re-derived the substance rather than trusting the comments: deleting either reset alone leaves the suite green while deleting both turns 4 cases red with exactly the documented 8-vs-3 items and five leak* entries; the five capacity-release branches each go red under individual mutation; the six sizeof values, the 14.8125 MiB total and the 1422 MiB 96-worker figure all recompute; and the cited row_frame.cpp:269 / column_frame.cpp:418 references are accurate. It also confirmed the new fixture produces identical md5 across go x cpp x row x column.
Contributor
🔍 PR 审查
未发现需要修改的问题。已核验 验证限制
本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
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
1 participant
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.
Closes #122. Closes #160.
What this does
#122 — pine-cpp reuses
OperatorOutputacrossExecutecalls. Ports thepine-go #119 win:
node_bodyno longer constructs a freshOperatorOutputpernode, using a
thread_localbuffer withreset()instead.thread_localrather than a pool because the ready-queue scheduler never migrates a
half-finished node, so there is nothing to synchronise and no Get/Put
bookkeeping to pay for.
#160 part 1 —
storage_modeselection guidance for users. The criteria onlyexisted in
llmdoc;README.mdsaid "DataFrame supports two storage modes" andstopped, and
doc/documented no root-level config at all. Adds a "Flow-levelconfiguration" section to both language versions of the pipeline guide covering
storage_mode/log_prefix/debug/skip_dead_code, with the criteriawritten out and a measurement entry point.
#160 part 2 — a synthetic column-mode benchmark guardrail. Every calibrated
fixture is
storage_mode: row, correct for their N≈10 production shape butleaving the column batch-access path unwatched.
transform_heavy_1000is onerecall_staticplus eight chainedtransform_normalize, pinned to column, andlabelled in its own
_commentas synthetic and explicitly not a productionproxy — calibrated fixtures remain the sole referee.
Prerequisite fixes it turned out to need
Wiring the guardrail into the nightly matrix meant fixing three pre-existing
defects, all of the same shape (data silently wrong, job green):
bench-results/but the workflow still read/tmp/bench_cross_runtime/infive places.
upload-artifactonly warns, so the job stayed green whiletrend-tracking degraded to a no-op. Run 30107050346 shows 0 artifacts.
--modeswas a no-op switch.$modereached only the report column;storage_modeis a root-level config field and no runtime takes it as a flag,so
--modes "row,column"ran the same config twice and labelled the twoidentical runs differently. Every column figure ever read out of such a report
was row-mode numbers.
bench-compare.pynever matched. It parsed only the 11-column layoutretired when the benchmark became fixture-driven, so it always reported "no
comparable data" — indistinguishable from a first run.
Documentation stance
User-facing docs quote no performance multiples. The figures in circulation
disagree with each other (~30% in llmdoc vs ~37% in the PR #155 description, two
measurements of one experiment), they come from Go microbenchmarks on Apple
silicon while README's Benchmark table is Linux 2C/4G HTTP end-to-end, and
#156/#157 have moved them three times. The docs give the criteria, which are
stable, and point at the A/B entry points so readers measure their own workload.
Recorded as a decision in
llmdoc/memory/decisions/.Verification
At
36380fd4:make cpp-test245 cases / 110611 assertions;make testallsuites (Java 315);
make lint;make codegen-checkclean;make cross-validate55/55 sections;make differential-fuzz1000/1000 withrow=588/0 column=412/0. The new fixture emits byte-identical output across
go × java × cpp and across row × column.
make fmt-checkwas not run — noclang-format locally, and it has no CI job either (tracked in
llmdoc/memory/doc-gaps.md).Review history, and two things worth knowing before merging
Twelve independent blind review rounds in isolated fixed-commit snapshots, each
with a fresh reviewer inheriting no context. Two were blocking, and both were in
my own fixes rather than in the original change:
capacity()measures — while the memory actually sits in element payload(strings and Variants inside
ItemWrite, a heap block peradded_items_row).Two rounds of work were bounding the wrong quantity.
try, so a throwing node skipped it —and a throwing node holds the most payload, since
apply_outputnever ran.resetis also what applies the ceiling, so the throw path escaped both.Both are fixed, with resets now on both sides and the trailing one outside
try/catch.Two honest gaps, stated in the code rather than papered over:
one node's reset and the next node's acquire, and every in-engine observation
point sits after an acquire reset, so an operator probing the buffer reads
zero either way. I wrote two such tests and deleted both when each passed
against a mutant with the fixed line removed. Verifying a change there needs
an allocator live-bytes probe: RSS is useless, since the two variants can
differ by under 0.1% with the allocator caching freed blocks.
stays green. Kept as defence in depth so the "execute() sees a clean buffer"
invariant holds locally, and the comment says exactly that rather than
claiming coverage.
Does the reuse pay for itself? Measured by an independent reviewer with paired
interleaved runs on pinned cores: +2.02% wall on
large_1000(10/10 pairs) and+1.76% on
transform_heavy_1000(9/10), allocation bytes down 4.6% atsmall_100 rising to 34.3% at large_5000. A single run would sit inside this
repo's documented ±5-7% layout noise; the paired design is what makes it
credible. Allocation count barely moves, so the saving is bytes churned by the
doubling sequence, not call count.
Filed along the way, not fixed here
100000000000000000000vs1e+20. Reproduced on cleanorigin/master; thisbranch touches no Go or Java.
storage_modefalls back to (go/java row, cpp column), and Java matches case-insensitively.
The Apple DSL rejects invalid values at compile time, so only hand-written
JSON reaches it.