From 66f1f40cb31359e77fd8674554ba5a5c06fd2216 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:09:03 -0500 Subject: [PATCH 01/13] Makes astar benchmark run. valid_exit() in the benchmark will receive negative x and y deltas from get_available_exist() which when the point they are examining a loc which is x=0 or y=0 would create a negative search position. This in turn would panic in algorithm2d when trying to coerce them into usize. --- bracket-pathfinding/benches/astar_benchmark.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bracket-pathfinding/benches/astar_benchmark.rs b/bracket-pathfinding/benches/astar_benchmark.rs index df69d6aa..7d13859a 100644 --- a/bracket-pathfinding/benches/astar_benchmark.rs +++ b/bracket-pathfinding/benches/astar_benchmark.rs @@ -63,6 +63,11 @@ impl Map { fn valid_exit(&self, loc: Point, delta: Point) -> Option { let destination = loc + delta; + + if destination.x < 0 || destination.y < 0 { + return None + } + let idx = self.point2d_to_index(destination); if self.in_bounds(destination) && self.tiles[idx] == '.' { Some(idx) From 1bfb8ee0e4c076305cede2566451cfdbbefb7ca4 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:20:40 -0500 Subject: [PATCH 02/13] Only search open_list until first node is found. [Note: I am making a larger PR fixing performance on astar but I will break them up into individual commits so the relative impact can be seen and reasoned with vs one big commit] I noticed a missing 'break' in the original code but then converted it to use find (code generated seemingly has same performance). Here is the output from bench: Benchmarking a_star_test_map: Collecting 100 samples in estimated 8.4233s (10k iterations) Benchmarking a_star_test_map: Analyzing a_star_test_map time: [821.89 us 847.62 us 877.18 us] change: [-10.059% -6.2216% -2.1936%] (p = 0.00 < 0.05) Performance has improved. --- bracket-pathfinding/src/astar.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 3a35c5bb..b747481d 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -113,20 +113,20 @@ impl AStar { true } else { let distance = self.distance_to_end(idx, map); + let f = distance + cost; let s = Node { idx, - f: distance + cost, + f: f, g: cost, h: distance, }; // If a node with the same position as successor is in the open list with a lower f, skip add - let mut should_add = true; - for e in &self.open_list { - if e.f < s.f && e.idx == idx { - should_add = false; - } - } + let mut should_add = self + .open_list + .iter() + .find(|e| e.f < f && e.idx == idx) + .is_none(); // If a node with the same position as successor is in the closed list, with a lower f, skip add if should_add && self.closed_list.contains_key(&idx) && self.closed_list[&idx] < s.f { From d2304fbcda3db52f5f2055246e1607b5594b2b4e Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:27:49 -0500 Subject: [PATCH 03/13] Do not make Node until you know you will use it. This is very minor perfwise but it seems reasonable to not make Node until you know you should_add. --- bracket-pathfinding/src/astar.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index b747481d..4f408040 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -114,12 +114,6 @@ impl AStar { } else { let distance = self.distance_to_end(idx, map); let f = distance + cost; - let s = Node { - idx, - f: f, - g: cost, - h: distance, - }; // If a node with the same position as successor is in the open list with a lower f, skip add let mut should_add = self @@ -129,11 +123,17 @@ impl AStar { .is_none(); // If a node with the same position as successor is in the closed list, with a lower f, skip add - if should_add && self.closed_list.contains_key(&idx) && self.closed_list[&idx] < s.f { + if should_add && self.closed_list.contains_key(&idx) && self.closed_list[&idx] < f { should_add = false; } if should_add { + let s = Node { + idx, + f: f, + g: cost, + h: distance, + }; self.open_list.push(s); self.parents.insert(idx, q.idx); } From 9aebe5cb20473694ddd69298d3c68419b4ae1ca9 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:30:49 -0500 Subject: [PATCH 04/13] Do not double hash activity in closed_list. Reduces checking to see if an element exists with a second retrieval with a single get. Get will return None if it does not exist so in essence this halves hashing/bounds checks. Benchmarking a_star_test_map: Analyzing a_star_test_map time: [733.21 us 748.35 us 766.88 us] change: [-11.883% -9.4497% -6.8425%] (p = 0.00 < 0.05) Performance has improved. --- bracket-pathfinding/src/astar.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 4f408040..c83307c9 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -122,9 +122,13 @@ impl AStar { .find(|e| e.f < f && e.idx == idx) .is_none(); - // If a node with the same position as successor is in the closed list, with a lower f, skip add - if should_add && self.closed_list.contains_key(&idx) && self.closed_list[&idx] < f { - should_add = false; + if should_add { + // If a node with the same position as successor is in the closed list, with a lower f, skip add + if let Some(cf) = self.closed_list.get(&idx) { + if *cf < f { + should_add = false; + } + } } if should_add { From e8b3a9cc7ba640beb7ccfad034175ea74c656256 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:35:14 -0500 Subject: [PATCH 05/13] Do not make failure NavigationPath unless you need to. This is a very minor perf change but it just avoids making an object unless you need it. --- bracket-pathfinding/src/astar.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index c83307c9..ab507f78 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -165,7 +165,6 @@ impl AStar { /// Performs an A-Star search fn search(&mut self, map: &dyn BaseMap) -> NavigationPath { - let result = NavigationPath::new(); while !self.open_list.is_empty() && self.step_counter < MAX_ASTAR_STEPS { self.step_counter += 1; @@ -187,6 +186,7 @@ impl AStar { } self.closed_list.insert(q.idx, q.f); } - result + + NavigationPath::new() } } From a294e8c6bf8b2afa8b48a2c94e2ab097c7574dec Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:39:34 -0500 Subject: [PATCH 06/13] Allocate right-sized Vec for steps. We know how many elements will be in answer so use with_capcity instead of new. --- bracket-pathfinding/src/astar.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index ab507f78..110c9b0c 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -60,11 +60,11 @@ impl PartialOrd for Node { impl NavigationPath { /// Makes a new (empty) NavigationPath - pub fn new() -> NavigationPath { + pub fn new(capacity: usize) -> NavigationPath { NavigationPath { destination: 0, success: false, - steps: Vec::new(), + steps: Vec::with_capacity(capacity), } } } @@ -148,7 +148,7 @@ impl AStar { /// Helper function to unwrap a path once we've found the end-point. fn found_it(&self) -> NavigationPath { - let mut result = NavigationPath::new(); + let mut result = NavigationPath::new(self.parents.len()); result.success = true; result.destination = self.end; @@ -187,6 +187,6 @@ impl AStar { self.closed_list.insert(q.idx, q.f); } - NavigationPath::new() + NavigationPath::new(0) } } From 5cee586dcb63e17f1888bfaf5bcf371355e82cac Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 09:42:52 -0500 Subject: [PATCH 07/13] Do not delete what you will overwrite anyways. The check and remove is not needed because an insert happens right after that. --- bracket-pathfinding/src/astar.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 110c9b0c..c0428fa0 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -181,9 +181,6 @@ impl AStar { } } - if self.closed_list.contains_key(&q.idx) { - self.closed_list.remove(&q.idx); - } self.closed_list.insert(q.idx, q.f); } From 30729852df698d20ad54d49c368985488bebf0a1 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 11:01:07 -0500 Subject: [PATCH 08/13] Typo using wrong point for get_avilable_exits. --- bracket-pathfinding/benches/astar_benchmark.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bracket-pathfinding/benches/astar_benchmark.rs b/bracket-pathfinding/benches/astar_benchmark.rs index 7d13859a..7c0e4dad 100644 --- a/bracket-pathfinding/benches/astar_benchmark.rs +++ b/bracket-pathfinding/benches/astar_benchmark.rs @@ -108,7 +108,7 @@ impl BaseMap for Map { if let Some(idx) = self.valid_exit(location, Point::new(-1, 1)) { exits.push((idx, 1.4)) } - if let Some(idx) = self.valid_exit(location, Point::new(-1, 1)) { + if let Some(idx) = self.valid_exit(location, Point::new(1, 1)) { exits.push((idx, 1.4)) } From dba11f490b15f8b72a0973bbb43f62256004d218 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 13:23:55 -0500 Subject: [PATCH 09/13] Revert "Allocate right-sized Vec for steps." This reverts commit a294e8c6bf8b2afa8b48a2c94e2ab097c7574dec. --- bracket-pathfinding/src/astar.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index c0428fa0..51d854b7 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -60,11 +60,11 @@ impl PartialOrd for Node { impl NavigationPath { /// Makes a new (empty) NavigationPath - pub fn new(capacity: usize) -> NavigationPath { + pub fn new() -> NavigationPath { NavigationPath { destination: 0, success: false, - steps: Vec::with_capacity(capacity), + steps: Vec::new(), } } } @@ -148,7 +148,7 @@ impl AStar { /// Helper function to unwrap a path once we've found the end-point. fn found_it(&self) -> NavigationPath { - let mut result = NavigationPath::new(self.parents.len()); + let mut result = NavigationPath::new(); result.success = true; result.destination = self.end; @@ -184,6 +184,6 @@ impl AStar { self.closed_list.insert(q.idx, q.f); } - NavigationPath::new(0) + NavigationPath::new() } } From cdb3b88ec93581892b7736292b4f040d7a17fad4 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 6 Jun 2020 13:29:39 -0500 Subject: [PATCH 10/13] Push vs insert into steps A small gain in this benchmark by trading n memcpy from insert 1 vs n memory location swaps (pushes + reverse). If we could know size this would be even bigger as I think LLVM would basically perform the reverse for us for free. --- bracket-pathfinding/src/astar.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 51d854b7..894400f6 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -156,9 +156,10 @@ impl AStar { let mut current = self.end; while current != self.start { let parent = self.parents[¤t]; - result.steps.insert(0, parent); + result.steps.push(parent); current = parent; } + result.steps.reverse(); result } From 24ad3bf4ca7c9726de7478882600c2d9637194f3 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sun, 7 Jun 2020 09:22:46 -0500 Subject: [PATCH 11/13] Minor (unused uses and short-hand Node construction) --- bracket-pathfinding/benches/astar_benchmark.rs | 2 -- bracket-pathfinding/src/astar.rs | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/bracket-pathfinding/benches/astar_benchmark.rs b/bracket-pathfinding/benches/astar_benchmark.rs index 7c0e4dad..70883819 100644 --- a/bracket-pathfinding/benches/astar_benchmark.rs +++ b/bracket-pathfinding/benches/astar_benchmark.rs @@ -24,8 +24,6 @@ pub fn criterion_benchmark(c: &mut Criterion) { criterion_group!(benches, criterion_benchmark); criterion_main!(benches); -use bracket_color::prelude::*; -use bracket_pathfinding::prelude::*; use bracket_random::prelude::RandomNumberGenerator; pub const MAP_WIDTH: usize = 80; diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 894400f6..23dcde71 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -134,7 +134,7 @@ impl AStar { if should_add { let s = Node { idx, - f: f, + f, g: cost, h: distance, }; From d6ac05f81470290c97bc29ec1235b3b104f5da83 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sun, 7 Jun 2020 10:23:32 -0500 Subject: [PATCH 12/13] Use int comparison rather than float comparison first. BinaryHeap iter() will return elements in an arbitrary order so since we cannot take advantage of f being in an order we may as well use idx as the first comparison operation. --- bracket-pathfinding/src/astar.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 23dcde71..430f4fbc 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -119,7 +119,7 @@ impl AStar { let mut should_add = self .open_list .iter() - .find(|e| e.f < f && e.idx == idx) + .find(|e| e.idx == idx && e.f < f ) .is_none(); if should_add { From 115e3b73db3c473a9e70b8f7038cdb88de3cf4a9 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sun, 7 Jun 2020 11:14:12 -0500 Subject: [PATCH 13/13] Move closed list out of struct and make a local. Compiler seems to figure out this need not be on the heap perhaps? Pretty decent perf win --- bracket-pathfinding/src/astar.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bracket-pathfinding/src/astar.rs b/bracket-pathfinding/src/astar.rs index 430f4fbc..e251f71b 100644 --- a/bracket-pathfinding/src/astar.rs +++ b/bracket-pathfinding/src/astar.rs @@ -74,7 +74,6 @@ struct AStar { start: usize, end: usize, open_list: BinaryHeap, - closed_list: HashMap, parents: HashMap, step_counter: usize, } @@ -95,7 +94,6 @@ impl AStar { end, open_list, parents: HashMap::new(), - closed_list: HashMap::new(), step_counter: 0, } } @@ -106,7 +104,7 @@ impl AStar { } /// Adds a successor; if we're at the end, marks success. - fn add_successor(&mut self, q: Node, idx: usize, cost: f32, map: &dyn BaseMap) -> bool { + fn add_successor(&mut self, q: Node, idx: usize, cost: f32, map: &dyn BaseMap, closed_list: &HashMap) -> bool { // Did we reach our goal? if idx == self.end { self.parents.insert(idx, q.idx); @@ -124,7 +122,7 @@ impl AStar { if should_add { // If a node with the same position as successor is in the closed list, with a lower f, skip add - if let Some(cf) = self.closed_list.get(&idx) { + if let Some(cf) = closed_list.get(&idx) { if *cf < f { should_add = false; } @@ -166,6 +164,8 @@ impl AStar { /// Performs an A-Star search fn search(&mut self, map: &dyn BaseMap) -> NavigationPath { + let mut closed_list: HashMap = HashMap::new(); + while !self.open_list.is_empty() && self.step_counter < MAX_ASTAR_STEPS { self.step_counter += 1; @@ -176,13 +176,13 @@ impl AStar { let successors = map.get_available_exits(q.idx); for s in successors { - if q.idx != s.0 && self.add_successor(q, s.0, s.1 + q.f, map) { + if q.idx != s.0 && self.add_successor(q, s.0, s.1 + q.f, map, &closed_list) { let success = self.found_it(); return success; } } - self.closed_list.insert(q.idx, q.f); + closed_list.insert(q.idx, q.f); } NavigationPath::new()