Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: use a different symbol for immutable nodes in log #3234

Closed
wants to merge 1 commit into from
Closed

cli: use a different symbol for immutable nodes in log #3234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,21 +948,7 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
.map(|commit| commit.id().clone())
.collect(),
);
let (params, immutable_heads_str) = self
.revset_aliases_map
.get_function("immutable_heads")
.unwrap();
if !params.is_empty() {
return Err(user_error(
r#"The `revset-aliases.immutable_heads()` function must be declared without arguments."#,
));
}
let immutable_heads_revset = self.parse_revset(immutable_heads_str)?;
let immutable_revset = immutable_heads_revset
.ancestors()
.union(&RevsetExpression::commit(
self.repo().store().root_commit_id().clone(),
));
let immutable_revset = self.immutable_revset()?;
let revset = self.evaluate_revset(to_rewrite_revset.intersection(&immutable_revset))?;
if let Some(commit) = revset.iter().commits(self.repo().store()).next() {
let commit = commit?;
Expand All @@ -984,6 +970,26 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
Ok(())
}

/// Returns revset represented by immutable commits including root.
pub fn immutable_revset(&self) -> Result<Rc<RevsetExpression>, CommandError> {
let (params, immutable_heads_str) = self
.revset_aliases_map
.get_function("immutable_heads")
.unwrap();
if !params.is_empty() {
return Err(user_error(
r#"The `revset-aliases.immutable_heads()` function must be declared without arguments."#,
));
}
let immutable_heads_revset = self.parse_revset(immutable_heads_str)?;
let immutable_revset = immutable_heads_revset
.ancestors()
.union(&RevsetExpression::commit(
self.repo().store().root_commit_id().clone(),
));
Ok(immutable_revset)
}

pub fn check_non_empty(&self, commits: &[Commit]) -> Result<(), CommandError> {
if commits.is_empty() {
return Err(user_error("Empty revision set"));
Expand Down
12 changes: 11 additions & 1 deletion cli/src/commands/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ pub(crate) fn cmd_log(
let mut graph = get_graphlog(command.settings(), formatter.raw());
let default_node_symbol = graph.default_node_symbol().to_owned();
let elided_node_symbol = graph.elided_node_symbol().to_owned();
let immutable_node_symbol = graph.immutable_node_symbol().to_owned();
let immutable_revset = workspace_command.immutable_revset()?;
let forward_iter = TopoGroupedRevsetGraphIterator::new(revset.iter_graph());
let iter: Box<dyn Iterator<Item = _>> = if args.reversed {
Box::new(ReverseRevsetGraphIterator::new(forward_iter))
Expand Down Expand Up @@ -180,7 +182,15 @@ pub(crate) fn cmd_log(
let node_symbol = if Some(&key.0) == wc_commit_id {
"@"
} else {
&default_node_symbol
let commit_revset = RevsetExpression::commit(key.0.clone());
let revset = workspace_command
.evaluate_revset(commit_revset.intersection(&immutable_revset))?;
let is_immutable = revset.iter().commits(store).next().is_some();
if is_immutable {
&immutable_node_symbol
} else {
&default_node_symbol
}
Comment on lines +185 to +193
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit expensive. How much does it slow down jj log -r .. in the jj repo, for example?

We have talked about adding a .contains() method to revsets. The most recent discussion was here:
https://discord.com/channels/968932220549103686/969291218347524238/1213888562790006824

Let me know if you prefer not to log on to Discord and I can paste that text here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's definitely slower:

λ time jj log -r .. > /dev/null

________________________________________________________
Executed in  546.09 millis    fish           external
   usr time  524.18 millis    0.09 millis  524.10 millis
   sys time   27.16 millis    1.40 millis   25.76 millis

versus main:

λ time jj log -r .. > /dev/null

________________________________________________________
Executed in  134.47 millis    fish           external
   usr time  114.93 millis   78.00 micros  114.85 millis
   sys time   24.40 millis  660.00 micros   23.74 millis

Let me know if you prefer not to log on to Discord and I can paste that text here.

Could you please paste the text here? I would greatly appreciate it.

Copy link
Member

Choose a reason for hiding this comment

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

Here you go:

fyi, the main complication is that in order to make it fast, we'll want to be able to ask revset if it contains a given commit. we'd ask the ancestors(immutable_heads()) revset if it contains the commit we're about to render. for the default revset engine (the one bundled by default), we will do that by walking the revset incrementally while caching the set of commits visited. for the revset engine at google, we'll be able to answer it more efficiently (thankfully, because it's not feasible to walk all ancestors). i think we know what we need to do. i just haven't gotten around to it

it'll be someting like let immutable_revset = immutable_revset.caching(); for commit_id in revset_to_draw.iter() { let is_immutable = immutable_revset.contains(&commit_id); ; }

We have the current Revset trait and we would add a caching() with_contains(), something like this:

trait Revset {
  fn iter(&self) -> Box<dyn Iterator<Item = CommitId> + '_>;
  // etc

  //
  fn caching(&self) -> Box<dyn CachingRevsetIterator + '_>;
}


trait CachingRevsetIterator : Iterator<Item = CommitId> {

  fn contains(&self, id: &CommitId) -> bool;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'll look into it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

fn caching(&self) -> Box<dyn CachingRevsetIterator + '_>;

fwiw, we might have to reintroduce the 'index lifetime to enable immutable.contains(commit_id) template keyword. For jj log use case, it wouldn't be needed. + '_ should be good enough.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly! Except that next() would have to first return any items previously peeked at by contains().

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, the source iterator and the cached positions would have to be shared across iterators (via Rc<RefCell<..>> or something)? (I assume the cached wrapper will provide .iter() to start over.)

CachedRevset {
  fn iter(&self) -> Box<dyn Iterator<Item = Commit>>;
  fn contains(&self, &CommitId) -> bool;
}

The IndexPosition returned from the source iterator is sorted, so contains() can do binary search over Vec<IndexPosition>.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking it would be a different iterator type rather than a different revset type, but your suggestion is probably better

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be a different iterator type rather than a different revset type

It doesn't have to be, but the cache object would have { visited: HashMap<IndexPosition>, nexts: VecDeque<IndexPosition> } or { visited: Vec<IndexPosition>, next_pos: usize } anyway, so adding separate .iter() wouldn't complicate things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened #3258 with draft implementation of CachedRevset, I invite you to discuss it there :)

};

graph.add_node(
Expand Down
18 changes: 15 additions & 3 deletions cli/src/graphlog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub trait GraphLog<K: Clone + Eq + Hash> {

fn elided_node_symbol(&self) -> &str;

fn immutable_node_symbol(&self) -> &str;

fn width(&self, id: &K, edges: &[Edge<K>]) -> usize;
}

Expand All @@ -49,6 +51,7 @@ pub struct SaplingGraphLog<'writer, R> {
writer: &'writer mut dyn Write,
default_node_symbol: String,
elided_node_symbol: String,
immutable_node_symbol: String,
}

impl<K: Clone> From<&Edge<K>> for Ancestor<K> {
Expand Down Expand Up @@ -91,6 +94,10 @@ where
&self.elided_node_symbol
}

fn immutable_node_symbol(&self) -> &str {
&self.immutable_node_symbol
}

fn width(&self, id: &K, edges: &[Edge<K>]) -> usize {
let parents = edges.iter().map_into().collect();
let w: u64 = self.renderer.width(Some(id), Some(&parents));
Expand All @@ -104,6 +111,7 @@ impl<'writer, R> SaplingGraphLog<'writer, R> {
formatter: &'writer mut dyn Write,
default_node_symbol: &str,
elided_node_symbol: &str,
immutable_node_symbol: &str,
) -> Box<dyn GraphLog<K> + 'writer>
where
K: Clone + Eq + Hash + 'writer,
Expand All @@ -114,6 +122,7 @@ impl<'writer, R> SaplingGraphLog<'writer, R> {
writer: formatter,
default_node_symbol: default_node_symbol.to_owned(),
elided_node_symbol: elided_node_symbol.to_owned(),
immutable_node_symbol: immutable_node_symbol.to_owned(),
})
}
}
Expand All @@ -130,10 +139,13 @@ pub fn get_graphlog<'a, K: Clone + Eq + Hash + 'a>(
formatter,
"◉",
"◌",
"◈",
),
"ascii" => SaplingGraphLog::create(builder.build_ascii(), formatter, "o", "."),
"ascii-large" => SaplingGraphLog::create(builder.build_ascii_large(), formatter, "o", "."),
"ascii" => SaplingGraphLog::create(builder.build_ascii(), formatter, "o", ".", "x"),
"ascii-large" => {
SaplingGraphLog::create(builder.build_ascii_large(), formatter, "o", ".", "x")
}
// "curved"
_ => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉", "◌"),
_ => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉", "◌", "◈"),
}
}
Loading