zb: make object destruction more robust - #1553
Conversation
Signed-off-by: Schmarni <marnistromer@gmail.com>
9bb6a95 to
c83f3d2
Compare
|
Thanks for the PR. I'm going on vacation for a week so I will likely only be able to look into this next week. |
CodSpeed Performance ReportMerging #1553 will not alter performanceComparing Summary
|
| !self.interfaces.keys().any(|k| { | ||
| (!self.interfaces.keys().any(|k| { | ||
| *k != Peer::name() | ||
| && *k != Introspectable::name() | ||
| && *k != Properties::name() | ||
| && *k != ObjectManager::name() | ||
| }) | ||
| })) && self.children.is_empty() | ||
| } |
There was a problem hiding this comment.
Looks like this change is about fixing the logic of "is this node empty" while the other changes aren't related to this fix? If so, could we please split the commit? It will also make it easier for me to review the changes.
There was a problem hiding this comment.
Looks like this change is about fixing the logic of "is this node empty" while the other changes aren't related to this fix?
Right, reading up the PR description, the other changes seems to be related but still a separate change: removing all empty nodes above the node being removed.
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Also, please add description/rationale to the commit messages too (not just the PR).
| if let Some(manager_path) = manager_path.clone() { | ||
| let ctxt = SignalEmitter::new(&self.connection(), manager_path.clone())?; |
There was a problem hiding this comment.
Why do we keep cloning the manager_path? 🤔 Pretty sure we don't need to do that for if let ... statements at least.
| fn gen_paths(paths: &mut Vec<(ObjectPath<'_>, String)>, path: ObjectPath<'_>) { | ||
| if path.is_empty() { | ||
| return; | ||
| } | ||
| let mut path_parts = path.rsplit('/').filter(|i| !i.is_empty()); | ||
| let last_part = path_parts.next().unwrap(); | ||
| let ppath = ObjectPath::from_string_unchecked( | ||
| path_parts.fold(String::new(), |a, p| format!("/{p}{a}")), | ||
| ); | ||
| paths.push((ppath.clone(), last_part.to_owned())); | ||
| gen_paths(paths, ppath); | ||
| } |
There was a problem hiding this comment.
That's potentially a lot of allocations. I'm confident we don't need them. We can parse the path as we traverse the hierarchy.
| if !node.is_empty() { | ||
| break; | ||
| } | ||
| let interfaces = node.interfaces().cloned().collect::<Vec<_>>(); |
|
@Schmarni-Dev so you think you'll be able to finish this? |
Hello? |
2 participants
the current implementation of object destruction causes issues if all interfaces are removed on an object that has a child with interfaces, this pr fixes that by only destroying objects if they don't have children, and cleaning up any parents that no longer have children