Reduce stored similar paths and checked cycles #346
Conversation
Performance SummaryComparing base 1acebc7 with head de82b3c on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Performance SummaryComparing base 1acebc7 with head c6badc5 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
hendrikvanantwerpen
force-pushed
the
leaner-similar-paths-detection
branch
from
c6badc5
to
52d7bd5
Compare
Performance SummaryComparing base 1acebc7 with head 52d7bd5 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
hendrikvanantwerpen
changed the title
Performance SummaryComparing base 1acebc7 with head d7bb4ad on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Performance SummaryComparing base 1acebc7 with head e535492 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
As follow-on work I think we can relax this restriction, by tracking the in-degree of each node and path at construction time, and storing that information in our storage layer. We would then propagate that through in the paths that we load into a Does that sound right? |
| next_iteration: (VecDeque<PartialPath>, VecDeque<AppendingCycleDetector<H>>), | ||
| next_iteration: ( | ||
| VecDeque<PartialPath>, | ||
| VecDeque<AppendingCycleDetector<H>>, | ||
| VecDeque<bool>, | ||
| ), |
There was a problem hiding this comment.
In the future, if this grows another field, we'll probably want to turn this into a named struct with named fields instead of a tuple
There was a problem hiding this comment.
Probably a good idea, yes :) It's only internal, so it doesn't pollute the API, but it becoming a bit unwieldy like this :P
Yes, I think that is correct. I even considered this, but backed off once I realized I'd have to go through the storage layer as well to get this to work. Still, I think this might be quite a big win, because the initial partial path finding during indexing will build paths for every clickable node in the graph. At query time (where we don't yet have the optimization available) we only look at a single or a few references. Writing this, I'm wondering if serializing the partial path finding (i.e., do every initial path separately) might be a good idea as well? We don't share any work between each initial path, so it might only make a difference in max in-flight paths (and thus memory profile) but not in run time. |
stack-graphs/src/cycles.rs
Outdated
| while idx < possibly_similar_paths.len() { | ||
| match cmp(arena, path, &possibly_similar_paths[idx]) { | ||
| Some(Ordering::Less) => { | ||
| // the new path is betetr, remove the old one |
There was a problem hiding this comment.
| // the new path is betetr, remove the old one | |
| // the new path is better, remove the old one |
| Some(Ordering::Less) => { | ||
| // the new path is betetr, remove the old one | ||
| possibly_similar_paths.remove(idx); | ||
| // keep `idx` which now points to the next element |
There was a problem hiding this comment.
I don't love mutating a collection while iterating through it, but if we're going to do so, this seems like the reasonable way to do so. That said, it could probably use a comment above the loop noting what's going on here (and why).
There was a problem hiding this comment.
Good suggestion! In 07ca85d.
stack-graphs/src/graph.rs
Outdated
| @@ -1442,9 +1449,10 @@ pub struct StackGraph { | |||
| pub(crate) nodes: Arena<Node>, | |||
| pub(crate) source_info: SupplementalArena<Node, SourceInfo>, | |||
| node_id_handles: NodeIDHandles, | |||
| outgoing_edges: SupplementalArena<Node, SmallVec<[OutgoingEdge; 8]>>, | |||
| outgoing_edges: SupplementalArena<Node, SmallVec<[OutgoingEdge; 4]>>, | |||
| incoming_edges: SupplementalArena<Node, u32>, | |||
There was a problem hiding this comment.
Given the other discussion about relaxing the stitching restriction, you might consider not tracking the actual in-degree of each node, but instead tracking something like
#[repr(u8)]
enum Degree {
Zero,
One,
Multiple,
}That would cut down 4× on the storage space of this new arena and would eventually present a cleaner API if we decide to track this info through the storage layer
There was a problem hiding this comment.
That's a nice idea! In cd58abb.
I don't have a clear intuition how that would play out. You're right that it would reduce the max memory used. It might increase the runtime a bit, depending on how many more times that causes us to cross the Go/Rust FFI boundary. (Though it also might not, if we're doing enough work on the Rust side to drown out the Go FFI overhead.) There's also the cost of parsing each partial path from the storage layer and loading those into the Rust-side data structures. Maybe a middle ground where we serialize the processing of each partial path with a new |
I'll create a follow-up PR to discuss, then. |
Performance SummaryComparing base 1acebc7 with head cd58abb on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
#321 »
This PR makes a couple of changes to similar path detection to reduce memory usage and run time:
Observing that similar but different paths can only arise when the path split first, and then later joins, we only store paths for similar path detection when the two following conditions hold:
The path was split before, that is, it was extended in two different ways. This is tracked in the path stitcher.
The path currently ends in a possible join, that is, a node with multiple incoming edges. This could possibly refined to only consider actual candidates in the database.
Similarly, we can limit the times we check cycles as well. A path can only end in a cycle if one of the following holds:
The start and end node are the same, that is, the path loops around.
The end node has at least two incoming egdes in the graph (or partial paths in the database), that is, one in the path from the start node, and the other coming out of the loop.
Note that both require us to know the number of incoming edges or path of a node. This makes this optimization incompatible with stitching that dynamically add paths to the database from one phase to the next. Therefore, the optimization must be explicitly enabled using a flag.
Use smaller pre-allocated buckets. Some manual experiments using Collect stitching and database stats #326 suggests that most buckets in the similar path detector are small, and a few are much larger. By reducing the preallocated size of buckets from 8 to 3, we waste less space.