Repository navigation
Benchmark regression - 2026-05-21 #282
Description
Bencher detected a performance regression in the nightly benchmark run (concurrent_file/readers_plus_writer).
Run: https://github.com/project-minigraf/minigraf/actions/runs/26208789060
Please review the Bencher dashboard for details on which benchmarks regressed.
Activity
adityamukho commentedon May 21, 2026
Closing as likely CI runner noise — the 10k variants were only ~17-22% slower (consistent with a slow runner), and the 100k regression amplifies that via fsync jitter on shared runners. The benchmarked commit (5fca2eb) is the #281 fix, so no code regression is expected. Will reopen if confirmed by next week's nightly runs.
adityamukho commentedon May 24, 2026
I dug through the nightly benchmark history for
concurrent_file/readers_plus_writer. My conclusion: this should not be closed as pure runner noise, but the May 21 alert also does not look like a new regression introduced by5fca2eb.The May 21 run is in the same performance band as the immediately surrounding runs. The broader history shows a sustained slowdown that appears earlier, around the May 16 nightly.
25715698048643fb6825780986047643fb6825844189250cdc91e4259028264172f156f025953856255bb4387125982764035aa770952601680260848d2bb4261447222815a71350262087890605fca2eb262714628022647857263248316562647857Observations:
concurrent_file/readers_plus_writer100k variants before5fca2eblanded.5fca2ebdid not introduce a fresh regression.bb43871(perf: predicate push-down for Expr clauses + mixed rule optimization (#207, #206) (#249)). That is the first suspect based on run history.bb43871did modifybenches/minigraf_bench.rs, but not theconcurrent_file/readers_plus_writerbenchmark definition. The benchmark-file diff added other benchmark code, so this does not look like a simple benchmark-definition change for this group.5a71350,5fca2eb,2647857) do not explain the step change by themselves; they mostly live within the post-May-16 slower band.So I would reframe this issue rather than close it as runner noise:
Suggested next step: investigate
bb43871/ #249 as the likely introduction point, or intentionally rebaseline only if that slowdown is expected and justified.adityamukho commentedon May 24, 2026
Update after local worktree comparison: this is a legitimate regression, not benchmark runner noise.
I checked out and benchmarked the relevant commits in separate worktrees on the same machine:
2f156f0fc58329df28df2ab608b02d1d20655d4ein/tmp/minigraf-baseline-282bb43871b24d949dc39c9de7d729b5939193c0e8din/tmp/minigraf-suspect-282cargo bench --bench minigraf_bench -- "^concurrent_file/readers_plus_writer/"Local Criterion medians:
10k_4t10k_8t10k_16t100k_4t100k_8t100k_16tThat shape matches the nightly signal: the slowdown scales with dataset size and thread count, and reproduces in a controlled local A/B run.
Root cause:
bb43871changedselective_fact_fetchto collect all bound entities and all bound attributes across the query, then union both lookup sets.[:e0 :val ?v].get_facts_by_entity(:e0)andget_facts_by_attribute(:val).:valfact, so a selective entity+attribute query degraded into a broad attribute scan.I confirmed the mechanism with a local patch on the suspect worktree that avoided the attribute scan when the same pattern already had a bound entity. The same benchmark points dropped from ms/s ranges into microseconds:
10k_4t:[45.842 µs 48.266 µs 49.960 µs]100k_4t:[39.377 µs 40.111 µs 40.744 µs]I then implemented the safer version in
src/query/datalog/executor.rs: lookup choice is now per pattern. A pattern with a bound entity uses the entity index and does not also add its bound attribute lookup; patterns without a bound entity can still add bound attribute lookups. If any pattern has neither, the executor falls back to the full scan path. This preserves multi-pattern join correctness while fixing the point-lookup regression.Regression test added:
test_selective_fetch_prefers_bound_entity_over_attribute_scan. It failed before the fix withleft: 22, right: 2, proving the bad attribute-scan candidate set; it passes after the fix.Verification on the patched tree:
cargo test test_selective_fetch_prefers_bound_entity_over_attribute_scan: passedcargo test --test complex_queries_test: passed, 11/11cargo test: passed, including doctestscargo bench --bench minigraf_bench -- "^concurrent_file/readers_plus_writer/(10k_4t|100k_4t)$":10k_4t:[47.995 µs 63.860 µs 74.986 µs]100k_4t:[36.247 µs 37.676 µs 38.689 µs]Conclusion: this issue is a real regression introduced by the selective fetch change, specifically by unioning a broad attribute lookup with an already-selective entity lookup for the same pattern.