feat(xtest): benchmark two named refs via workflow_dispatch - #583
feat(xtest): benchmark two named refs via workflow_dispatch#583dmihalcik-virtru wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
First dispatch:
|
| cell | metric | baseline | candidate | ratio (95% CI) | p (BH) | n | verdict |
|---|---|---|---|---|---|---|---|
| encrypt-1MiB-control | wall | 471.8 ms | 455.4 ms | 1.011x [0.947, 1.038] | — | 57 | PASS |
| encrypt-1MiB-control | rss | 71.7 MB | 70.3 MB | 0.995x [0.972, 1.018] | — | 57 | PASS |
| encrypt-1KiB | wall | 455.0 ms | 443.4 ms | 0.968x [0.944, 1.040] | 0.830 | 48 | PASS |
| encrypt-1KiB | rss | 65.6 MB | 64.7 MB | 0.977x [0.949, 1.003] | 0.987 | 48 | PASS |
| encrypt-1MiB | wall | 447.6 ms | 473.7 ms | 1.039x [0.990, 1.093] | 0.339 | 60 | PASS |
| encrypt-1MiB | rss | 71.6 MB | 70.1 MB | 0.998x [0.981, 1.027] | 0.830 | 60 | PASS |
| encrypt-32MiB | wall | 526.7 ms | 533.6 ms | 1.012x [0.963, 1.041] | 0.830 | 34 | PASS |
| encrypt-32MiB | rss | 179.3 MB | 176.9 MB | 0.998x [0.981, 1.024] | 0.830 | 34 | PASS |
| decrypt-1KiB | wall | 533.6 ms | 526.6 ms | 1.012x [0.957, 1.051] | 0.830 | 34 | PASS |
| decrypt-1MiB | wall | 513.1 ms | 540.4 ms | 1.052x [0.971, 1.112] | 0.616 | 60 | PASS |
| decrypt-32MiB | wall | 600.7 ms | 596.1 ms | 0.981x [0.920, 1.062] | 0.830 | 60 | PASS |
| decrypt-32MiB | rss | 195.9 MB | 195.9 MB | 0.999x [0.984, 1.011] | 0.830 | 60 | PASS |
CPU rows omitted; all ungated, all PASS, none interesting.
The honest reading: this run could not have found what #3865 claims
PASS here means "not 15% slower". It does not mean the allocation and memcpy reductions did not happen. Look at how wall clock scales with payload:
| operation | 1 KiB | 1 MiB | 32 MiB | payload-dependent |
|---|---|---|---|---|
| encrypt | 455.0 ms | 447.6 ms | 526.7 ms | ~72 ms |
| decrypt | 533.6 ms | 513.1 ms | 600.7 ms | ~67 ms |
Going from 1 KiB to 32 MiB — a 32,000x increase in payload — adds 72 ms. About 450 ms of every encrypt is fixed cost: Go runtime start, config load, TLS handshake, token fetch, KAS key fetch. Payload-dependent work is ~13.6% of the largest cell we have.
The gate is 1.15x of the total, which is +79 ms. That is more than the entire payload-dependent portion. So:
At the current payload sizes, an encrypt cell cannot fail on payload-path work at any magnitude. A candidate that doubled every per-segment cost would come in at 1.136x and report PASS.
The 1 MiB cells are worse than useless for this: they are indistinguishable from 1 KiB (both ~450 ms) and the -7.4 ms at 1 MiB is noise, not a payload effect.
This is not a statistics problem — the intervals are tight (±2–4% on RSS) and the control is clean. It is a cell matrix problem, and it means my "32 MiB encrypt is where the claim has room to show" in the PR description was wrong. It has ~13% of the room it needs. The README's "only 32 MiB means throughput" overstates what that cell isolates, and I will fix both.
Peak RSS is the more interesting null. It was genuinely measured — the floor was 12.4 MB against a 179 MB peak, nowhere near censoring — and the interval [0.981, 1.024] at 32 MiB encrypt rules out a change larger than ~2%. Sixteen 2 MiB segment allocations removed against a 179 MB peak is ~18% if they were all live simultaneously; they are not, and Go's allocator reuses them. That is a real, if narrow, result: the allocation reduction does not lower peak RSS at 32 MiB. It says nothing about allocation count or GC pressure, which peak RSS cannot see.
Two harness fixes this turned up
-
Payload sizes are too small to gate throughput. Needs a cell large enough that payload work dominates fixed cost — 512 MiB puts it at ~70% of wall clock rather than 14%. That cell is slower, so it trades against the round count.
-
max_roundsbinds long before the budget does. Three of seven cells stopped atmax_rounds, notprecision, and the whole run used 418 s of its 1500 s budget. Over a third of the cells wanted more rounds while two thirds of the allowance went unspent. Raising--bench-max-roundsnarrows those intervals for free, and the freed budget is roughly what a 512 MiB cell would cost.
Both are worth doing before this comparison is re-run, because right now a green result on the encrypt path is close to uninformative about anything except startup cost.
Still to run
The stacked-baseline dispatch (bench-baseline-ref: feat/DSPX-2604) to isolate this commit from #3782. Given the above I would rather fix the payload matrix first — against the current cells it would return the same uninformative PASS.
The nightly benchmark compares the newest release against the branch head, which is the right question every night and the wrong one to ask about a specific change: the baseline carries every other commit since the release. Adds bench-baseline-ref / bench-candidate-ref. Both refs are resolved and built from source side by side, and arm selection is told which is which explicitly, so neither has to be a release. The *-ref inputs are untouched and still drive the functional matrix. The bench matrix now honours focus-sdk, so investigating one SDK no longer spends 45 minutes each measuring the two nobody asked about. Bad combinations fail in resolve-versions, before a runner is spent: only one arm named, focus-sdk left at 'all', or two refs that turn out to be the same commit. otdf-sdk-mgr resolved a branch reached *by name* to a tag that kept its slashes, while the same branch reached by SHA was flattened to '--'. A dist tag is one path component -- all_versions_of() lists dist/*/ and the go Makefile finds src/*/ -- so 'feat/x' installed as a build named 'feat' with no cli.sh, which raises during collection. Flatten both paths the same way.
390c5fb to
3481193
Compare
51f3d7c to
0b491d0
Compare
X-Test Failure Report |
At 32 MiB a go encrypt spends ~450 ms on fixed startup against ~72 ms that scales with the payload, so payload work is ~14% of the cell and the 1.15x gate is wider than the whole of it: a candidate that doubled every per-segment cost would report 1.136x and PASS. Gating throughput needs a size where that ratio inverts, which at 1 GiB it does (~2.3 s payload work against the same ~450 ms fixed). Adds --bench-payloads, plus bench-payloads / bench-budget-seconds / bench-max-rounds dispatch inputs so a manual run -- which is not repeated on the nightly's schedule -- can afford sizes the nightly cannot. The scheduled run supplies no inputs, so the workflow falls back to today's 1KiB,1MiB,32MiB / 1500 s / 60 rounds rather than moving the nightly. Making 1 GiB survivable took four supporting changes: - Payload files are written in 8 MiB chunks rather than one randbytes call, so a 1 GiB payload is not first built in RAM. The chunk size is a multiple of 4 -- CPython draws a 32-bit word at a time -- so the bytes are identical to the unchunked stream and the seed contract holds. - Arm outputs are unlinked in a finally. Each cell leaves two files the size of its payload and nothing reads them again; at 1 GiB that is 2 GiB per cell that every later cell has to fit around, so the cell that fails on disk is not the one that filled it. - A disk preflight refuses the run up front. ENOSPC mid-run arrives as a non-zero exit from the CLI under measurement, which reads as 'this build is broken' rather than 'the runner is out of space'. - Cells run smallest-first within an SDK, so a run that exhausts its budget loses the most expensive cell rather than an arbitrary one. The A/A control stays pinned at 1 MiB instead of following the selection: its width is the run's noise floor and every other cell is judged against it, so it has to mean the same thing across runs. CI validates bench-payloads with perf.cells.parse_payloads under a bare python3 -- perf.cells is stdlib-only -- rather than a second copy of the grammar in bash that would drift from the one pytest enforces.
timeout-minutes was 45 against a default bench-budget-seconds of 1500. That only ever worked because setup was ~3 minutes with a warm Go module cache; a cold cache took 19, and 19 + 25 does not fit in 45. A job killed mid-measurement loses the report entirely, which is strictly worse than one that collects fewer rounds -- the budget already handles the latter. Raise it to 90 so the backstop is for a hung job rather than a working one, which is also what a dispatch asking for 1 GiB payloads needs.
|
1 participant
Stacked on #580 — base is
DSPX-4372, so review that one first. This PR issmall; most of it is the discussion below, which is the point of opening it.
What this adds
Two
workflow_dispatchinputs on X-Test,bench-baseline-refandbench-candidate-ref, that point the existing benchmark harness at two refsyou name instead of its nightly newest-release-vs-branch-head pair. Plus a
resolver fix that a slashed branch name needs, and the bench matrix now
honours
focus-sdk.Second commit: three more inputs —
bench-payloads,bench-budget-seconds,bench-max-rounds— because the first dispatch showed the default matrixcannot resolve a throughput claim at all. See Why 32 MiB cannot gate
throughput below. The scheduled nightly
passes no inputs and is unchanged.
To run the comparison this PR is written around:
run-benchmarksfocus-sdkgobench-baseline-refmainbench-candidate-reffeat/DSPX-2604-createtdf-chunkedbench-payloads1KiB,1GiBbench-budget-seconds5400bench-max-rounds200The change under the microscope
opentdf/platform#3865 — refactor(sdk): rewrite CreateTDF on top of
ChunkedWriter — rewrites
CreateTDFContextto delegate payload encryption,archive framing, and manifest assembly to
ChunkedWriter. It makes explicitperformance claims, per segment:
and states costs: three extra interface dispatches per segment,
io.Copyoveran
io.MultiReader, and anO(n log n)sort over ~34k map keys at the 68 GiBmaximum.
That is a good test case for the harness precisely because it is not a
regression hunt. It is a claim of an improvement, of a size the harness may or
may not be able to see, on a path the harness may or may not exercise. Walking
through what the run would and would not tell us is the clearest way to explain
what the thing does.
How the harness works, and what it would say here
Full design notes are in
xtest/perf/README.md; thisis the tour.
It measures whole CLI invocations, not functions
There is no Go benchmark here and no in-process timing. Each measurement forks
otdfctl encrypt(ordecrypt) and records wall clock, CPU time, and peak RSSfrom
os.wait4. The cell matrix per SDK is{encrypt, decrypt} × {1 KiB, 1 MiB, 32 MiB}, plus one A/A control.The consequence for #3865: the 1 KiB cells are almost entirely process boot.
Go runtime start, config load, TLS handshake, token fetch, KAS public key
fetch. Removing one 2 MiB allocation is real, and it is going to be a rounding
error next to a token fetch. The
min(segmentSize, inputSize)read-bufferchange is exactly the kind of improvement this harness is structurally unable
to resolve at 1 KiB.
Why 32 MiB cannot gate throughput
I assumed the 32 MiB encrypt cell — 16 segments deep — was where the claim had
room to show. The first dispatch says otherwise, and the arithmetic is worth
stating because it applies to any throughput claim, not just this one.
Fitting the measured go cells against payload size:
At 32 MiB the payload-dependent part of an encrypt is ~72 ms of a ~527 ms cell.
The 1.15x gate is +79 ms — wider than the entire portion of the cell that
scales with the payload. A candidate that doubled every per-segment cost
would report 1.136x and PASS. The default matrix is a startup-cost gate wearing
a throughput gate's clothes.
At 1 GiB the ratio inverts: ~2.3 s of payload work against the same ~450 ms
fixed, ~84% of the cell. That is what
bench-payloadsis for, and why thesecond commit exists. If #3865 is visible anywhere it is there, and in peak
RSS — a full-segment allocation removed per segment is a memory claim first
and a time claim second.
Cost of asking: a 1 GiB pair is ~3 s per round per arm, so it needs a budget to
match. Manual dispatches are not on the nightly's schedule, which is the whole
argument for letting them spend more;
bench-budget-secondsandbench-max-roundsare the two knobs, and both have to move — at the default60, cells were stopping on
max_roundswith two thirds of the 1500 s budgetunspent.
It compares two builds on one runner, never against history
Both builds are installed side by side under
sdk/go/dist/, and every roundruns both, order shuffled within the round. The statistic is the within-round
log-ratio. Runner speed, tenancy, and steal time are shared factors and divide
out. Nothing is stored and nothing is diffed against last week.
This is why "benchmark this branch" has to mean "install both refs on one
runner", which is all the plumbing in this PR really is.
The verdicts, and which one #3865 should expect
The gate is one-sided: a cell is a REGRESSION when the bootstrap CI lower
bound on the ratio exceeds 1.15x and the BH-adjusted p < 0.05. An improvement
reports IMPROVED and fails nothing.
So the honest expected outcome for this branch is PASS on every cell, with
maybe an IMPROVED on 32 MiB encrypt. That is not a disappointing result; the
question the harness is built to answer is "did this rewrite cost anything?",
and for a change that swaps one implementation for another under an unchanged
public entry point, that is the question worth asking. The PR's own risk
section says the same thing: one implementation now serves both entry points.
IMPROVEDis worth reading with the same suspicion as a regression, though.The PR says payload output is unchanged and the round-trip matrix pins it —
good, because a candidate that got faster by doing less work is the failure
mode a ratio cannot distinguish from one that got faster by doing the same work
better. The harness has a narrow guard for this (
comparability_problem()refuses to compare arms that disagree on
hexless,hexaflexible, orautoconfigure) but it is a feature-flag check, not a correctness check. Thefunctional xtest suite is what backs the "same work" premise; the benchmark
assumes it.
Inconclusive is the likely answer, and it is not a pass
Each SDK gets an A/A cell comparing the baseline against itself through the
identical pipeline. Its true ratio is 1.0 by construction, so its CI width is
the run's noise floor — the smallest effect this runner could have resolved. If
the floor is wider than the 15% threshold, cells report inconclusive rather
than PASS, because "we looked and found nothing" only counts when we could have
found something.
On a shared GitHub runner the floor is frequently wider than the improvement
#3865 is claiming. A cell that comes back inconclusive means the run could not
tell — not that the change did nothing. Note that this is a different failure
from the one above: an inconclusive cell admits it could not resolve the
effect, whereas a 32 MiB PASS does not, because the noise floor can be
perfectly tight around a ratio that is mostly process startup.
Peak RSS has a second trap the harness handles explicitly: readings that sit at
the measurement floor get censored. Both arms clip to the same value,
producing a
1.000xratio with a tight interval — the most convincing-lookingPASS the harness can emit, and completely meaningless. That floor is why
perf/_launcher.pyexists at all: on Linuxru_maxrsscomes back asmax(child's true peak, parent's RSS at fork time),so forking from pytest would report pytest's ~165 MiB for every invocation.
Two caveats specific to this comparison
The baseline includes the parent branch. #3865 is stacked on #3782 —
its base is
feat/DSPX-2604, notmain. Dispatching withbench-baseline-ref: mainmeasures the whole stack: theChunkedWriterintroduced in #3782 and the rewrite that puts
CreateTDFon top of it. Toisolate the top commit, set
bench-baseline-ref: feat/DSPX-2604. Bothdispatches are worth running and they answer different questions — this is the
generalisation of the existing "anything about your change specifically if the
baseline moved too" caveat in the README, and stacked branches make it sharp.
The server is pinned to
mainregardless. The bench job always starts theplatform at
mainwith a single KAS — the six extra KAS instances the ABACtests need would draw background CPU on the runner doing the measuring. #3865
is a client-side SDK change so that is the right call here, but it means the
harness cannot see a candidate whose speed depends on a matching server change.
The plumbing, and one bug it turned up
Mostly wiring: resolve the two refs with
otdf-sdk-mgr versions resolve, handthe result to
setup-cli-toolin place of the matrix version-info, and pass--bench-baseline/--bench-candidateto pytest so arm selection does notfall back to "newest final release", which does not exist when both arms are
branches.
Fail-fast checks run in
resolve-versions, before a runner is spent, because amatrix cannot be narrowed from inside the job it belongs to: only one arm
named,
focus-sdkleft atall, or two refs that resolve to the same commit.That last one is the same trap the README already documents for java, where
v0.18.0 == mainmademain latestinstall one build and every cell skip.The bug:
otdf-sdk-mgrflattened/to--when it resolved a branch reachedby SHA, and left the slashes in when it reached the same branch by name
— which is the shape a
workflow_dispatchinput arrives in. A dist tag is onepath component;
tdfs.all_versions_of()listsdist/*/and the goMakefilefinds
src/*/. Sofeat/DSPX-2604-createtdf-chunkedwould have installed as abuild named
featwith nocli.shin it, andall_versions_of()raises onthat during collection, before any cell can report why. Both paths flatten now,
with a test on each.
Reviewing
Does
bench-baseline-ref/bench-candidate-refread as clearly as the*-refinputs beside it, given they mean something different (arms of onecomparison, not versions of a matrix)?
Narrowing the bench matrix by
focus-sdkchanges existing dispatchbehaviour. It seems strictly better — nobody dispatching
focus-sdk: gowanted 90 minutes of java and js — but it is a behaviour change, not just an
addition.
Is the fail-fast set the right one? Notably it does not refuse a candidate
older than the baseline, which is a legitimate thing to want.
The second commit's supporting changes are all consequences of 1 GiB being
survivable rather than of it being requested: chunked payload generation (no
1 GiB
bytesin RAM), arm outputs unlinked in afinally(2 GiB per cellotherwise, so the cell that fails on disk is not the one that filled it), a
disk preflight (ENOSPC mid-run reads as "this build is broken"), and
smallest-cell-first ordering so a short run loses the expensive cell rather
than an arbitrary one. Each is defensible alone; together they are a fair
amount of machinery to carry for an opt-in size.
The A/A control stays pinned at 1 MiB rather than following
bench-payloads, so the reported noise floor means the same thing acrossruns. It does mean a
1GiB-only run's floor is measured at a size no othercell uses.
Once this is in I will dispatch both comparisons —
mainandfeat/DSPX-2604as baselines — at 1 GiB, and post the tables here.