WIP: Materialized rebuild index + single-rule UF rebuild, rebuilt on #36 - #3
Draft
oflatt-claude wants to merge 5 commits into
Draft
Conversation
Replace the per-column eq-sort rebuild fan-out with a single rule per child eq-sort, driven by an @uf edge joined against a materialized term->row index. This mirrors how a native rebuild iterates the e-nodes referencing a changed e-class, instead of matching the view once per column. - Index: one hidden `(function <F>Index_<S> (S key...) Unit :merge old)` per distinct child eq-sort, maintained wherever the view is written or deleted. Leading with the term makes "which rows mention this term" a key-prefix lookup. Container children are not indexed: they carry no @uf row and are canonicalized structurally. - uf_canon / uf_canon_proof: per-eq-sort canonicalization primitives, expressed as the generic view-column read over the two-output @UF_<S> table so every backend (including Differential Dataflow) services them against its own storage. Registered from the sort's Sort command, so they survive re-parse. Each takes a fallback, making them leader-or-self and proof-or-reflexive. - The rule re-canonicalizes every eq-sort child of the row in its action, so one firing fixes the whole row. It reads @uf there, so it is :unsafe-seminaive; the driving @uf delta in the body makes that read sound. - Container children keep their per-column :naive rule, and the FD view's value column keeps its own rule. instrument_construct_into wrote the view with a raw `set`, bypassing update_fd_view; it now goes through update_fd_view so its rows get index entries. Without this those rows were unreachable from the rebuild rule and stayed stale, which unsafe_seminaive_matches_naive caught. Regenerated the proof snapshots: the different rebuild order yields different (checker-validated) proof derivations, plus fresh-var renumbering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
proof_encoding.md's rebuilding section now shows the index-driven rule that replaced the per-column eq-sort fan-out, plus a CHANGELOG entry. rebuild_index_covers_every_view_row asserts the invariant the rule depends on: a view row missing its index entries is unreachable from the @UF-driven rebuild and silently stays stale. Verified non-vacuous by reverting the instrument_construct_into fix, which the test then catches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The index atom already binds the view row's key, so joining the view for its (eclass, proof) was a third body atom the search had to plan. Read those columns in the action instead, leaving the body as the @uf delta plus the index lookup. The read is fallback-free: Backend::register_view_column_lookup returns None on an absent key, and ExternalFunction::invoke halts the calling rule when no value comes back — the same outcome as a body join that fails to match. The existing register_view_column_read keeps its fallback for set-if-empty's connector proof, where the caller does have a meaningful default. The DD interpreter cannot abandon an action mid-flight, so it reports an absent key instead of inventing a value; the index witnesses the row, so it never fires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Binding the moved term at a known key position in the index atom makes its leader a static substitution, so the action re-keys the row and composes one Congr at that position from the edge proof the body already bound. This drops the whole-row recanon: no uf_canon read per eq-sort child, no reflexive <S>Proof lookup per child, and no Congr mint for children that did not move (which the simplifier only discarded later anyway). A row with k stale children now takes k firings, as it did before this branch. Proof derivations move back to the baseline's shapes, so most of the regenerated snapshots revert; the 11 that remain differ only by fresh-var renumbering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Driving the rebuild from a known child position removed every caller of uf_canon / uf_canon_proof, so stop registering two primitives per eq-sort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Draft — this is saulshanabrook#19 rebuilt on top of saulshanabrook#36, and the result is a measured regression. Not for merge as-is.
Rebuilds #19 (materialized rebuild index + single-rule UF rebuild) on top of #36 (hash-consed term/proof tables). saulshanabrook#19's original diff no longer applies —
rebuilding_rulesmoved toproof_encoding_rebuild.rs, views are now always FD (keyed on children) for constructors and custom functions, and saulshanabrook#36 added theStmts/minthash-consing layer — so this is a reimplementation rather than a rebase.Base is saulshanabrook#36's head (
f5fb271), so the diff here is only the saulshanabrook#19 rebuild.Why this was worth retrying
saulshanabrook#19 measured ~+18% on
math-microbenchmarkand diagnosed the cause as the native rebuild scanning the extra index tables':internal-ufcolumns. saulshanabrook#36 makes native rebuild provably dead under the encoding (forbid_native_rebuild), which should have removed exactly that cost. It did not — the approach is still slower — so that diagnosis was wrong. The measurements below locate the real cost instead.What it does
(function <F>Index_<S> (S key...) Unit :merge old)per view, per distinct child eq-sort, holding for each view row one entry perS-typed child: that child first, then the row's whole key. Leading with the child makes "which rows mention this term" a key-prefix lookup. Maintained wherever the view is written or deleted. Container children are not indexed — they have no@UFrow to drive a lookup.@UF_<S>edge joined against the index rather than by matching the view. The moved term is bound at a known position, so its leader is a plain substitution and proof mode composes oneCongrthere from the edge proof the body already bound.Backend::register_view_column_lookup, a fallback-free(keys) -> columnview read.ExternalFunction::invokereturningNonehalts the calling rule, which is exactly the semantics a body join has when the row is absent — so unlike the existing fallback-takingregister_view_column_read, there is no need to invent an e-class for a row that is not there. The DD interpreter cannot abandon an action mid-flight, so it reports an absent key instead; the index witnesses the row, so it never fires.Two dead ends worth recording, both verified against the code:
uf_canonas aReadPrim(as WIP: Materialized rebuild index + single-rule UF rebuild saulshanabrook/egglog-encoding#19 had it) is rejected by the Differential Dataflow backend in an action context —requires a backend action registry, 22 DD test failures. Re-expressing it as a generic view-column read over the two-output@UF_<S>table fixed that. It is gone from the head anyway: binding the child position removed every caller.:mergemaintain the index (so the index could carry the e-class and absorb the value-column rule) is not expressible. A:mergebody sees onlyold/new(0/1) andletlocals —MergeFnhasOld,New,OldCol(i),NewCol(i),LetVar(slot)and no way to name a key column — so it cannot name the index row to update. It would need aKeyCol(i)variant in the bridge. WIP: Materialized rebuild index + single-rule UF rebuild saulshanabrook/egglog-encoding#19 hit the same wall.A real bug this surfaced
instrument_construct_intowrote the view with a rawset, bypassingupdate_fd_view, so those rows got no index entries and were unreachable from the rebuild rule — they silently stayed stale.unsafe_seminaive_matches_naivecaught it. Addedrebuild_index_covers_every_view_row, which asserts the invariant directly; verified non-vacuous by reverting the fix, which the test then catches.Testing
Whole workspace green:
--test files792 passed,--lib69 passed,egglog-experimental-dd --test files135 passed, 0 failures anywhere.cargo fmt --all --checkand clippy clean. Proof-mode tests run withverify_proofs, so every passing proof test is checker-validated, not just snapshot-compared.11 proof snapshots regenerated, differing only by fresh-var renumbering — the per-position rule reproduces the baseline's proof derivations.
Performance — still a regression, but the shape changed
bench.py --target . --compare-target @f5fb271 --treatment proofs --compare-treatment proofs,6 rounds per endpoint/file, machine otherwise idle. Ratios are candidate / baseline.
Three variants were built and measured in turn:
uf_canonrecanon, view joined in the bodyCongr, nouf_canon(head)Head — wall time, suite total 1.10–1.11x slower.
Peak RSS — 6–16% higher on 5 of 6. The index costs one entry per eq-sort child per
view row, which gives back part of saulshanabrook#36's memory win.
Where the time goes
Rebuildis 0 ns on both sides everywhere, so native rebuild is not involved. Per-rulesetdeltas on math-microbenchmark, tracked across the three variants:
@rebuildingSearch ΔThe interesting result is the middle row: once the view join leaves the body, the
index-driven rebuild searches faster than the per-column fan-out it replaces. The premise
of saulshanabrook#19 — that a materialized term→row index beats matching the view — does hold. What sank
the first version was the third body atom, not the index.
What remains is write cost, not search cost, in two places no rebuild-rule tuning reaches:
Merge). Every term construction writes
kindex rows on top of its view row. This isinherent to keeping a materialized index, and it is also where the extra RSS comes from.
@rebuildingMerge (+797 ms), the re-keying writes themselves.Whole-row recanon and per-position are a wash overall (1.08–1.13x vs 1.10–1.11x, overlapping
CIs): whole-row fixes a row in one firing but pays
kunion-find reads andkmostly-reflexiveCongrmints per firing; per-position pays O(1) per firing but needskfirings, tradingApply for Merge. Head keeps per-position because it is simpler — no
uf_canonprimitives atall, and proof derivations match the baseline's.
Conclusion: the index's search win is real and reproducible, but it does not cover the
maintenance and re-keying writes it adds. Preserved for reference, as saulshanabrook#19 was.
🤖 Generated with Claude Code