dtourolle
e8e96eed40
docs/requirements.md §8 has said since it was written that performance is verified by "an automated benchmark suite against a synthetic 50k catalog, run per-commit … A regression beyond stated tolerance fails the build." There was none. No benches/, no [[bench]], no criterion, no synthetic catalog, and three CI workflows that between them measured nothing. Ten performance requirements could therefore be neither passed nor failed, and five of them carried a TRACES: tag regardless. tools/bench is the half of that promise that can be kept honestly on a runner with no GPU and no display. # The fixture Rows are cheap and pixels are not, so it builds fifty thousand catalog rows over a pool of a dozen real files, each referenced by several thousand of them. Everything the catalog half touches is rows and is exact at full scale; everything the pixel half touches is one file at a time and does not care how many rows point at it. Fourteen megabytes on disk instead of two terabytes, and neither half is flattered by the trade. It is reproducible from a seed, and a stamp beside it — seed, row count, source size, dr-catalog's schema version — rebuilds it rather than letting a run be compared against a baseline that describes a different library. # What it can now pass or fail NFR-P1, and R2's second sentence with it: Catalog::open plus the count, first window and timeline the grid cannot paint without. The interesting part turned out to be the open itself — schema::backfill runs three passes over the images table on every open, which is O(library) work on a path whose budget is stated in absolute seconds. Tagged TRACES: NFR-P1, on a gate that fails if it breaks. NFR-P3: thumbnail throughput on the embedded preview path, through the same per-image work spawn_thumbnail_sweep does and in the same shape — chunks of 96, lanes owning disjoint slices, the single thread that owns the store writing the finished chunk. Mirrored rather than called, because that function takes a RemoteBackend and would measure somebody's network. Tagged TRACES: NFR-P3. # What it deliberately does not claim NFR-P7 is the whole chain, and only the encode half of it runs without an adapter. So the export row is a one-sided gate — over two seconds in the encode alone violates the requirement; under it proves nothing — and there is no TRACES: NFR-P7 anywhere. NFR-P8 is about the application at idle, and the probe is a process holding the catalog and nothing else, so it records the catalog layer's share and carries no budget until somebody decides what that share should be. No tag there either. CONTRIBUTING.md asks that a requirement be closed by a test that would fail if the behaviour were removed, and two more plumbing tags is what this repository already has too many of. NFR-P8 also gets the answer §4.1 demands: RSS is exclusive of device-local GPU allocations and cannot be made otherwise, because such an allocation never enters the process's address space. The requirement should be restated as two figures, and docs/benchmarks.md says so. # Two gates, and why one of them steps aside off the reference desktop The budget is the requirement's own number and never moves. The baseline is what the reference desktop last measured, and drifting 15% past it fails the build even while still inside the budget — which is how performance rot actually arrives, never over the line, always a little worse. A budget written for twenty-four threads cannot be asserted on a two-core container. §8 names the reference desktop, not CI, so each metric declares whether its budget is machine-sensitive; those are asserted under --reference and reported everywhere else. Catalog open is not one of them: two seconds against an expected figure two orders of magnitude smaller is a threshold any machine can be held to. This is the trap core/dr-gpu/tests/frame_budget.rs already refuses — a red gate everybody learns to ignore. # The baseline ships with no numbers in it Every recorded field is null, because nobody has run it yet. Writing plausible-looking figures would make every later comparison a comparison against a guess, and the first real regression would be invisible. Run `dr-bench record --reference` on the reference desktop and commit the diff; until then the budget gate works and the report says the other one cannot. # CI .gitea/workflows/benchmark.yml, and its own workflow rather than a step in build-and-test.yml: a red "Build and test" says the code is wrong, a red "Benchmarks" says it got slower, and the second must not be reachable by retrying a flaky compile. The cpu job runs on every push and builds -p dr-bench alone — which is why that crate depends on no GPU and no UI crate. The gpu job is the frame budget that already exists and already skips without an adapter, on workflow_dispatch, because building wgpu on every commit to rediscover that the runner has no device is not a use of anybody's minutes.
181 lines
7.4 KiB
Markdown
181 lines
7.4 KiB
Markdown
# Contributing to DarkRoom
|
|
|
|
There is a lot of documentation here — 14 documents and 177 numbered
|
|
requirements — and almost all of it is written for someone who has already
|
|
decided to work on this. This file is the other thing: how to get a first
|
|
change landed without reading any of it.
|
|
|
|
## The shortest useful contribution
|
|
|
|
**A develop operation is one file.** Not one file plus a registration, plus a
|
|
shader edit, plus a control in the UI — one file:
|
|
|
|
```
|
|
core/dr-pipeline/ops/split_toning.yaml
|
|
```
|
|
|
|
`build.rs` finds it with `read_dir`, compiles it into Rust implementing
|
|
`Operation`, and from there it is indistinguishable from a hand-written node.
|
|
It arrives with controls built from its declared parameter kinds, a place in
|
|
the chain from `order:`, a place in the panel from `attributes:`, sidecar
|
|
persistence, and its own tests — which are declared in the same file and run
|
|
under `cargo test`.
|
|
|
|
Read [`core/dr-pipeline/ops/README.md`](core/dr-pipeline/ops/README.md) and
|
|
copy [`exposure.yaml`](core/dr-pipeline/ops/exposure.yaml). Split toning,
|
|
colour zones, selective colour and channel-mixer variants are all pure point
|
|
operations, which means all of them are declarations rather than code.
|
|
|
|
If you want to understand one thing about the architecture before starting,
|
|
make it this: **the core describes its capabilities and the interface composes
|
|
them.** No code in `ui/` names an operation, and a test enforces that
|
|
(`ui/dr-ui/tests/ui_names_no_operation.rs`). It is why your node needs no UI
|
|
change.
|
|
|
|
## Getting it to build
|
|
|
|
**Git LFS is required.** Model weights are stored in LFS, and a clone made
|
|
without it leaves a ~130-byte text pointer where an 11 MB model should be:
|
|
|
|
```bash
|
|
git lfs install && git lfs pull
|
|
```
|
|
|
|
Forget this and `dr-segment`'s build script stops with an instruction rather
|
|
than embedding the pointer and failing at inference time — but it is easier to
|
|
run the two commands now.
|
|
|
|
**The toolchain pins itself.** `rust-toolchain.toml` selects 1.92.0 and rustup
|
|
fetches it on first use. Do not override it; `cargo fmt` and `clippy` are both
|
|
version-sensitive and CI runs exactly this version.
|
|
|
|
**System packages.** Slint and winit need these at build time. On Debian or
|
|
Ubuntu:
|
|
|
|
```bash
|
|
sudo apt-get install pkg-config libfontconfig1-dev libxkbcommon-dev
|
|
```
|
|
|
|
**Then:**
|
|
|
|
```bash
|
|
cargo run -p darkroom-desktop
|
|
```
|
|
|
|
The first build resolves 826 crates and takes a while — on a laptop, long
|
|
enough to look like a hang. It is not one.
|
|
|
|
Android is a containerised toolchain and is not needed for most work; see
|
|
[`docker/android/README.md`](docker/android/README.md) if you get there.
|
|
|
|
## What CI will check
|
|
|
|
All four of these run on every push, so run them before you send anything:
|
|
|
|
```bash
|
|
cargo fmt --all -- --check
|
|
cargo clippy --workspace --all-targets -- -D warnings
|
|
cargo test --workspace
|
|
cargo build --workspace --release
|
|
```
|
|
|
|
GPU tests skip themselves where there is no adapter rather than failing — a
|
|
test that cannot run is not evidence either way — so a green run on a machine
|
|
without a GPU is expected, and does not mean the GPU paths were exercised.
|
|
|
|
There is a fifth check, and it is not in that list because you are unlikely to
|
|
break it by accident:
|
|
|
|
```bash
|
|
cargo run --release -p dr-bench -- check
|
|
```
|
|
|
|
That is the benchmark suite (`docs/requirements.md` §8), which builds a
|
|
synthetic 50,000-image catalog and fails the build if a performance target is
|
|
missed or a measurement has drifted past its tolerance. It runs on every push in
|
|
its own workflow. [`docs/benchmarks.md`](docs/benchmarks.md) says what it
|
|
measures, what it deliberately does not, and how to read a failure. If you have
|
|
touched the catalog, the decoder, the thumbnail store or the exporter, run it
|
|
before you send.
|
|
|
|
## Requirements and traceability
|
|
|
|
[`requirements.md`](docs/requirements.md) is the register of record.
|
|
[`traceability.md`](docs/traceability.md) is generated from `TRACES:` tags in
|
|
the source and must never be hand-edited:
|
|
|
|
```rust
|
|
// TRACES: FR-DEV-3a | FR-DEV-3c
|
|
```
|
|
|
|
Tags are read from `.rs`, `.slint`, `.wgsl` and `.yaml` — the last so a
|
|
declared operation can record the requirement it satisfies, since the Rust it
|
|
generates lands in `OUT_DIR` and is not scanned.
|
|
|
|
A pre-commit hook regenerates the matrix and stages it whenever you touch
|
|
something that can carry a tag, so you should not have to think about it. If
|
|
you do need to run it by hand:
|
|
|
|
```bash
|
|
cargo run -p traceability -- report
|
|
```
|
|
|
|
Note that it tracks line numbers, so a change that only moves code still moves
|
|
the matrix. Never regenerate it with a stale prebuilt binary.
|
|
|
|
**One convention that the tooling cannot enforce.** A tag proves that a tag
|
|
exists, not that the code under it does the thing — `docs/code-health.md`
|
|
CH-4 has the details, and two requirements currently read as covered on the
|
|
strength of plumbing a future feature would use. So: **close a requirement
|
|
with a test that would fail if the behaviour were removed.** Coverage that
|
|
moves slowly and means something beats coverage that moves quickly.
|
|
|
|
## Two invariants the build defends
|
|
|
|
Worth knowing before you trip one, because both failures name a requirement
|
|
rather than a line:
|
|
|
|
- **No operation may be named in `ui/`** (FR-DEV-3a). Special-casing one
|
|
operation in the panel to fix a layout problem is how a generated interface
|
|
stops being generated. If a node needs presentation the panel cannot give it,
|
|
the answer is a `presentation:` hint in the declaration and a `WidgetKind`,
|
|
not a branch in `develop.rs`.
|
|
- **The operation schema rejects ambiguity at build time**: a duplicate
|
|
`order:`, a filename disagreeing with its `id:`, a default outside its own
|
|
range, an expression naming something that is not a parameter. Each error
|
|
names the key you got wrong and exits rather than panicking.
|
|
|
|
## Commit messages
|
|
|
|
Imperative subject describing the change from the reader's side — "Offer the
|
|
merge when two people turn out to share a name", not "fix: merge dialog". No
|
|
conventional-commits prefixes.
|
|
|
|
The body is where the reasoning goes, and it is expected to be substantial when
|
|
the change is. This codebase records *why* far more than most, in commits and
|
|
in comments alike, and that is the single habit most worth adopting: the
|
|
constraint you worked around is invisible to whoever reads the diff next.
|
|
|
|
One commit per change. If you fixed two things, that is two commits.
|
|
|
|
## Where to read next, in order
|
|
|
|
| Document | Read it when |
|
|
|---|---|
|
|
| [`core/dr-pipeline/ops/README.md`](core/dr-pipeline/ops/README.md) | Adding or changing a develop operation — start here regardless |
|
|
| [`docs/architecture.md`](docs/architecture.md) | Anything touching the render path, catalog or sync |
|
|
| [`docs/code-health.md`](docs/code-health.md) | Deciding what to work on; grades each seam by what it costs |
|
|
| [`docs/benchmarks.md`](docs/benchmarks.md) | A change that could plausibly cost time or memory |
|
|
| [`docs/technical-debt.md`](docs/technical-debt.md) | Something looks wrong — check it was not chosen |
|
|
| [`docs/distribution.md`](docs/distribution.md) | Packaging a build, or adding a permission to one |
|
|
| [`docs/requirements.md`](docs/requirements.md) | Reference, not reading |
|
|
|
|
`technical-debt.md` is the one to check before "fixing" anything surprising.
|
|
It records compromises that were deliberate, each with the reasoning and a
|
|
falsifiable condition for when it stops being one — the point being that you
|
|
can tell a constraint from an accident without asking.
|
|
|
|
## Licence
|
|
|
|
GPL-3.0-or-later. By contributing you agree your work is licensed the same way.
|