Skip to content

Remove extra lookups and memory allocations from tsort graph construction #8694

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

Merged
merged 2 commits into from
Sep 28, 2025
Merged

Remove extra lookups and memory allocations from tsort graph construction #8694

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c862fc0
Remove extra lookups and memory allocations from tsort graph construc…
Nekrolm Sep 21, 2025
9db21a9
Remove extra lookups and memory allocations from tsort graph construc…
Nekrolm Sep 21, 2025
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
39 changes: 20 additions & 19 deletions src/uu/tsort/src/tsort.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,23 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {

// Create the directed graph from pairs of tokens in the input data.
let mut g = Graph::new(input.to_string_lossy().to_string());
for ab in data.split_whitespace().collect::<Vec<&str>>().chunks(2) {
match ab {
[a, b] => g.add_edge(a, b),
_ => return Err(TsortError::NumTokensOdd(input.to_string_lossy().to_string()).into()),
}
// Input is considered to be in the format
// From1 To1 From2 To2 ...
// with tokens separated by whitespaces
let mut edge_tokens = data.split_whitespace();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe document this a bit more to explain what it is doing ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded a bit

// Note: this is equivalent to iterating over edge_tokens.chunks(2)
// but chunks() exists only for slices and would require unnecessary Vec allocation.
// Itertools::chunks() is not used due to unnecessary overhead for internal RefCells
loop {
// Try take next pair of tokens
let Some(from) = edge_tokens.next() else {
// no more tokens -> end of input. Graph constructed
break;
};
let Some(to) = edge_tokens.next() else {
return Err(TsortError::NumTokensOdd(input.to_string_lossy().to_string()).into());
};
g.add_edge(from, to);
}

g.run_tsort();
Expand Down Expand Up @@ -127,19 +139,11 @@ impl<'input> Graph<'input> {
}
}

fn add_node(&mut self, name: &'input str) {
self.nodes.entry(name).or_default();
}

fn add_edge(&mut self, from: &'input str, to: &'input str) {
self.add_node(from);
let from_node = self.nodes.entry(from).or_default();
if from != to {
self.add_node(to);

let from_node = self.nodes.get_mut(from).unwrap();
from_node.add_successor(to);

let to_node = self.nodes.get_mut(to).unwrap();
let to_node = self.nodes.entry(to).or_default();
to_node.predecessor_count += 1;
}
}
Expand Down Expand Up @@ -233,10 +237,7 @@ impl<'input> Graph<'input> {

fn detect_cycle(&self) -> Vec<&'input str> {
// Sort the nodes just to make this function deterministic.
let mut nodes = Vec::new();
for node in self.nodes.keys() {
nodes.push(node);
}
let mut nodes: Vec<_> = self.nodes.keys().collect();
nodes.sort_unstable();

let mut visited = HashSet::new();
Expand Down