Merge pull request #205 from github/new-similar-path-detection

Author: hendrikvanantwerpen

stack-graphs/include/stack-graphs.h

@@ -1024,6 +1024,13 @@ struct sg_forward_path_stitcher *sg_forward_path_stitcher_new(const struct sg_st
                                                               size_t count,
                                                               const sg_node_handle *starting_nodes);
 
+// Sets whether similar path detection should be enabled during path stitching. Paths are similar
+// if start and end node, and pre- and postconditions are the same. The presence of similar paths
+// can lead to exponential blow up during path stitching. Similar path detection is disabled by
+// default because of the accociated preformance cost.
+void sg_forward_path_stitcher_set_similar_path_detection(struct sg_forward_path_stitcher *stitcher,
+                                                         bool detect_similar_paths);
+
 // Sets the maximum amount of work that can be performed during each phase of the algorithm. By
 // bounding our work this way, you can ensure that it's not possible for our CPU-bound algorithm
 // to starve any worker threads or processes that you might be using.  If you don't call this
@@ -1078,6 +1085,13 @@ struct sg_forward_partial_path_stitcher *sg_forward_partial_path_stitcher_from_p
                                                                                              size_t count,
                                                                                              const struct sg_partial_path *initial_partial_paths);
 
+// Sets whether similar path detection should be enabled during path stitching. Paths are similar
+// if start and end node, and pre- and postconditions are the same. The presence of similar paths
+// can lead to exponential blow up during path stitching. Similar path detection is disabled by
+// default because of the accociated preformance cost.
+void sg_forward_partial_path_stitcher_set_similar_path_detection(struct sg_forward_partial_path_stitcher *stitcher,
+                                                                 bool detect_similar_paths);
+
 // Sets the maximum amount of work that can be performed during each phase of the algorithm. By
 // bounding our work this way, you can ensure that it's not possible for our CPU-bound algorithm
 // to starve any worker threads or processes that you might be using.  If you don't call this

stack-graphs/src/c.rs

@@ -1825,6 +1825,21 @@ pub extern "C" fn sg_forward_path_stitcher_new(
     Box::into_raw(Box::new(ForwardPathStitcher::new(stitcher, paths))) as *mut _
 }
 
+/// Sets whether similar path detection should be enabled during path stitching. Paths are similar
+/// if start and end node, and pre- and postconditions are the same. The presence of similar paths
+/// can lead to exponential blow up during path stitching. Similar path detection is disabled by
+/// default because of the accociated preformance cost.
+#[no_mangle]
+pub extern "C" fn sg_forward_path_stitcher_set_similar_path_detection(
+    stitcher: *mut sg_forward_path_stitcher,
+    detect_similar_paths: bool,
+) {
+    let stitcher = unsafe { &mut *(stitcher as *mut ForwardPathStitcher) };
+    stitcher
+        .stitcher
+        .set_similar_path_detection(detect_similar_paths);
+}
+
 /// Sets the maximum amount of work that can be performed during each phase of the algorithm. By
 /// bounding our work this way, you can ensure that it's not possible for our CPU-bound algorithm
 /// to starve any worker threads or processes that you might be using.  If you don't call this
@@ -2011,6 +2026,21 @@ pub extern "C" fn sg_forward_partial_path_stitcher_from_partial_paths(
     ))) as *mut _
 }
 
+/// Sets whether similar path detection should be enabled during path stitching. Paths are similar
+/// if start and end node, and pre- and postconditions are the same. The presence of similar paths
+/// can lead to exponential blow up during path stitching. Similar path detection is disabled by
+/// default because of the accociated preformance cost.
+#[no_mangle]
+pub extern "C" fn sg_forward_partial_path_stitcher_set_similar_path_detection(
+    stitcher: *mut sg_forward_partial_path_stitcher,
+    detect_similar_paths: bool,
+) {
+    let stitcher = unsafe { &mut *(stitcher as *mut InternalForwardPartialPathStitcher) };
+    stitcher
+        .stitcher
+        .set_similar_path_detection(detect_similar_paths);
+}
+
 /// Sets the maximum amount of work that can be performed during each phase of the algorithm. By
 /// bounding our work this way, you can ensure that it's not possible for our CPU-bound algorithm
 /// to starve any worker threads or processes that you might be using.  If you don't call this

stack-graphs/src/cycles.rs

@@ -29,10 +29,9 @@
 //! always use this particular heuristic, however!  We reserve the right to change the heuristic at
 //! any time.
 
-use std::collections::HashMap;
-
 use enumset::EnumSet;
 use smallvec::SmallVec;
+use std::collections::HashMap;
 
 use crate::arena::Handle;
 use crate::arena::List;
@@ -45,6 +44,7 @@ use crate::partial::PartialPath;
 use crate::partial::PartialPaths;
 use crate::paths::Path;
 use crate::paths::PathResolutionError;
+use crate::paths::Paths;
 use crate::stitching::Database;
 use crate::stitching::OwnedOrDatabasePath;
 
@@ -58,44 +58,48 @@ pub struct SimilarPathDetector<P> {
 pub struct PathKey {
     start_node: Handle<Node>,
     end_node: Handle<Node>,
+    symbol_stack_precondition_len: usize,
+    scope_stack_precondition_len: usize,
+    symbol_stack_postcondition_len: usize,
+    scope_stack_postcondition_len: usize,
 }
 
 #[doc(hidden)]
 pub trait HasPathKey: Clone {
+    type Arena;
     fn key(&self) -> PathKey;
-    fn is_shorter_than(&self, other: &Self) -> bool;
 }
 
 impl HasPathKey for Path {
+    type Arena = Paths;
+
     fn key(&self) -> PathKey {
         PathKey {
             start_node: self.start_node,
             end_node: self.end_node,
+            symbol_stack_precondition_len: 0,
+            scope_stack_precondition_len: 0,
+            symbol_stack_postcondition_len: self.symbol_stack.len(),
+            scope_stack_postcondition_len: self.scope_stack.len(),
         }
     }
-
-    fn is_shorter_than(&self, other: &Self) -> bool {
-        self.edges.len() < other.edges.len() && self.symbol_stack.len() <= other.symbol_stack.len()
-    }
 }
 
 impl HasPathKey for PartialPath {
+    type Arena = PartialPaths;
+
     fn key(&self) -> PathKey {
         PathKey {
             start_node: self.start_node,
             end_node: self.end_node,
+            symbol_stack_precondition_len: self.symbol_stack_precondition.len(),
+            scope_stack_precondition_len: self.scope_stack_precondition.len(),
+            symbol_stack_postcondition_len: self.symbol_stack_postcondition.len(),
+            scope_stack_postcondition_len: self.scope_stack_postcondition.len(),
         }
     }
-
-    fn is_shorter_than(&self, other: &Self) -> bool {
-        self.edges.len() < other.edges.len()
-            && (self.symbol_stack_precondition.len() + self.symbol_stack_postcondition.len())
-                <= (other.symbol_stack_precondition.len() + other.symbol_stack_postcondition.len())
-    }
 }
 
-const MAX_SIMILAR_PATH_COUNT: usize = 7;
-
 impl<P> SimilarPathDetector<P>
 where
     P: HasPathKey,
@@ -107,34 +111,35 @@ where
         }
     }
 
-    /// Determines whether we should process this path during the path-finding algorithm.  If our
-    /// heuristics decide that this path is a duplicate, or is "non-productive", then we return
-    /// `false`, and the path-finding algorithm will skip this path.
-    pub fn should_process_path<F>(&mut self, path: &P, cmp: F) -> bool
+    /// Determines whether we should process this path during the path-finding algorithm.  If we have seen
+    /// a path with the same start and end node, and the same pre- and postcondition, then we return false.
+    /// Otherwise, we return true.
+    pub fn has_similar_path<Eq>(
+        &mut self,
+        _graph: &StackGraph,
+        arena: &mut P::Arena,
+        path: &P,
+        eq: Eq,
+    ) -> bool
     where
-        F: FnMut(&P) -> std::cmp::Ordering,
+        Eq: Fn(&mut P::Arena, &P, &P) -> bool,
     {
         let key = path.key();
-        let paths_with_same_nodes = self.paths.entry(key).or_default();
-        let index = match paths_with_same_nodes.binary_search_by(cmp) {
-            // We've already seen this exact path before; no need to process it again.
-            Ok(_) => return false,
-            // Otherwise add it to the list.
-            Err(index) => index,
-        };
 
-        // Count how many paths we've already processed that have the same endpoints and are
-        // "shorter".
-        let similar_path_count = paths_with_same_nodes
-            .iter()
-            .filter(|similar_path| similar_path.is_shorter_than(path))
-            .count();
-        if similar_path_count > MAX_SIMILAR_PATH_COUNT {
-            return false;
+        let possibly_similar_paths = self.paths.entry(key).or_default();
+        for other_path in possibly_similar_paths.iter() {
+            if eq(arena, path, other_path) {
+                return true;
+            }
         }
 
-        paths_with_same_nodes.insert(index, path.clone());
-        true
+        possibly_similar_paths.push(path.clone());
+        false
+    }
+
+    #[cfg(feature = "copious-debugging")]
+    pub fn max_bucket_size(&self) -> usize {
+        self.paths.iter().map(|b| b.1.len()).max().unwrap_or(0)
     }
 }