Skip to content

Cap BFS visits in compute_hidden_forks to prevent hangs#29

Open
eljaska wants to merge 2 commits into
masterfrom
eljaska/fix-hidden-forks-hang
Open

Cap BFS visits in compute_hidden_forks to prevent hangs#29
eljaska wants to merge 2 commits into
masterfrom
eljaska/fix-hidden-forks-hang

Conversation

@eljaska
Copy link
Copy Markdown
Member

@eljaska eljaska commented May 9, 2026

Summary

  • Adds a benchmark test (query_bench) that measures evaluate_revset_str, compute_hidden_forks, and get_page across four revset types on any jj repo. The test is #[ignore] and run manually via BENCH_REPO=/path/to/repo cargo test query_bench -- --ignored --nocapture.
  • Caps the total BFS visits in compute_hidden_forks at 100,000 to prevent unbounded ancestor traversal when most bookmarks are outside the log revset.

Context

PR #22 replaced the O(B × n) per-bookmark evaluate_revset_expr call with a BFS walk per out-of-log bookmark. The PR assumed "cost is O(depth_to_log) per bookmark, which is tiny in practice since bookmarks are usually a few commits off the log boundary." This holds when the log revset is large (e.g. all() or ::@), since most bookmarks are already in-log or close to it. But with small log revsets like default or mine, nearly all ~4,000 bookmarks of stargate are out-of-log, and each BFS walks thousands of commits before finding a log ancestor — or never finds one. With no bound on total work, this causes an indefinite hang on startup for any repo with many bookmarks and a small default revset.

Before (on master)

revset         eval      forks       page      total  rows
----------------------------------------------------------------------
all()           8ms       36ms       59ms      104ms  50
::@             0ms      484ms       16ms      501ms  50
default         —     TIMEOUT (>30s)
mine            —     never reached

After (with BFS cap at 100k visits)

revset         eval      forks       page      total  rows
----------------------------------------------------------------------
all()           9ms       40ms       66ms      116ms  50
::@             0ms      519ms       18ms      538ms  50
default        34ms     3359ms       20ms     3414ms  35
mine            0ms     3395ms       15ms     3411ms  4

Test plan

  • BENCH_REPO=/path/to/repo cargo test query_bench -- --ignored --nocapture completes for all four revsets
  • cargo test (non-ignored tests) still passes
  • Verified the jjuicy desktop app loads the stargate repo without hanging

🤖 Generated with Claude Code

@eljaska eljaska force-pushed the eljaska/fix-hidden-forks-hang branch from c6a71ac to 77c4e3c Compare May 9, 2026 02:00
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.

1 participant