chore(bench): a profiler, a regression suite, and the gate fix main needs today - #182
Merged
Conversation
docs/benchmarks.md says the ~2x is "dominated by fixed work — re-scanning the
vars table per section, ~100M printf calls, a 2.6 GB CSV write". Those are all
emit-side costs, and the committed phase timings say emit is not the problem.
At the l rung, taking the best of each repeat:
case/size build fk build lp ratio emit fk emit lp ratio
dispatch/l 3.89s 0.40s 9.7x 2.78s 2.79s 1.0x
transport/l 5.49s 0.90s 6.1x 3.76s 3.07s 1.2x
Emit is at parity; the whole gap is build. The phases are not doing the same
work — linopy allocates arrays cheaply and materialises coefficients at write
time, so a phase-to-phase ratio is not apples to apples — but it localises our
cost: build is 58% of our wall at dispatch/l, and emit is already as fast as
the writer we are compared against.
bench/profile_build.py tags every statement with the build step that issued
it. On dispatch/l, _build_variable is 40% of all SQL time and one statement
inside it — the chunked INSERT INTO var_p that assigns labels — is most of it.
The cost is the ORDER BY in
ROW_NUMBER() OVER (ORDER BY t_snapshot.ord, t_generator.ord)
which sorts every chunk. Measured on the same data, building the same 10M-row
frame four ways: 8.3s as written, 2.4s with the ORDER BY dropped, and 4.4s
storing ordinals rather than coordinate values. Packing the two sort columns
into one BIGINT saves 15%, so it is the sort itself and not the key width.
Dropping the ORDER BY is not free: it changes which coordinate gets which
solver index, which tests/test_walkthrough.py pins as a golden file because
that mapping is part of the documented story.
There is a variant that keeps the mapping exactly. When the mask does not
depend on the leading dim — `where: p_max > 0` depends only on generator — the
surviving trailing coordinates are the same for every leading value, so they
can be ranked once over a tiny table and the label computed arithmetically,
with no window function and no sort. Verified by building both frames and
diffing them: 0 rows differ at 1M and at 10M, 2.2x and 3.2x faster.
Two smaller items on the same path, measured at 10M rows: the vtype column is
VARCHAR ('continuous' written ten million times) where UTINYINT is 2.5x faster
(0.87s -> 0.35s), and the objective runs SUM(coeff) GROUP BY col over groups
that each hold exactly one row when the objective has a single term, where
skipping the aggregate is 2.7x faster (0.60s -> 0.22s).
No engine change here — this commit adds the profiler and the finding. The
docs' attribution is left alone until a fix lands to replace it with.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0145rKirxsCRGodhkKozjaYW
`bench/run.py` publishes how farkas compares to linopy. It cannot answer "did this PR make it worse" well: peak RSS moves with machine load, and I mis-read a coin-flip OOM as a regression twice this week before bisecting proved otherwise. `bench/regressions/` is the other half — pytest-benchmem, which is pytest-benchmark timing plus a memray peak pass, with `isolate=True` so each pass gets a fresh process. That matters twice over: duckdb would otherwise be measured with a warm buffer pool, and isolation is what makes whole-process `rss` available beside the memray peak, so the two can be watched for divergence. Measured, `dispatch/m`: | arm | ru_maxrss | memray peak | |---|---|---| | farkas | 309 MB | 211 MB | | linopy | 604 MB | 2967 MB | memray counts polars' reserved arenas as allocated and does not count the interpreter or mapped libraries at all, so the bias points in opposite directions in the two lanes: the peak ratio is 0.51x by RSS and 0.07x by memray. Publishing that would turn a defensible 2x memory win into a fictitious 14x one, false to anyone who ran /usr/bin/time. Within one lane the bias sits on both sides of a diff and cancels. So: RSS for the comparison we publish, memray for the regressions we chase, and `--benchmark-memory-compare-fail` to make the second a gate. Its own dependency group, not `dev` — nothing in the test suite or the published ladder needs memray. `testpaths` is still `tests`, so `uv run pytest` does not collect it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#148 split the solve result into linopy's two fields — `status` is the solver's health ('ok'), `termination_condition` is what it concluded ('optimal'). It landed after the harness did, so the gate has been comparing `status` against 'Optimal' and aborts every run on main before a single timing is taken.
FBumann
added
chore
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
FBumann
changed the title
This was referenced Jul 27, 2026
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
2 participants
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.
Split out of #178, which mixed harness work with engine changes. This half touches no
src/— it is measurement only, and one line of it is a bug fixmainneeds regardless of whether the perf work lands.fix(bench): the parity gate is broken on main#148 split the solve result into linopy's two fields —
statusis the solver's health ('ok'),termination_conditionis what it concluded ('optimal'). It landed after the harness did, sobench/_run_case.pycomparesstatusagainst'Optimal'and aborts every run onmainbefore a single timing is taken. One line.bench/profile_build.py: attribution, not just detectionbench/run.pysays how much slower; this says where. It wrapsexecuteand tags every statement with the build step that issued it. That is how the label sort was found — 71% of build at 10M variables in one statement — and it is what makes the next round of perf work aimable rather than speculative.bench/regressions/: the questionrun.pycannot answerrun.pypublishes how farkas compares to linopy, so it must measure peak RSS: that is the number a reader can check with/usr/bin/time. It is a poor regression detector, though — RSS moves with machine load, and I mis-read a coin-flip OOM as a regression twice before bisecting settled it.This is
pytest-benchmem: pytest-benchmark timing plus a memray peak pass, withisolate=Trueso every pass runs in a fresh process. That matters twice — duckdb would otherwise be measured with a warm buffer pool, and isolation is what makes whole-processrssavailable beside the memray peak, so the two can be watched for divergence.Why memray here and not in the published ladder. Measured,
dispatch/m, same runs reporting both:ru_maxrssmemray counts polars' reserved arenas as allocated and does not count the interpreter or mapped libraries at all, so the bias points in opposite directions in the two lanes: the peak ratio is 0.51x by RSS and 0.07x by memray. Publishing that would turn a defensible 2x memory win into a fictitious 14x one — false to the first reader who ran
/usr/bin/time. Within one lane the same bias sits on both sides of a diff and cancels, leaving a metric that is deterministic and attributable to a call stack.So: RSS for the comparison we publish, memray for the regressions we chase, and
--benchmark-memory-compare-fail=mean:10%to make the second a gate.Four workloads, ~74 s, time and both memory metrics side by side:
Its own dependency group (
--group bench), notdev— nothing in the test suite or the published ladder needs memray.testpathsstaystests, souv run pytestdoes not collect it. The tests assert a column count as a cheap correctness gate: the published ladder has a parity gate against linopy, this one at least refuses to time a model it did not build.Verification: 342 passed, 1 xfailed · ruff check + format clean · no
src/changes.🤖 Generated with Claude Code