chore: manage call stacks using a tree #6791
Conversation
Peak Memory Sample
|
Compilation Sample
|
|
Looks like this is an improvement over #6747 and gets us closer to the optimal #6753 (comment) |
|
We are eating a performance penalty however. |
My guess is that it is due to getting a big list of flattened call stacks; not too deep but very wide, which means some elements have a lot of children and searching among them would be significant. I will try to use some sorted container for the children, like a btree. |
| fn add_location(&self, location: Location, locations: &mut Vec<LocationNode>) -> CallStackId { | ||
| if let Some(result) = locations[self.index()] | ||
| .children | ||
| .iter() | ||
| .find(|child| locations[child.index()].value == location) |
There was a problem hiding this comment.
There are quite a few invariants assumed about the locations vector, e.g. that self is in it, and that the children of a LocationNode are all in it as well; it would be worth creating a newtype to wrap the Vec and make sure these invariants hold, and you're not dealing with just any odd vec! someone made.
| if self.location_array.is_empty() { | ||
| self.location_array.push(LocationNode { | ||
| parent: None, | ||
| children: vec![], | ||
| value: location, | ||
| }); |
There was a problem hiding this comment.
I think the fact that here we have to think about the array being empty is a sign that the array itself should be a newtype that holds these methods, like self.location_array.add_location_to_root would establish the root ID, and then self.location_array.add_location(parent_id, location) would be an example of adding more stuff, rather than the IDs manipulating the collection.
There was a problem hiding this comment.
I mean the sign that something is not right is that a person cannot just safely do CallStackId::root().add_location(location, &mut self.location_array) because it might panic, so CallStackId alone is not responsible for the invariants, although it could special case add_location for when self.is_root() is true and the target is empty.
There was a problem hiding this comment.
I tried to refactor this.
| @@ -85,6 +86,8 @@ impl Function { | |||
| /// Note that any parameters or attributes of the function must be manually added later. | |||
| pub(crate) fn new(name: String, id: FunctionId) -> Self { | |||
| let mut dfg = DataFlowGraph::default(); | |||
| // Adds root node for the location tree | |||
| dfg.add_location_to_root(Location::dummy()); | |||
There was a problem hiding this comment.
Is this something that could be done in DataFlowGraph::default() itself?
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Description
Problem*
Resolves #6603
Summary*
Call stacks are stored inside a big tree, which allows to share identical prefixes between call stacks
Additional Context
The only drawback is that we need to re-create the trees among function contexts during inlining
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.