perf(dag): select visible commits more efficiently

As reported in #625, commit c90b52f introduced a performance regression. It's not clear to me why queries like `ancestors(X)` are slow in this case. In my experimentation, the result was often a single-span commit set, which should be fairly quick to compute and process for other operations (such as `intersection`). Nonetheless, avoiding that query seems to improve the speed of evaluating various revsets.

git-branchless-lib/src/core/dag.rs

@@ -114,6 +114,7 @@ pub struct Dag {
     obsolete_commits: CommitSet,
 
     public_commits: OnceCell<CommitSet>,
+    visible_heads: OnceCell<CommitSet>,
     visible_commits: OnceCell<CommitSet>,
     draft_commits: OnceCell<CommitSet>,
     default_smartlog_commits: OnceCell<CommitSet>,
@@ -194,6 +195,7 @@ impl Dag {
             observed_commits,
             obsolete_commits,
             public_commits: Default::default(),
+            visible_heads: Default::default(),
             visible_commits: Default::default(),
             draft_commits: Default::default(),
             default_smartlog_commits: Default::default(),
@@ -371,8 +373,27 @@ impl Dag {
         self.inner.borrow()
     }
 
-    /// Return the set of commits which are public (checked into the main branch).
-    pub fn query_public_commits(&self) -> eyre::Result<&CommitSet> {
+    /// Determine whether or not the given commit is a public commit (i.e. is an
+    /// ancestor of the main branch).
+    #[instrument]
+    pub fn is_public_commit(&self, commit_oid: NonZeroOid) -> eyre::Result<bool> {
+        let main_branch_commits = commit_set_to_vec(&self.main_branch_commit)?;
+        for main_branch_commit in main_branch_commits {
+            if self
+                .inner
+                .is_ancestor(commit_oid.into(), main_branch_commit.into())?
+            {
+                return Ok(true);
+            }
+        }
+        Ok(false)
+    }
+
+    /// Return the set of commits which are public, as per the definition in
+    /// `is_public_commit`. You should try to use `is_public_commit` instead, as
+    /// it will be faster to compute.
+    #[instrument]
+    pub fn query_public_commits_slow(&self) -> eyre::Result<&CommitSet> {
         self.public_commits.get_or_try_init(|| {
             let public_commits = self.query().ancestors(self.main_branch_commit.clone())?;
             Ok(public_commits)
@@ -382,19 +403,39 @@ impl Dag {
     /// Determine the set of commits which are considered to be "visible". A
     /// commit is "visible" if it is not obsolete or has a non-obsolete
     /// descendant.
-    pub fn query_visible_commits(&self) -> eyre::Result<&CommitSet> {
-        self.visible_commits.get_or_try_init(|| {
-            let public_commits = self.query_public_commits()?;
-            let visible_heads = public_commits
+    #[instrument]
+    pub fn query_visible_heads(&self) -> eyre::Result<&CommitSet> {
+        self.visible_heads.get_or_try_init(|| {
+            let visible_heads = CommitSet::empty()
                 .union(&self.observed_commits.difference(&self.obsolete_commits))
                 .union(&self.head_commit)
                 .union(&self.main_branch_commit)
                 .union(&self.branch_commits);
-            let visible_commits = self.query().ancestors(visible_heads)?;
-            Ok(visible_commits)
+            let visible_heads = self.query().heads(visible_heads)?;
+            Ok(visible_heads)
         })
     }
 
+    /// Query the set of all visible commits, as per the definition in
+    /// `query_visible_head`s. You should try to use `query_visible_heads`
+    /// instead if possible, since it will be faster to compute.
+    #[instrument]
+    pub fn query_visible_commits_slow(&self) -> eyre::Result<&CommitSet> {
+        self.visible_commits.get_or_try_init(|| {
+            let visible_heads = self.query_visible_heads()?;
+            let result = self.inner.ancestors(visible_heads.clone())?;
+            Ok(result)
+        })
+    }
+
+    /// Keep only commits in the given set which are visible, as per the
+    /// definition in `query_visible_heads`.
+    #[instrument]
+    pub fn filter_visible_commits(&self, commits: CommitSet) -> eyre::Result<CommitSet> {
+        let visible_heads = self.query_visible_heads()?;
+        Ok(commits.intersection(&self.query().range(commits.clone(), visible_heads.clone())?))
+    }
+
     /// Determine the set of obsolete commits. These commits have been rewritten
     /// or explicitly hidden by the user.
     #[instrument]
@@ -407,17 +448,19 @@ impl Dag {
     #[instrument]
     pub fn query_draft_commits(&self) -> eyre::Result<&CommitSet> {
         self.draft_commits.get_or_try_init(|| {
-            let visible_commits = self.query_visible_commits()?;
-            let public_commits = self.query_public_commits()?;
-            Ok(visible_commits.difference(public_commits))
+            let visible_heads = self.query_visible_heads()?;
+            let draft_commits = self
+                .query()
+                .only(visible_heads.clone(), self.main_branch_commit.clone())?;
+            Ok(draft_commits)
         })
     }
 
     /// Determine the default set of commits that is shown in the smartlog when
     /// no revset is passed.
     pub fn query_default_smartlog_commits(&self) -> eyre::Result<&CommitSet> {
         self.default_smartlog_commits.get_or_try_init(|| {
-            let public_commits = self.query_public_commits()?;
+            let public_commits = self.query_public_commits_slow()?;
             let active_commits = self.observed_commits.clone();
             let active_heads = self.query().heads(active_commits)?;
             let active_heads = active_heads.difference(public_commits);

git-branchless-lib/src/core/rewrite/evolve.rs

@@ -82,9 +82,8 @@ pub fn find_abandoned_children(
         Some(MaybeZeroOid::Zero) => oid,
         None => return Ok(None),
     };
-
     let children = dag.query().children(CommitSet::from(oid))?;
-    let children = children.intersection(dag.query_visible_commits()?);
+    let children = dag.filter_visible_commits(children)?;
     let non_obsolete_children = children.difference(&dag.query_obsolete_commits());
     let non_obsolete_children_oids: Vec<NonZeroOid> = non_obsolete_children
         .iter()?

git-branchless-lib/src/core/rewrite/plan.rs

@@ -192,7 +192,7 @@ impl<'a> RebasePlanPermissions<'a> {
         // error message which indicates the magnitude of the issue.
         let commits = dag.query().descendants(commits.clone())?;
 
-        let public_commits = dag.query_public_commits()?;
+        let public_commits = dag.query_public_commits_slow()?;
         if !build_options.force_rewrite_public_commits {
             let public_commits_to_move = public_commits.intersection(&commits);
             if !public_commits_to_move.is_empty()? {
@@ -303,8 +303,8 @@ impl<'a> ConstraintGraph<'a> {
                         .dag
                         .query()
                         .children(CommitSet::from(*children_of_oid))?
-                        .difference(&commits_to_move)
-                        .intersection(self.dag.query_visible_commits()?);
+                        .difference(&commits_to_move);
+                    let source_children = self.dag.filter_visible_commits(source_children)?;
 
                     for child_oid in commit_set_to_vec(&source_children)? {
                         self.inner.entry(parent_oid).or_default().insert(child_oid);
@@ -358,7 +358,7 @@ impl<'a> ConstraintGraph<'a> {
         let _effects = effects;
 
         let all_descendants_of_constrained_nodes = {
-            let visible_commits = self.dag.query_visible_commits()?;
+            let visible_commits = self.dag.query_visible_commits_slow()?;
 
             let mut acc = Vec::new();
             let parents = self.commits_to_move();

git-branchless/src/commands/hide.rs

@@ -58,9 +58,7 @@ pub fn hide(
 
     let commits = union_all(&commit_sets);
     let commits = if recursive {
-        dag.query()
-            .descendants(commits)?
-            .intersection(dag.query_visible_commits()?)
+        dag.filter_visible_commits(dag.query().descendants(commits)?)?
     } else {
         commits
     };

git-branchless/src/commands/move.rs

@@ -377,8 +377,8 @@ pub fn r#move(
             let component_children: CommitSet = dag
                 .query()
                 .children(component.clone())?
-                .difference(component)
-                .intersection(dag.query_visible_commits()?);
+                .difference(component);
+            let component_children = dag.filter_visible_commits(component_children)?;
 
             for component_child in commit_set_to_vec(&component_children)? {
                 // If the range being extracted has any child commits, then we
@@ -457,8 +457,8 @@ pub fn r#move(
                 .query()
                 .children(CommitSet::from(dest_oid))?
                 .difference(&source_oids)
-                .difference(&exact_oids)
-                .intersection(dag.query_visible_commits()?);
+                .difference(&exact_oids);
+            let dest_children = dag.filter_visible_commits(dest_children)?;
 
             for dest_child in commit_set_to_vec(&dest_children)? {
                 builder.move_subtree(dest_child, vec![source_head])?;

git-branchless/src/commands/navigation.rs

@@ -104,10 +104,8 @@ fn advance(
         let candidate_commits = match command {
             Command::Next => {
                 let child_commits = || -> eyre::Result<CommitSet> {
-                    let result = dag
-                        .query()
-                        .children(CommitSet::from(current_oid))?
-                        .intersection(dag.query_visible_commits()?);
+                    let result = dag.query().children(CommitSet::from(current_oid))?;
+                    let result = dag.filter_visible_commits(result)?;
                     Ok(result)
                 };
 

git-branchless/src/commands/smartlog.rs

@@ -122,7 +122,6 @@ mod graph {
         effects: &Effects,
         repo: &'repo Repo,
         dag: &Dag,
-        public_commits: &CommitSet,
         active_heads: &CommitSet,
     ) -> eyre::Result<SmartlogGraph<'repo>> {
         let mut graph: HashMap<NonZeroOid, Node> = {
@@ -151,7 +150,7 @@ mod graph {
                             object,
                             parent: None,         // populated below
                             children: Vec::new(), // populated below
-                            is_main: public_commits.contains(&oid.into())?,
+                            is_main: dag.is_public_commit(oid)?,
                             is_obsolete: dag.query_obsolete_commits().contains(&oid.into())?,
                         },
                     );
@@ -225,12 +224,11 @@ mod graph {
         let mut graph = {
             let (effects, _progress) = effects.start_operation(OperationType::WalkCommits);
 
-            let public_commits = dag.query_public_commits()?;
             for oid in commit_set_to_vec(commits)? {
                 mark_commit_reachable(repo, oid)?;
             }
 
-            walk_from_commits(&effects, repo, dag, public_commits, commits)?
+            walk_from_commits(&effects, repo, dag, commits)?
         };
         sort_children(&mut graph);
         Ok(graph)

git-branchless/src/revset/builtins.rs

@@ -63,11 +63,16 @@ lazy_static! {
 #[instrument]
 fn fn_all(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult {
     eval0(ctx, name, args)?;
-    Ok(ctx
+    let visible_heads = ctx
         .dag
-        .query_visible_commits()
-        .map_err(EvalError::OtherError)?
-        .clone())
+        .query_visible_heads()
+        .map_err(EvalError::OtherError)?;
+    let visible_commits = ctx.dag.query().ancestors(visible_heads.clone())?;
+    let visible_commits = ctx
+        .dag
+        .filter_visible_commits(visible_commits)
+        .map_err(EvalError::OtherError)?;
+    Ok(visible_commits)
 }
 
 #[instrument]
@@ -109,11 +114,12 @@ fn fn_range(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult {
 #[instrument]
 fn fn_not(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult {
     let expr = eval1(ctx, name, args)?;
-    let active_commits = ctx
+    let visible_heads = ctx
         .dag
-        .query_visible_commits()
+        .query_visible_heads()
         .map_err(EvalError::OtherError)?;
-    Ok(active_commits.difference(&expr))
+    let visible_commits = ctx.dag.query().ancestors(visible_heads.clone())?;
+    Ok(visible_commits.difference(&expr))
 }
 
 #[instrument]
@@ -212,7 +218,7 @@ fn fn_public(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult {
     eval0(ctx, name, args)?;
     let public_commits = ctx
         .dag
-        .query_public_commits()
+        .query_public_commits_slow()
         .map_err(EvalError::OtherError)?;
     Ok(public_commits.clone())
 }

git-branchless/src/revset/eval.rs

@@ -135,14 +135,13 @@ fn eval_inner(ctx: &mut Context, expr: &Expr) -> EvalResult {
         Expr::Name(name) => eval_name(ctx, name),
         Expr::FunctionCall(name, args) => {
             let result = eval_fn(ctx, name, args)?;
-            let visible_commits = if ctx.show_hidden_commits {
-                &result
+            let result = if ctx.show_hidden_commits {
+                result
             } else {
                 ctx.dag
-                    .query_visible_commits()
+                    .filter_visible_commits(result)
                     .map_err(EvalError::OtherError)?
             };
-            let result = result.intersection(visible_commits);
             Ok(result)
         }
     }

git-branchless/src/revset/pattern.rs

@@ -145,7 +145,7 @@ pub(super) fn make_pattern_matcher_set(
         repo: repo.try_clone().map_err(PatternError::Repo)?,
         visible_commits: ctx
             .dag
-            .query_visible_commits()
+            .query_visible_commits_slow()
             .map_err(EvalError::OtherError)
             .map_err(Box::new)?
             .clone(),