Skip to content

core-relations: reset only modified tables' indexes in merge_all - #957

Open
oflatt-claude wants to merge 3 commits into
egraphs-good:mainfrom
oflatt-claude:merge-all-reset-dirty
Open

core-relations: reset only modified tables' indexes in merge_all#957
oflatt-claude wants to merge 3 commits into
egraphs-good:mainfrom
oflatt-claude:merge-all-reset-dirty

Conversation

@oflatt-claude

@oflatt-claude oflatt-claude commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Motivation

This came out of investigating why programs with many global let bindings /
large top-level expressions are slow (#836, #756). One of the costs is that
merge_all reset the cached indexes of every table on each operation and
re-summed every table's length — work that grows with the total number of tables.
That is especially pronounced when a program creates many small tables (e.g. the
term/proof encoding gives each global its own view table), where a run of global
definitions becomes quadratic. This PR removes that per-operation cost.

Change

Database::merge_all reset the cached column/key indexes of every table and
re-summed every table's length on each call, regardless of which tables actually
changed — O(all tables) per call.

The full reset is unnecessary: an unmodified table's version is unchanged, so
refreshing its cached index would be a no-op anyway. This tracks the tables
actually modified during the call — the union of every notification_list.reset()
batch, accumulated in merge_all and merge_simple — and resets only those. It
also maintains total_size_estimate incrementally at each merge (mirroring
merge_table) instead of re-summing all tables.

Correctness

touched must contain every table whose version bumps during the call:
ResettableOnceLock::get_or_update runs an index refresh only after a reset(),
so a modified-but-unreset table would keep serving a stale cached index. Every
merged table is drawn from notification_list.reset(), which is exactly what
touched accumulates.

Testing

egglog-core-relations unit tests (52) and the full egglog test suite (792
lib + integration) pass unchanged.

🤖 Generated with Claude Code

@oflatt-claude
oflatt-claude requested a review from a team as a code owner July 20, 2026 02:56
@oflatt-claude
oflatt-claude requested review from yihozhang and removed request for a team July 20, 2026 02:56
`Database::merge_all` reset the cached column/key indexes of *every* table and
re-summed every table's length on each call, regardless of which tables actually
changed. That is O(all tables) per call, and quadratic when many small tables
each trigger a merge (e.g. workloads that create many small functions and run
short rule sets between them).

The full reset is unnecessary: an unmodified table's version is unchanged, so
its cached index would be a no-op to refresh anyway. Track the tables actually
modified during the call — the union of every `notification_list.reset()` batch,
accumulated here and in `merge_simple` — and reset only those. `total_size_estimate`
is maintained incrementally at each merge (mirroring `merge_table`) instead of
re-summed.

Correctness rests on `touched` containing every table whose version bumps this
call: `ResettableOnceLock::get_or_update` runs an index refresh only after a
`reset()`, so a modified-but-unreset table would serve a stale cached index.
Every merged table is drawn from `notification_list.reset()`, which is exactly
what `touched` accumulates. `egglog-core-relations` tests (52) and the full
`egglog` test suite pass unchanged.
@oflatt-claude
oflatt-claude force-pushed the merge-all-reset-dirty branch from 94ae44d to ee3a865 Compare July 20, 2026 02:59
@codecov-commenter

codecov-commenter commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.60%. Comparing base (30eedcc) to head (41e2e89).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
core-relations/src/free_join/mod.rs 96.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
+ Coverage   86.29%   86.60%   +0.31%     
==========================================
  Files          95       95              
  Lines       29353    29615     +262     
==========================================
+ Hits        25330    25649     +319     
+ Misses       4023     3966      -57     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 27.68%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 35 untouched benchmarks
⏩ 227 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation rust_rule_insert_loop[ops1000_funcs2000] 892.3 µs 621.4 µs +43.59%
Simulation tests[luminal-llama] 504.6 ms 444.5 ms +13.53%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing oflatt-claude:merge-all-reset-dirty (af91b80) with main (6575930)

Open in CodSpeed

Footnotes

  1. 227 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@oflatt
oflatt marked this pull request as ready for review July 24, 2026 23:52
Copilot AI review requested due to automatic review settings July 24, 2026 23:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes core-relations merge processing by eliminating per-merge_all work that scaled with the total number of tables. Instead of resetting cached indexes and recomputing total table size for all tables on each call, it tracks which tables were actually modified and performs resets/size updates only for those.

Changes:

  • Track “touched” tables during Database::merge_all/merge_simple and reset cached indexes only for those tables.
  • Maintain total_size_estimate incrementally during merges (mirroring merge_table) instead of re-summing all table lengths at the end.
  • Thread the touched-table set through the merge_simple fixed-point loop so newly-notified tables are also captured.
Comments suppressed due to low confidence (1)

core-relations/src/free_join/mod.rs:606

  • This reset-loop comment restates implementation details (including internal types) and repeats earlier rationale. It would be easier to maintain if it only stated the required invariant: reset cached indexes for the tables merged this call so they refresh on next access, and touched must include every merged table.
        // Reset the cached indexes of only the tables modified during this call so
        // they refresh on next access; unmodified tables keep their still-valid
        // cached indexes. `touched` must contain *every* table whose version bumped
        // this call: `ResettableOnceLock::get_or_update` runs the index `refresh`
        // only after a `reset()`, so a modified-but-unreset table would keep serving

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core-relations/src/free_join/mod.rs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants