Add tsort#359
Conversation
There was a problem hiding this comment.
This could be let n = start_nodes.shift().unwrap();. You wouldn't need the next line in this case.
There was a problem hiding this comment.
The regression I said in below (refactoring looping n_out_edges) was found caused by this change.
diff --git a/tsort/tsort.rs b/tsort/tsort.rs
index 1a514a9..b0c6283 100644
--- a/tsort/tsort.rs
+++ b/tsort/tsort.rs
@@ -154,7 +154,7 @@ impl Graph {
}
while !start_nodes.is_empty() {
- let n = start_nodes[0].clone();
+ let n = start_nodes.shift().unwrap();
start_nodes.remove(0);
self.result.push(n.clone());There was a problem hiding this comment.
Oh. You need to remove start_nodes.remove(0) below. Otherwise, it will try to remove the node twice.
There was a problem hiding this comment.
The double removing removed and it came back to life. Thanks.
|
Everything else seems fine. The performance issue can be worked out later. 👍 |
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
|
All proposals applied. No regression found and performance really improved. I think refactoring that inner loop did it. I will submit another pull request |
There was a problem hiding this comment.
This could be written as
print!("{name} v{version}
Usage:
{name} [OPTIONS] FILE
{usage}",
name = NAME,
vers = VERSION,
usage = getopts::usage(...));There was a problem hiding this comment.
I'll probably refactor all of the utilities to use this at some point.
|
Yay! 😄 Yes, it's faster because of the inner loop. In case you're interested, the reason for this is the loop no longer has bounds checks, and the elements in the |
|
I assume you want me to merge the other pull request, so I can close this one, correct? |
Yes. Please close and merge #361 |
3 participants
Signed-off-by: Akira Hayakawa ruby.wktk@gmail.com
Another test project is here
https://github.com/akiradeveloper/rust-tsort
Testing
It generates random graph and compares both outputs of GNU-version and Rust-version. I think Rust-version works correctly.
Performance
The current version doesn't perform well for especially large scale dataset.
Here is a experimental result from the above test project.
The test is first create DAG of N nodes and N(N-1)/2 edges and then feed the dataset into both versions. Note, t_new is of Rust-version and t_ref is of GNU-version.
N increases from top to bottom as logarithmic scale (10, 20, 50, 100, 200, 500, 1000, 2000, 5000). When the N is small. Rust-version is not bad however, it loses more the higher the N is. At the last case, it's 10 times worse.
The algorithm is O(|V|+|E|) which is O(|V|^2) in this case. I will improve the algorithm if I find better one.