Repository navigation
perf(engine): a set costs a fraction of the variable it is declared over - #691
Merged
FBumann merged 4 commits intoAug 14, 2026
Conversation
FBumann
force-pushed
the
claude/pr-687-performance-n6rrs6
branch
from
August 14, 2026 06:08
b40335a to
3a87296
Compare
FBumann
changed the base branch from
claude/sos-constraints-sinks-t3h0nj
to
claude/diagnostics-what-the-sink-added
August 14, 2026 06:08
Merging this PR will not alter performance
Comparing Footnotes
|
FBumann
force-pushed
the
claude/pr-687-performance-n6rrs6
branch
from
August 14, 2026 06:14
3a87296 to
bbddbd3
Compare
…nded Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017LHj7GrWFG1KYFjY2fwDNN
…says Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
FBumann
force-pushed
the
claude/pr-687-performance-n6rrs6
branch
from
August 14, 2026 09:30
bbddbd3 to
195c561
Compare
This was referenced Aug 14, 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
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.
Stacked on #689, which is stacked on #688, which is stacked on #687 — review those first; this diff is only what it adds. Rebased onto
0429cc8; the numbers below were taken ate2c76e1, which differs from it only by a commit touchingexamples/README.md.Note
The following content was generated by AI.
The build side is ~4× faster and the reformulation ~1.6×. On
examples/sos.yamlat 20 000 snapshots × 20 generators × 5 breakpoints (2 000 000 members, 2 800 000 columns, 1 220 000 rows), against the stack's head:e2c76e1_build_sos— placing every setsos.reformulated(tables)structuredigest (a rebind's keep-or-reload question)Ranges are the min of 15 reps in each of three interleaved rounds, on a shared 4-core box; medians move ±25% and are not worth quoting. A model that declares no set still pays nothing: 971 ms against 988 ms over four alternating rounds of the same build with the
sos:block dropped, the sign flipping between them.Where the time was
Placing the sets read every member's coordinate back into an ordinal.
row_major(…, ordinal_of)costs areplace_strictagainst a per-label dictionary for every dim binding did not encode as anEnum— 30 ms forsnapshot, 23 ms forbp, plus 6 ms building the 20 000-entry dict.But a set and a weight are the two halves of a coordinate's position in the declared product, split at
over: withcits cardinality andsthe product of the dims after it, a position readshcs + ws + l, the weight isw, and the set ishs + l— the position the same coordinate would have in the product withoutover. And the position is the label, wherever the variable dropped nothing, so an unmasked variable's sets are two divisions onint_rangeand its frame is never read. A masked one still reads its ordinals and still renumbers densely; that renumbering turned out to be load-bearing, for the reason_DTYPESalready gives aboutcol— a pre-mask position can pass 2^31 while every survivor fits, so a gapped set number is one anInt32cannot hold. This is the splitlabels.framealready makes ("nothing renumbers unless a row was dropped", #520), in the one other place that numbers by position.SosDeclaration.dimsgoes with it. It was the variable'sforeachminusover— a second home for a fact, and the engine now needs whereoversits in the fullforeachanyway, so it asksProgram.variable.The reformulation built a 14-column × 2M frame to answer questions that are all scans. One of those columns,
#cardinality row, was computed with acum_sumand read by nothing at all.filter(holds)copied all 14 to use 2,n_unique()hashed a column already sorted, and the two refusals were two passes.sinks/sos.pyis numpy end to end now — the same departure #687 already made for_linking_matrix, and the module docstring says so where the departure is.The linking block also got simpler on the way.
np.repeat(binary, entries)already names the right binary in every slot except a member's own and the segment closing into it, so three masked scatters became one broadcast, one scatter and one-= 1:setandweightwereInt64in the one frame with a row per member. Both are bounded by a count that already has to fit a 32-bit index — a set holds at least one column, a weight counts along one dim of one variable — so they areInt32for the reasoncolis. 16 MB less carried, and the digest reads it.Verified
Full suite green on the rebase (1393 passed, 6 skipped),
ruff format --check,ruff checkandpyreflyclean. The gurobi and LP-file arms oftest_sos.pyboth run here, so the native lane is covered by the same change as the reformulating one — and #688'spiecewise: {method: sos2}expands into asos:block, so its cases go down this path too.Two guards are new, because the placement now has two paths where it had one:
foreachorders, since the split is about whereoversits in it;Int32bound needs.Not checked: nothing was measured on a quiet machine, and no solver other than HiGHS and Gurobi has read a file this writes — both inherited from #687.
Mutation table — every guard the change rests on, deleted in turn
Re-run after the rebase.
uv run pytest -q -x tests/test_sos.py, falling back to the whole suite where that was green.over(the stride)overdim (the span)overordinal¹ survived the first pass and got a purpose-built probe before merge — the density one above. Both mutations leave a gapped set numbering, which every sink downstream tolerates (they read edges off
!=, and_n_setsonly offsets the next declaration), so nothing observable broke until the numbering itself was asserted.Method
Base
e2c76e1, both arms in separate processes off one venv (PYTHONPATHat a worktree of the base), alternating base/head, model built once per process and the two functions timed in isolation — a whole build is ~1 s on this box and the difference of two of those is noise.structureis re-read off a fresh_tables()each rep so thecached_propertyis not being timed.The end-to-end marginal (
buildwith thesos:block againstbuildwithout) moved 115–146 ms → 4–28 ms, consistent with the isolated numbers plus the narrower stream, but it is a difference of two ~1 s builds and is quoted only as corroboration. That pair was taken against94d8948, an earlier head of #687; the sos paths are byte-identical across every rebase this branch has been through, so it was not re-run.Deliberately not done
_add_setsin the gurobi sink is untouched, and it is the next thing that will hurt. It callsx.tolist(), which materialises a PythonVarper model column rather than per member — the opposite of what its docstring claims — and then loopspartition_by('set'), one frame per set. At 400 000 sets that lane is not usable. It is the native path rather than the one this PR made fast, and measuring it needs a licence past the wheel's size limit, so it is a finding and not a change.typeandbig_mstill ride per member, and are still hashed per member by the digest, though both are per-declaration constants. Dropping them fromstructurewould be reasoning from the declarations where that docstring deliberately reasons from the data, and it would trade a correctness margin for ~30 ms._row_startsstill allocates two temporaries it could write through. ~7 ms, against four lines where there are two.import numpyinsinks/sos.py, where the neighbouringtables.pydefers it into each function: this module is numpy in seven of its eight functions now, it already imports polars at module level, and it is reachable only from the engine-side sink registry.