From 8bdf66f36e7365d00a3ef29ced789ee4de2d7d27 Mon Sep 17 00:00:00 2001 From: Eli Dowling Date: Wed, 20 May 2026 18:22:17 +0200 Subject: [PATCH] fix(graph): avoid eager graph collapse blowups\n\nMemoize hidden-ancestor expansion and switch full/collapsed node construction to lazy memoization so shared merge ancestry is only built once. Also avoid eagerly materializing the full DAG on collapsed startup paths. Add timing logs around the post-parse graph stages to isolate future regressions, and add a regression test for hidden merge parent deduplication. --- AGENTS.md | 12 +- README.md | 1 + jj_tui/bin/graph_view.ml | 13 +- jj_tui/lib/jj_json.ml | 236 ++++++++++++++++++++------------- jj_tui/lib/jj_json_tests.ml | 31 ++++- jj_tui/lib/process_wrappers.ml | 10 +- 6 files changed, 193 insertions(+), 110 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d5561f9..c435909 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,22 +30,22 @@ jj_tui/ nix develop # Build -dune build +dune build --pkg disabled # Build and watch -dune build --watch +dune build --pkg disabled --watch # Run the application -dune exec jj_tui +dune exec --pkg disabled jj_tui # Run all tests -dune runtest +dune runtest --pkg disabled # Run tests for specific library -dune runtest -p jj_tui +dune runtest --pkg disabled -p jj_tui # Run tests and show output -dune runtest --force +dune runtest --pkg disabled --force # Format code dune fmt diff --git a/README.md b/README.md index 4d102e4..273022e 100644 --- a/README.md +++ b/README.md @@ -81,4 +81,5 @@ For a full list of commands ids see [`jj_tui/bin/graph_commands.ml`](jj_tui/bin/ # Dev Can be built with nix `nix build` or open a nix shell with `nix develop` +NOTE: if you are using the nix dev shell and would like to build with dune use `dune build --pkg disabled` to build using the deps provided by nix Can also be built directly with Dune package management via `dune build`. diff --git a/jj_tui/bin/graph_view.ml b/jj_tui/bin/graph_view.ml index fb069bf..7ac6245 100644 --- a/jj_tui/bin/graph_view.ml +++ b/jj_tui/bin/graph_view.ml @@ -120,12 +120,13 @@ module Make (Vars : Global_vars.Vars) = struct |> Array.of_list else rev_ids in - let rendered_rows = - Render_jj_graph.render_nodes_structured - state - nodes - ~node_attr:Commit_render.graph_node_attr - in + let rendered_rows = + Jj_tui.Logging.timeStampLog "rendered graph rows" @@ fun () -> + Render_jj_graph.render_nodes_structured + state + nodes + ~node_attr:Commit_render.graph_node_attr + in error_var $= None; rendered_rows, rev_ids with diff --git a/jj_tui/lib/jj_json.ml b/jj_tui/lib/jj_json.ml index 389ab28..95512b5 100644 --- a/jj_tui/lib/jj_json.ml +++ b/jj_tui/lib/jj_json.ml @@ -40,6 +40,20 @@ type jj_commit = { } [@@deriving yojson] +(** Preserve parent order while preventing merge-heavy hidden ancestry from + duplicating the same visible parent once per path through the DAG. *) +let dedupe_preserve_order items = + let seen = Hashtbl.create (List.length items) in + List.filter + (fun item -> + if Hashtbl.mem seen item + then false + else ( + Hashtbl.add seen item (); + true)) + items +;; + (** The jj template that produces JSONL output *) let json_log_template = {|'{' @@ -153,7 +167,7 @@ let commits_to_nodes let commit_tbl : (string, jj_commit) Hashtbl.t = Hashtbl.create (List.length commits) in commits |> List.iter (fun jj_commit -> Hashtbl.replace commit_tbl jj_commit.commit_id jj_commit); - let full_node_tbl : (string, Render_jj_graph.node) Hashtbl.t = + let full_node_tbl : (string, Render_jj_graph.node Lazy.t) Hashtbl.t = Hashtbl.create (List.length commits) in let anonymous_elided_tbl : (string, Render_jj_graph.node) Hashtbl.t = @@ -162,49 +176,55 @@ let commits_to_nodes let rec build_full_node commit_id = match Hashtbl.find_opt full_node_tbl commit_id with | Some node -> - node + Lazy.force node | None -> - (match Hashtbl.find_opt commit_tbl commit_id with - | None -> - (match Hashtbl.find_opt anonymous_elided_tbl commit_id with - | Some elided -> - elided - | None -> - let elided = Render_jj_graph.make_elided_node ~id:commit_id () in - Hashtbl.add anonymous_elided_tbl commit_id elided; - elided) - | Some jj_commit -> - let parents = List.map build_full_node jj_commit.parents in - let node : Render_jj_graph.node = - { - parents - ; creation_time = Int64.of_int 0 - ; working_copy = jj_commit.working_copy - ; immutable = jj_commit.immutable - ; wip = jj_commit.wip - ; change_id = jj_commit.change_id - ; commit_id = jj_commit.commit_id - ; description = jj_commit.description - ; bookmarks = display_refs jj_commit - ; author_email = jj_commit.author.email - ; author_timestamp = jj_commit.author.timestamp - ; empty = jj_commit.empty - ; hidden = jj_commit.hidden - ; divergent = jj_commit.divergent - ; conflict = jj_commit.conflict - ; is_preview = false - ; change_id_prefix = jj_commit.change_id_prefix - ; change_id_rest = jj_commit.change_id_rest - ; commit_id_prefix = jj_commit.commit_id_prefix - ; commit_id_rest = jj_commit.commit_id_rest - } - in - Hashtbl.add full_node_tbl commit_id node; - node) + let node = + lazy + (match Hashtbl.find_opt commit_tbl commit_id with + | None -> + (match Hashtbl.find_opt anonymous_elided_tbl commit_id with + | Some elided -> + elided + | None -> + let elided = Render_jj_graph.make_elided_node ~id:commit_id () in + Hashtbl.add anonymous_elided_tbl commit_id elided; + elided) + | Some jj_commit -> + let parents = List.map build_full_node jj_commit.parents in + let node : Render_jj_graph.node = + { + parents + ; creation_time = Int64.of_int 0 + ; working_copy = jj_commit.working_copy + ; immutable = jj_commit.immutable + ; wip = jj_commit.wip + ; change_id = jj_commit.change_id + ; commit_id = jj_commit.commit_id + ; description = jj_commit.description + ; bookmarks = display_refs jj_commit + ; author_email = jj_commit.author.email + ; author_timestamp = jj_commit.author.timestamp + ; empty = jj_commit.empty + ; hidden = jj_commit.hidden + ; divergent = jj_commit.divergent + ; conflict = jj_commit.conflict + ; is_preview = false + ; change_id_prefix = jj_commit.change_id_prefix + ; change_id_rest = jj_commit.change_id_rest + ; commit_id_prefix = jj_commit.commit_id_prefix + ; commit_id_rest = jj_commit.commit_id_rest + } + in + node) + in + Hashtbl.add full_node_tbl commit_id node; + Lazy.force node in - List.iter (fun jj_commit -> ignore (build_full_node jj_commit.commit_id)) commits; if not collapse_hidden_ancestry then ( + (* The non-collapsed path returns the original commit DAG, so materialize the full + node table eagerly and reuse those shared node objects in output order. *) + List.iter (fun jj_commit -> ignore (build_full_node jj_commit.commit_id)) commits; let visible_commit_ids = match visible_commit_ids with | Some ids -> @@ -213,8 +233,12 @@ let commits_to_nodes commits |> List.map (fun commit -> commit.commit_id) in visible_commit_ids - |> List.filter_map (fun commit_id -> Hashtbl.find_opt full_node_tbl commit_id)) + |> List.filter_map (fun commit_id -> + Hashtbl.find_opt full_node_tbl commit_id |> Option.map Lazy.force)) else ( + (* The collapsed path only needs visible nodes plus rare missing-parent placeholders. + Building the full DAG eagerly here does a large amount of work up front for big + repos while providing little value, so keep full-node construction lazy. *) let visible_commit_ids = match visible_commit_ids with | Some ids -> @@ -225,83 +249,105 @@ let commits_to_nodes let visible_set = visible_commit_ids |> List.to_seq |> Seq.map (fun id -> id, ()) |> Hashtbl.of_seq in + (* Hidden ancestry is shared heavily in large repos. Cache the nearest visible + parents for each hidden node so collapsed graph construction does not walk the + same ancestry over and over for every visible child. *) + let visible_parent_ids_cache : (string, string list) Hashtbl.t = + Hashtbl.create (List.length commits) + in let rec visible_parent_ids commit_id = - match Hashtbl.find_opt commit_tbl commit_id with + match Hashtbl.find_opt visible_parent_ids_cache commit_id with + | Some parent_ids -> + parent_ids | None -> - [ commit_id ] - | Some jj_commit -> - jj_commit.parents - |> List.concat_map (fun parent_id -> - if Hashtbl.mem visible_set parent_id || not (Hashtbl.mem commit_tbl parent_id) - then [ parent_id ] - else visible_parent_ids parent_id) + let parent_ids = + match Hashtbl.find_opt commit_tbl commit_id with + | None -> + [ commit_id ] + | Some jj_commit -> + jj_commit.parents + |> List.concat_map (fun parent_id -> + if Hashtbl.mem visible_set parent_id || not (Hashtbl.mem commit_tbl parent_id) + then [ parent_id ] + else visible_parent_ids parent_id) + |> dedupe_preserve_order + in + Hashtbl.replace visible_parent_ids_cache commit_id parent_ids; + parent_ids in - let collapsed_node_tbl : (string, Render_jj_graph.node) Hashtbl.t = + let collapsed_node_tbl : (string, Render_jj_graph.node Lazy.t) Hashtbl.t = Hashtbl.create (List.length visible_commit_ids * 2) in let rec build_collapsed_node node_id = match Hashtbl.find_opt collapsed_node_tbl node_id with | Some node -> - node + Lazy.force node | None -> if Render_jj_graph.is_elided_id node_id then failwith "build_collapsed_node should not be called directly for elided ids" else ( - let jj_commit = Hashtbl.find commit_tbl node_id in - let parents = - jj_commit.parents - |> List.concat_map (fun parent_id -> - if Hashtbl.mem visible_set parent_id && Hashtbl.mem commit_tbl parent_id - then [ build_collapsed_node parent_id ] - else if - Hashtbl.mem commit_tbl parent_id || not (Hashtbl.mem commit_tbl parent_id) - then [ build_elided_node ~child_id:node_id ~hidden_parent_id:parent_id ] - else []) - in - let node : Render_jj_graph.node = - { - parents - ; creation_time = Int64.of_int 0 - ; working_copy = jj_commit.working_copy - ; immutable = jj_commit.immutable - ; wip = jj_commit.wip - ; change_id = jj_commit.change_id - ; commit_id = jj_commit.commit_id - ; description = jj_commit.description - ; bookmarks = display_refs jj_commit - ; author_email = jj_commit.author.email - ; author_timestamp = jj_commit.author.timestamp - ; empty = jj_commit.empty - ; hidden = jj_commit.hidden - ; divergent = jj_commit.divergent - ; conflict = jj_commit.conflict - ; is_preview = false - ; change_id_prefix = jj_commit.change_id_prefix - ; change_id_rest = jj_commit.change_id_rest - ; commit_id_prefix = jj_commit.commit_id_prefix - ; commit_id_rest = jj_commit.commit_id_rest - } + let node = + lazy + (let jj_commit = Hashtbl.find commit_tbl node_id in + let parents = + jj_commit.parents + |> List.concat_map (fun parent_id -> + if Hashtbl.mem visible_set parent_id && Hashtbl.mem commit_tbl parent_id + then [ build_collapsed_node parent_id ] + else if + Hashtbl.mem commit_tbl parent_id || not (Hashtbl.mem commit_tbl parent_id) + then [ build_elided_node ~child_id:node_id ~hidden_parent_id:parent_id ] + else []) + in + let node : Render_jj_graph.node = + { + parents + ; creation_time = Int64.of_int 0 + ; working_copy = jj_commit.working_copy + ; immutable = jj_commit.immutable + ; wip = jj_commit.wip + ; change_id = jj_commit.change_id + ; commit_id = jj_commit.commit_id + ; description = jj_commit.description + ; bookmarks = display_refs jj_commit + ; author_email = jj_commit.author.email + ; author_timestamp = jj_commit.author.timestamp + ; empty = jj_commit.empty + ; hidden = jj_commit.hidden + ; divergent = jj_commit.divergent + ; conflict = jj_commit.conflict + ; is_preview = false + ; change_id_prefix = jj_commit.change_id_prefix + ; change_id_rest = jj_commit.change_id_rest + ; commit_id_prefix = jj_commit.commit_id_prefix + ; commit_id_rest = jj_commit.commit_id_rest + } + in + node) in Hashtbl.add collapsed_node_tbl node_id node; - node) + Lazy.force node) and build_elided_node ~child_id ~hidden_parent_id = let elided_id = Printf.sprintf "%s:%s:%s" Render_jj_graph.elided_marker child_id hidden_parent_id in match Hashtbl.find_opt collapsed_node_tbl elided_id with | Some node -> - node + Lazy.force node | None -> - let parents = - visible_parent_ids hidden_parent_id - |> List.map (fun parent_id -> - if Hashtbl.mem commit_tbl parent_id && Hashtbl.mem visible_set parent_id - then build_collapsed_node parent_id - else build_full_node parent_id) + let elided = + lazy + (let parents = + visible_parent_ids hidden_parent_id + |> List.map (fun parent_id -> + if Hashtbl.mem commit_tbl parent_id && Hashtbl.mem visible_set parent_id + then build_collapsed_node parent_id + else build_full_node parent_id) + in + Render_jj_graph.make_elided_node ~id:elided_id ~parents ()) in - let elided = Render_jj_graph.make_elided_node ~id:elided_id ~parents () in Hashtbl.add collapsed_node_tbl elided_id elided; - elided + Lazy.force elided in let emitted = Hashtbl.create (List.length visible_commit_ids * 2) in commits diff --git a/jj_tui/lib/jj_json_tests.ml b/jj_tui/lib/jj_json_tests.ml index cbf2784..69df71e 100644 --- a/jj_tui/lib/jj_json_tests.ml +++ b/jj_tui/lib/jj_json_tests.ml @@ -343,6 +343,35 @@ let%expect_test "commits_to_nodes_prefers_local_refs_and_appends_tags" = [%expect {| Visible refs: [main*;v0.18] - Hidden refs: [main@origin] + Hidden refs: [main@origin] + |}] +;; + +let%expect_test "commits_to_nodes_dedupes_visible_parents_through_hidden_merges" = + let input = + {|{"commit_id":"child","parents":["merge"],"change_id":"c","description":"Child","working_copy":false,"immutable":false,"wip":false,"hidden":false,"divergent":false,"conflict":false,"empty":false,"local_bookmarks":[],"remote_bookmarks":[],"tags":[],"author":{"email":"test@example.com","timestamp":"2024-01-05"},"change_id_prefix":"c","change_id_rest":"","commit_id_prefix":"chi","commit_id_rest":"ld"} +{"commit_id":"merge","parents":["left","right"],"change_id":"m","description":"Hidden merge","working_copy":false,"immutable":true,"wip":false,"hidden":true,"divergent":false,"conflict":false,"empty":false,"local_bookmarks":[],"remote_bookmarks":[],"tags":[],"author":{"email":"test@example.com","timestamp":"2024-01-04"},"change_id_prefix":"m","change_id_rest":"","commit_id_prefix":"mer","commit_id_rest":"ge"} +{"commit_id":"left","parents":["root"],"change_id":"l","description":"Hidden left","working_copy":false,"immutable":true,"wip":false,"hidden":true,"divergent":false,"conflict":false,"empty":false,"local_bookmarks":[],"remote_bookmarks":[],"tags":[],"author":{"email":"test@example.com","timestamp":"2024-01-03"},"change_id_prefix":"l","change_id_rest":"","commit_id_prefix":"lef","commit_id_rest":"t"} +{"commit_id":"right","parents":["root"],"change_id":"r","description":"Hidden right","working_copy":false,"immutable":true,"wip":false,"hidden":true,"divergent":false,"conflict":false,"empty":false,"local_bookmarks":[],"remote_bookmarks":[],"tags":[],"author":{"email":"test@example.com","timestamp":"2024-01-02"},"change_id_prefix":"r","change_id_rest":"","commit_id_prefix":"rig","commit_id_rest":"ht"} +{"commit_id":"root","parents":[],"change_id":"o","description":"Root","working_copy":false,"immutable":true,"wip":false,"hidden":false,"divergent":false,"conflict":false,"empty":false,"local_bookmarks":[],"remote_bookmarks":[],"tags":[],"author":{"email":"test@example.com","timestamp":"2024-01-01"},"change_id_prefix":"o","change_id_rest":"","commit_id_prefix":"roo","commit_id_rest":"t"}|} + in + (match parse_jj_log_output input with + | Ok commits -> + let nodes = commits_to_nodes ~visible_commit_ids:[ "child"; "root" ] commits in + let child = List.nth nodes 0 in + let elided = List.nth nodes 1 in + Printf.printf "Child parents: %d\n" (List.length child.parents); + Printf.printf "Elided parents: %d\n" (List.length elided.parents); + let elided_parent_ids = + elided.parents |> List.map (fun (p : Render_jj_graph.node) -> p.commit_id) + in + Printf.printf "Elided parent commit ids: [%s]\n" (String.concat ";" elided_parent_ids) + | Error msg -> + Printf.printf "Error: %s\n" msg); + [%expect + {| + Child parents: 1 + Elided parents: 1 + Elided parent commit ids: [root] |}] ;; diff --git a/jj_tui/lib/process_wrappers.ml b/jj_tui/lib/process_wrappers.ml index f5b0999..9194a46 100644 --- a/jj_tui/lib/process_wrappers.ml +++ b/jj_tui/lib/process_wrappers.ml @@ -333,8 +333,14 @@ struct (* Default view: fetch the full visible graph topology, then locally decide which commits remain visible and where elision rows must be inserted. *) let commits = get_graph_json ~revset:"all()" limit in - let visible_commit_ids = Jj_json.select_visible_commit_ids commits in - let nodes = Jj_json.commits_to_nodes ~visible_commit_ids commits in + let visible_commit_ids = + timeStampLog "selected visible commit ids" @@ fun () -> + Jj_json.select_visible_commit_ids commits + in + let nodes = + timeStampLog "collapsed commits to graph nodes" @@ fun () -> + Jj_json.commits_to_nodes ~visible_commit_ids commits + in let visible_commit_id_set = visible_commit_ids |> List.to_seq -- 2.51.2