chadtmiller.com/knotless

Switch repository

Loading…

What changes

The gate test waits 8 seconds, not 30. a_merge_queue_waits_for_its_gate in crates/knotless-run/tests/run.rs sets the runner's review_wait, then sits out what is left of it before checking the pull is ejected, saying how long it waited. The wait is now 8 seconds and the expected message says so. The check that the pull is not ejected before the wait is up still runs whenever the test's pass reaches it in time.

Why

Measuring the integration binary on the runner (a temporary timing guard in every test, gathered from a draft pull) showed the binary is bound by real work, not by waiting: about 1,700 test-seconds spread over 8 or so test threads, of which fixed sleeps and timer waits are under a tenth. The measured cost per operation is what a pull, a push or a reconcile takes against the shared PDS while the host runs other jobs: a git push to the PDS about 0.6 s, a pull create or merge about 0.65 s, a runner pass about 1.3 s. The one wait that was pure idle and easy to shorten without weakening what it checks is this 30 second review wait, so that is what changes.

How it was checked

The change is a constant and one expected string in a test. CI's test check runs the binary.

Changeset: none

Shortcomings

It saves about 20 seconds of one test, a small part of the binary's time. Most of the rest is work the tests have to do against the PDS.

This is a throwaway measurement and says so: it isn't meant to merge. It adds a Timing guard to every test in crates/knotless-run/tests/run.rs. Once 38 tests have finished, the last one panics on purpose with a table of start, end and duration for each test. It also narrows the test workflow's cov step to COV_ONLY=run COV_NETS=spaces.

I found no bug that would hurt the measurement. Notes for reading the numbers:

• The hard-coded 38 must match the test count. If a test is added or removed, or one returns early as a_private_repository_runs_in_its_space does without spaces, the table never prints, or it prints early.
• The deliberate panic makes the binary fail, and cov's retry runs it a second time. Read the first failure's log, because T0 and the row table start fresh on the retry.
• With COV_ONLY and COV_NETS narrowed, the 100% coverage gate and the stock-PDS run no longer cover the whole workspace. Timing taken here is only for run.rs on the spaces net.

Before any real change goes in, revert the workflow edit and every Timing line. This draft needs no changeset and no docs.

This round adds per-category timing to the throwaway measurement: Guard records time for git, knotless_bin, cli, record, wait_until, step and pass. Each Timing row now gets those totals appended. I found nothing that breaks the measurement, and my earlier approval still stands. The draft says it isn't meant to merge.

Notes for reading the numbers:

• The categories nest, so they overlap and don't sum to the test's duration. pass includes step, and wait_until includes any git, cli or record calls made inside its closure.
• acc_take doesn't clear the accumulator despite its name. That is fine here because each test runs on its own thread, so the totals stay per-test.

Before any real change goes in, revert all of the Guard and Timing code and the workflow edit.

This round adds a global KeyGuard map. It records total, count and max time per key (git <subcommand>, cli <a> <b>, record <collection>, step, idle+step). The Timing panic message now ends with the top 40 keys. It also shortens the merge-queue gate wait in a_merge_queue_waits_for_its_gate from 30s to 5s, and updates the expected message to match. My earlier approval still stands, because I found nothing that breaks the measurement.

Notes for reading the numbers:

• KEYS is global across all tests, so its totals are summed across threads. They overlap with each other and with wall time.
• idle+step includes step, and it also includes any nested git, cli or record calls.
• The 5s wait makes a_merge_queue_waits_for_its_gate flaky under load. The comment says the check is skipped when the wait runs out, but the message assert at the end isn't obviously skipped. Don't merge that change as it is.

Before any real change goes in, revert all the Guard, KeyGuard and Timing code, the wait change and the workflow edit.

This round removes the timing instrumentation, which is what my earlier reviews asked for before merging. It deletes the Timing, Guard and KeyGuard code and every call site in run.rs and common/mod.rs. It also restores the test workflow's cov step by dropping COV_ONLY=run COV_NETS=spaces. A search of crates on the head branch finds no Guard( or test-Timing references left. The Timings hits are in telemetry.rs and are unrelated.

The one change that stays is a_merge_queue_waits_for_its_gate. The wait goes from 30s to 8s, and the expected message now says "8s". The comment is accurate: the test skips its waiting check if the wait runs out before the pass sees it, so a loaded run won't flake on that check. The message assert at the end still holds because it uses the same wait value. The title and body still say "draft, not for merging", so update them to match what the pull now does.