Conversation
Three changes to the egglog backend, requiring the egglog-experimental build from egraphs-good/egglog-experimental#63: 1. The rewrite schedule uses (back-off :node-limit N :eager-apply 1) with get-node-size! :until guards: the scheduler checks the live e-node count before each rule and stops approving matches at the node limit, instead of choosing all matches and overshooting it by up to 4.7x. Node limits now measure e-nodes, like egg, rather than total table rows. 2. One egglog subprocess per worker instead of one per call: the prelude and all rule declarations are identical across calls within a run, so they are loaded once and each call is isolated with push/pop. The subprocess lives in its own custodian so per-test timeouts cannot reclaim it; an in-flight flag discards it if a call is interrupted mid-protocol. dump:egglog still spawns per call so dumps stay self-contained. 3. Extraction uses multi-extract's :dag output, which let-binds subterms shared across variants instead of expanding every variant to a tree (responses shrink ~12x). The response is read directly from the subprocess port and converted with memoized e1/e2 conversion that interns each shared subterm once, embedding batchrefs as children. Also wires *egglog-variants-limit* into patch.rkt (previously dead; the default keeps the hardcoded 1000000). On bench/numerics (seed 1): report wall time 365.7s -> 84.2s, rewrite phase 128.3s -> 19.7s (egg: 60.2s / 10.4s); final e-graph sizes now match egg's distribution; per-test errors unchanged and one blow-up-induced timeout fixed. Steps 2 and 3 are bit-identical in behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves conflicts with the batch->block rename (herbie-fp#1641): keeps the persistent-subprocess and :dag extraction paths, renamed to the block API (block-add!, output-block, insert-block/insert-vs, egglog-dag->blockrefs), and keeps *egglog-variants-limit* wired in patch.rkt over the upstream hardcoded literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dd5a3a1 to
859c56f
Compare
egglog's num_nodes now excludes relations as well as base-sort functions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Expand rules with a bare-variable left-hand side (pow1: a -> (pow a 1)) into one rule per spec constructor instead of dropping them. egg applies such rules; skipping them made egglog do less per iteration, run more iterations, and extract ~2x wider root classes at the same node limit. - Run constant folding with the default scheduler, not through the back-off instance: it is not rewriting to be rationed, and sharing the instance advanced its iteration counter twice per repeat, halving ban lengths. - Declare one indexed `herbie-const` constructor in the prelude instead of one constructor command per binding (tens of thousands of round trips per run). bench/numerics, seed 1, one thread: 79.9 s -> 74.3 s wall (egg 60.0 s), 477,707 -> 357,988 rewrite candidates (egg 297,539), mean end error 1.444 (egg 1.492). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The back-off scheduler now always applies each rule's matches before consulting the next rule (the :eager-apply tag was removed upstream), so drop it from the schedule. Stop the rewrite loop as soon as bad-merge? fires: check it in the :until guard alongside the node limit, run bad-merge-rule after the rewrite phase as well as after constant folding, and saturate const-fold with the same guard so an unsound merge cannot keep folding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A program with no real-typed roots (e.g. an FPCore whose output is a vector) produced an egglog call ending in (multi-extract N :dag) with zero expressions. egglog rejects that as a parse error, so the reader blocked on a response that never came and the test sat at the per-test timeout. With no roots there is nothing to insert or extract; return no variants without consulting the subprocess, matching the egg backend. Fixes the "Return input vector" timeout in bench/arrays.fpcore (150s -> 0.4s, result identical to the egg backend). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pavpanchekha
left a comment
There was a problem hiding this comment.
This PR mixes a bunch of different optimizations. Most of them are basically good, I like it, and I'm very excited to get egglog closer to egg performance. I basically see two "big" changes I want:
- The process caching idea is way complex and has a lot of sharp edges. Like, what if egglog spins forever. Its owning benchmark times out, next one runs, and what, it just waits in line? I'm not sure, but the lifetime of the egglog process is confusing to me. And I don't like the fact that caching-specific code is spread throughout the codebase. If we're gonna cache the process, I'd rather it be in a single place. Honestly some of this code looks un-reviewed.
- The multi-extract code looks basically fine, but it really feels like we should move the dag-resolution code into the
egglog-multi-extractmethod, and then also the fact that that method does its own CLI I/O handling is suspicious. Ideally we'd optimizeegglog-sendfor all cases instead of having a special case use a fast path.
| ;; declarations are already loaded (see static-egglog-commands), and isolate | ||
| ;; this call with push/pop. With dump:egglog, spawn a fresh subprocess so | ||
| ;; every dump file is a complete, replayable session. | ||
| (define use-persistent? (not (flag-set? 'dump 'egglog))) |
There was a problem hiding this comment.
And actually rather the dump exist and work the same way whether or not we’re using a persistent session, the reason being, we use these dumps for actually debugging egglog, and we wanted to be faithful to how Herbie actually uses it
There was a problem hiding this comment.
In other words, we don’t actually wanna dump the commands Herbie would send for a single call. We want to dump the commands Herbie actually sent, for egglog debugging.
| (define subproc | ||
| (cond | ||
| [use-persistent? (get-persistent-subprocess (static-egglog-commands pform))] | ||
| [else | ||
| (define fresh (create-new-egglog-subprocess label)) | ||
| ;; 1. Add the prelude - send directly to egglog. | ||
| (prelude fresh #:mixed-egraph? #t) | ||
| fresh])) |
There was a problem hiding this comment.
Seems better to encapsulate all this in get-persistent-subprocess? All the way from use-persistent? though the branch and creation logic?
| (when use-persistent? | ||
| (egglog-send subproc '(push))) |
There was a problem hiding this comment.
Probably also this? Like, we should make some abstraction over the caching so we don't have to think about it all the time.
| (unless use-persistent? | ||
| (apply egglog-send subproc (egglog-step-commands step pform))) |
There was a problem hiding this comment.
Why not just put these in the prelude?
| (egglog-multi-extract subproc | ||
| `(multi-extract ,extract |
There was a problem hiding this comment.
Wait what does egglog-multi-extract do that we still need to spell out the multi-extract command here?
| (define (persistent-subprocess-usable? static-commands) | ||
| (and persistent-subprocess | ||
| (not persistent-call-in-progress?) | ||
| (not (port-closed? (egglog-subprocess-input persistent-subprocess))) | ||
| (eq? (subprocess-status (egglog-subprocess-process persistent-subprocess)) 'running) | ||
| (equal? persistent-subprocess-key static-commands))) |
There was a problem hiding this comment.
This all feels quite complex.
| (define output (expr->e1-pattern (rule-output rule))) | ||
| (cond | ||
| [(symbol? input) | ||
| ;; A bare-variable left-hand side (e.g. pow1: a -> (pow a 1)) matches |
There was a problem hiding this comment.
Interesting and clever
| `(rule ((= ,input (,ctor ,@args))) ((union ,input ,output)) :ruleset ,tag))] | ||
| [else `((rewrite ,(expr->e1-pattern input) ,output :ruleset ,tag))])))) | ||
|
|
||
| (define (keyword-like? x) |
There was a problem hiding this comment.
When exactly does this come up? This seems super hacky.
| ; Add the binding and constructor union to all-bindings for the future rule | ||
| (set! all-bindings (cons curr-var-spec-binding all-bindings)) | ||
| (set! all-bindings (cons `(union (,constructor-name) ,binding-name) all-bindings)) | ||
| (set! all-bindings (cons `(union (herbie-const ,constructor-num) ,binding-name) all-bindings)) |
|
|
||
| ;; Send an extract command and read its single s-expression response directly | ||
| ;; from the subprocess port. Responses can be many megabytes, so this avoids | ||
| ;; materializing them as intermediate line strings before parsing. |
There was a problem hiding this comment.
This code also seems really hacky. I get that the intermediate string is probably bad for perf, but it's worth checking what other outputs we ever read from egglog that can't be read. Like, why not extend the benefits of this optimization to everyone?
|
@yihozhang if you want to address these, great, if you want me to take over the PR that's OK too. |
Draft: requires the egglog-experimental build from egraphs-good/egglog-experimental#63 (pinned to egraphs-good/egglog#1005, currently rev
924b5cd0) on PATH. Nightly-infra note: theegglog-herbieMakefile target installs egglog-experimental from upstream main, which lacks:dagand the back-off tags this branch uses — and because Herbie blocks on egglog protocol errors, that binary turns every test into a silent 150 s timeout. Until #63 merges, the target must install from that PR's branch.What's added
(back-off :node-limit ,node-limit)with an:untilguard on(or (bad-merge?) (<= node-limit (get-node-size!))). The scheduler (Give schedulers e-graph access and per-rule match application egraphs-good/egglog#1005) applies each rule's chosen matches before consulting the next rule and checks the live e-node count against the limit, instead of choosing all matches against a stale size; the e-graph is rebuilt once per iteration, as in egg. Node limits count e-nodes (like egg) rather than total table rows.bad-merge-rulenow also runs after the rewrite phase, and both the rewrite loop and constant folding stop as soon asbad-merge?fires, so an unsound merge halts the schedule instead of feeding further growth.pow1: a -> (pow a 1)) are expanded into one rule per spec constructor instead of being dropped (egg applies them; skipping them made egglog do less per iteration, run more iterations, and extract ~2× wider root classes at the same node limit). Constant folding saturates with the default scheduler ((saturate (run const-fold :until (bad-merge?)))), like egg's analysis-based folding: it is not rewriting to be rationed, and running it through the back-off instance advanced that scheduler's iteration counter twice per repeat, halving ban lengths relative to egg.push/pop. Timeout-interrupted calls are detected and the subprocess discarded;dump:egglogstill spawns per call so dumps stay self-contained. Bindings are anchored by one indexedherbie-constconstructor declared in the prelude instead of oneconstructorcommand per binding.multi-extract ... :daglet-binds subterms shared across variants instead of expanding each variant to a tree (~12× smaller responses); the response isreaddirectly from the subprocess port and interned with per-shared-subterm memoization.multi-extract— an egglog parse error that left Herbie blocked on a response until the per-test timeout.run-egglognow returns no variants without consulting the subprocess, matching the egg path. Fixes theReturn input vectortimeout inbench/arrays.fpcore(150 s → 0.4 s, result identical to egg).*egglog-variants-limit*is wired intopatch.rkt(previously dead code; default keeps the hardcoded 1,000,000 — no behavior change).Performance (bench/numerics, seed 1,
--threads 1):dagWhere the remaining single-threaded 1.24× goes (profile in the branch's
egglog-scheduler-design.md): Herbie'sreconstruct!scales with candidate count (egglog still returns 1.2× egg's), e-matching over egg-length bans, per-callpush/popcloning of the rule set, and the taylor-lowering path's fixed per-call cost.Full bench/ evaluation (seed 1,
--threads 8, 150 s/test, 718 FPCores)Both backends run sequentially from the same checkout on an idle 128-core machine, 2026-08-31:
Accuracy is a wash: 51 egglog wins / 61 losses / 598 within 0.1 bits over the 710 tests both scored, with symmetric tails. Summed per-test time is 1.07× egg's — the remaining serial overhead is fully absorbed by parallelism. The failure set is now identical between backends: 5 pre-existing errors and the 2 expected fidget timeouts (prospero, bear), all backend-independent. The regressions in the previous version of this evaluation (1.41× wall, 45 series-phase timeouts, 4 vector-program crashes, multi-GB memory spikes on the
projsuite) are gone with the schedule changes and the no-roots fix above.A replication of the nightly flow (
infra/nightly.sh bench reports --threads 2, date seed) completes all 718 tests in 26m14s of benchmarking with the same 7 backend-independent failures.🤖 Generated with Claude Code