Benchmark regression - 2026-05-21

[github-actions]

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.

[adityamukho]

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]

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 by 5fca2eb.

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.

Run date	Run	Commit	10k_4t	10k_8t	10k_16t	100k_4t	100k_8t	100k_16t
2026-05-12	25715698048	643fb68	18.9 ms	38.7 ms	82.4 ms	236.8 ms	482.3 ms	1.05 s
2026-05-13	25780986047	643fb68	19.9 ms	41.0 ms	86.8 ms	251.1 ms	553.9 ms	1.13 s
2026-05-14	25844189250	cdc91e4	22.0 ms	41.4 ms	89.5 ms	241.9 ms	453.8 ms	963 ms
2026-05-15	25902826417	2f156f0	18.5 ms	39.2 ms	83.2 ms	221.5 ms	441.2 ms	950 ms
2026-05-16	25953856255	bb43871	31.9 ms	62.2 ms	140.0 ms	762.2 ms	1.75 s	4.06 s
2026-05-17	25982764035	aa77095	33.8 ms	65.8 ms	145.1 ms	818.5 ms	1.94 s	4.34 s
2026-05-18	26016802608	48d2bb4	34.1 ms	63.4 ms	141.4 ms	791.3 ms	2.00 s	4.07 s
2026-05-20	26144722281	5a71350	28.4 ms	58.0 ms	129.2 ms	863.6 ms	2.19 s	4.54 s
2026-05-21	26208789060	5fca2eb	28.0 ms	57.7 ms	127.3 ms	905.7 ms	2.19 s	4.34 s
2026-05-22	26271462802	2647857	28.7 ms	57.9 ms	124.7 ms	808.6 ms	1.80 s	3.76 s
2026-05-23	26324831656	2647857	27.8 ms	57.1 ms	124.6 ms	865.8 ms	2.10 s	4.28 s

Observations:

• The May 21 alert is not isolated: May 20 already had Bencher alerts for concurrent_file/readers_plus_writer 100k variants before 5fca2eb landed.
• The May 21 numbers are close to May 20, May 22, and May 23. That supports the narrower claim that 5fca2eb did not introduce a fresh regression.
• However, the May 16+ band is much slower than May 12-15: roughly 50-70% slower at 10k, and roughly 3x-4.5x slower at 100k.
• The first sampled run in the slower band is bb43871 (perf: predicate push-down for Expr clauses + mixed rule optimization (#207, #206) (#249)). That is the first suspect based on run history.
• bb43871 did modify benches/minigraf_bench.rs, but not the concurrent_file/readers_plus_writer benchmark definition. The benchmark-file diff added other benchmark code, so this does not look like a simple benchmark-definition change for this group.
• Later commits (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:

concurrent_file/readers_plus_writer has a sustained regression since the May 16 nightly. The May 21 alert is probably not a new 5fca2eb regression, but the benchmark family remains legitimately slower than the pre-May-16 baseline.

Suggested next step: investigate bb43871 / #249 as the likely introduction point, or intentionally rebaseline only if that slowdown is expected and justified.

[adityamukho]

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:

• baseline: 2f156f0fc58329df28df2ab608b02d1d20655d4e in /tmp/minigraf-baseline-282
• suspect: bb43871b24d949dc39c9de7d729b5939193c0e8d in /tmp/minigraf-suspect-282
• command in each worktree: cargo bench --bench minigraf_bench -- "^concurrent_file/readers_plus_writer/"

Local Criterion medians:

benchmark	baseline	suspect	slowdown
10k_4t	16.107 ms	36.532 ms	2.27x
10k_8t	34.712 ms	89.268 ms	2.57x
10k_16t	78.680 ms	205.63 ms	2.61x
100k_4t	165.62 ms	691.27 ms	4.17x
100k_8t	372.17 ms	1.7894 s	4.81x
100k_16t	797.49 ms	3.9141 s	4.91x

That 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:

• bb43871 changed selective_fact_fetch to collect all bound entities and all bound attributes across the query, then union both lookup sets.
• The benchmark query is effectively a point lookup like [:e0 :val ?v].
• For that query, the executor was fetching both get_facts_by_entity(:e0) and get_facts_by_attribute(:val).
• The attribute lookup pulls every :val fact, 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 with left: 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: passed
• cargo test --test complex_queries_test: passed, 11/11
• cargo test: passed, including doctests
• cargo 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.