fix(codegen): deterministic ord via single-thread fact interning#208
fix(codegen): deterministic ord via single-thread fact interning#208hdz284 wants to merge 2 commits into
ord via single-thread fact interning#208Conversation
01f1731 to
42b933c
Compare
ord deterministic under multithreadingord via single-thread fact interning
| } | ||
| } | ||
| } else { | ||
| // Each worker ingests its own ~1/N byte-range slice in parallel. |
There was a problem hiding this comment.
Only when ord builtin function is used we need the deterministic loading. ord is default require to enable str-intern.
There was a problem hiding this comment.
Done — the deterministic load is now gated on actual ord use: deterministic_load = str_intern_enabled() && program.uses_ord(). --str-intern programs that never call ord keep the fully parallel loader. Program::uses_ord() also walks loop/fixpoint blocks. Rebased onto main-next; cargo test 328/0 and the full 19-family DOOP suite is byte-exact + deterministic at -w32.
42b933c to
8786779
Compare
`ord(x)` compiles to `x`'s string-interning key (a `lasso` `Spur`, handed out in insertion order). The binary loader reads each fact file as parallel byte ranges, so under `-w N>1` workers intern their slices concurrently and every `ord` value -- and any `min(ord(..))` representative built on it (e.g. the DOOP port's `HeapAllocation_Merge`) -- becomes nondeterministic across worker counts and even between runs. Only `-w 1` (sequential interning) was stable. Load fact files on worker 0 alone -- in the same order the single-thread run uses -- exactly when the program uses `ord`; evaluation still runs on every worker. `ord` is the only construct that can observe interning order: equality on interned strings is a bijection, ordered string comparison resolves operands back to content (`resolve(l) < resolve(r)`), and `min`/`max` are numeric-only so a string extremum must route through `ord`. Gating the serial load on `Program::uses_ord()` (which also walks `loop`/`fixpoint` blocks) keeps `-w N` byte-for-byte identical to `-w 1` (and to Souffle) wherever it matters, while every other program -- including `--str-intern` programs that never call `ord` -- keeps the fully parallel loader at no cost. `ord`'s value is unchanged, so no fixtures or reference outputs need re-baselining. Signed-off-by: Hangdong Zhao <hdz284@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8786779 to
82b1049
Compare
The retry-with-yield loop around try_get_or_intern looks like a redundant spin over an already thread-safe interner, so it is an easy target for a "cleanup". It is not: under concurrent interning lasso's lock-free arena can transiently return Err(FailedAllocation) when workers race on arena growth, and only retrying recovers. Empirically a bare get_or_intern crashes -w32 nondeterministically while the retry loop recovers at zero measurable cost. Document this so it is not removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HarukiMoriarty
left a comment
There was a problem hiding this comment.
Fix is correct and the determinism write-up is great. One structural ask before merge: the new ord-detection (flowlog-parser/src/program/builtins.rs + mod builtins; + its 5 tests) re-implements a walk flowlog-typechecker::check_builtin_config_requirements (primitive/mod.rs:38) already does — same rule/head/compare/factor traversal, already branches on BuiltinOperator::Ord, already has config in hand. Let's make "needs deterministic load" a side effect of that existing check, carried on the unified Config, and drop the standalone walker.
Guidance
1. flowlog-common::Config — add a derived flag (same category as the existing derived output_to_stdout):
pub struct Config {
// ...
/// Set by the typechecker when the program uses `ord(_)`. Forces the
/// binary-mode loader to intern on worker 0 alone so `ord` keys match
/// `-w 1`. Derived from the program, not user-supplied.
pub serialize_load: bool,
}
// getter, next to str_intern_enabled():
pub fn serialize_load(&self) -> bool { self.serialize_load }The 4 exhaustive Config { .. } literals each need serialize_load: false: cli.rs:80, pipeline.rs:99, and the two test literals (dedup.rs:96, planner_errors.rs:20).
2. check_program — take &mut Config (lib.rs:81) and fold detection into the one walk. Rewriting check_builtin_config_requirements as a pure "first ord span" finder + a decision at the top keeps the nested fns capture-free and does both jobs:
pub(crate) fn resolve_ord_load(
program: &Program,
config: &mut Config,
) -> Result<(), TypeCheckError> {
fn arith(a: &Arithmetic) -> Option<Span> {
factor(a.init()).or_else(|| a.rest().iter().find_map(|(_, f)| factor(f)))
}
fn factor(f: &Factor) -> Option<Span> {
match f {
Factor::Var(_) | Factor::Const(_) => None,
Factor::FnCall(fc) => fc.args().iter().find_map(arith),
Factor::Builtin(bc) if bc.op() == BuiltinOperator::Ord => Some(bc.span()),
Factor::Builtin(bc) => bc.args().iter().find_map(arith),
Factor::Cast(c) => factor(c.inner()),
Factor::Group(a) => arith(a),
Factor::Tuple(t) => t.exprs().find_map(arith),
Factor::TupleProj { tuple, .. } => arith(tuple),
}
}
// NOTE: chain `as_loop()` — the current loop only visits `as_rules()`
// (line 69), which skips `loop`/`fixpoint` blocks.
let ord_span = program.segments().iter().find_map(|seg| {
let plain = seg.as_rules().iter();
let looped = seg.as_loop().into_iter().flat_map(|b| b.rules().iter());
plain.chain(looped).find_map(|rule| {
let body = rule.rhs().iter().find_map(|p| match p {
Predicate::PositiveAtom(_) | Predicate::NegativeAtom(_) => None,
Predicate::Compare(c) => arith(c.left()).or_else(|| arith(c.right())),
});
body.or_else(|| {
rule.head().head_arguments().iter().find_map(|h| match h {
HeadArg::Var(_) => None,
HeadArg::Arith(a) => arith(a),
HeadArg::Aggregation(agg) => arith(agg.arithmetic()),
})
})
})
});
if let Some(span) = ord_span {
if !config.str_intern_enabled() {
return Err(TypeCheckError::OrdRequiresStrIntern { span });
}
config.serialize_load = true; // the side effect
}
Ok(())
}Two things this fixes vs. the current function: (a) it no longer early-returns when str_intern is on (primitive/mod.rs:42-43) — that early-return is why the ord + --str-intern case would otherwise never be walked; (b) it now walks loop/fixpoint rules, which also closes a latent gap where ord inside a fixpoint with --str-intern off currently escapes OrdRequiresStrIntern. Update the call in check_program (lib.rs:85) to the new name.
3. io/input.rs:67 — consume the flag, drop the Program::uses_ord() call:
let deterministic_load = self.config.serialize_load();str_intern is implied (the flag is only ever set under it), so both the && and uses_ord() disappear.
4. Delete flowlog-parser/src/program/builtins.rs and the mod builtins; line in program/mod.rs. Move the essential cases — positive, comparison, min(ord()) aggregation, and the fixpoint-nested regression — into a flowlog-typechecker test asserting the side effect:
let mut cfg = Config { str_intern: true, ..Default::default() };
check_program(&mut prog, &mut cfg).unwrap();
assert!(cfg.serialize_load);5. Point the 5 check_program call sites at &mut: main.rs:40, pipeline.rs:57, common.rs:32, typechecker_errors.rs:16, program_planner.rs:129 (the &Config::default() ones become &mut Config::default()).
One nuance worth a code comment: the flag is set at typecheck, before const-fold/DCE, so a dead ord-bearing rule that's later eliminated leaves it conservatively true — an unnecessary serial load, never a wrong result.
|
Minor / non-blocking — just to scope the guarantee: worker-0 load pins interning order for strings that come from input files / inline facts, but That's fine for the DOOP |
|
@hdz284 quick reminder to sign off the commits — |
Summary
ord(x)compiles tox's string-interning key (alassoSpur, handed out ininsertion order). The binary loader reads each fact file as parallel byte-ranges,
so under
-w N>1workers intern their slices concurrently — everyordvalue,and any
min(ord(..))representative built on it (e.g. the DOOP port'sHeapAllocation_Merge), becomes nondeterministic across worker counts andeven run-to-run. Only
-w 1was stable.Fix: when the program uses
ord, load facts on worker 0 alone, in thesame order the single-thread run uses; evaluation still runs on every worker.
So
-w Ninterning order ==-w 1== Souffle, andord(with everymin(ord(..))) is deterministic and byte-exact again.ord's value isunchanged — no fixtures or reference outputs need re-baselining.
Why gate on
ord(addresses the review on #208)ordis the only construct that can observe interning order, so serialisingthe load for every
--str-internprogram was unnecessary:Spureq == content eq);resolve(l) < resolve(r)— resolves to content;min/maxsemirings are numeric-only, so a string extremum must go throughord.Hence
deterministic_load = str_intern_enabled() && program.uses_ord(). Every--str-internprogram that never callsordkeeps the fully parallel loader atno cost. New
Program::uses_ord()also walksloop/fixpointblocks (a gapthe typechecker's own
ordcheck has viaSegment::as_rules).Validation
cargo test --release --workspace— 328 passed / 0 failed (incl. 5 newuses_ordunit tests).cargo fmt --checkclean. Rebased onmain-next.-w32vsSouffle
-j32(compiled and run threaded), luindex, each run twice:19/19 byte-exact to Souffle AND run-to-run deterministic, every count ==
the
-w1reference. The worst pre-fix family(
1-object-1-type-sensitive+heap, which drifted ~146 K tuples run-to-run) isnow MATCH.
-w32)-w32)ordvalue / referencesOverhead: nil for
ord-free programs; a bounded ~2 s worker-0 load forordprograms (FlowLog
-w32timings/RSS match the pre-fix build, e.g.context-insensitive 19.1 s / 4.5 GB). Full runtime + peak-RSS table available.
Updated from the previous single-thread-interning commit with the requested
ord-gate refinement, and rebased ontomain-next.