add ActiveLeavesUpdate, remove StartWork, StopWork #1458
Conversation
coriolinus
added
A3-in_progress
Note: this does not yet convert the tests; some of the tests now freeze: test tests::overseer_start_stop_works ... test tests::overseer_start_stop_works has been running for over 60 seconds test tests::overseer_finalize_works ... test tests::overseer_finalize_works has been running for over 60 seconds
github-actions
bot
added
A0-please_review
node/overseer/src/lib.rs
Outdated
| trait Equivalent { | ||
| fn equiv(&self, other: &Self) -> bool; | ||
| } | ||
|
|
||
| impl Equivalent for ActiveLeavesUpdate { | ||
| fn equiv(&self, other: &Self) -> bool { | ||
| self.activated.iter().collect::<HashSet<_>>() == other.activated.iter().collect::<HashSet<_>>() && | ||
| self.deactivated.iter().collect::<HashSet<_>>() == other.deactivated.iter().collect::<HashSet<_>>() | ||
| } | ||
| } | ||
|
|
||
| impl Equivalent for OverseerSignal { | ||
| fn equiv(&self, other: &Self) -> bool { | ||
| use OverseerSignal::{ | ||
| ActiveLeaves, | ||
| Conclude, | ||
| }; | ||
|
|
||
| match (self, other) { | ||
| (ActiveLeaves(lalu), ActiveLeaves(ralu)) => lalu.equiv(ralu), | ||
| (Conclude, Conclude) => true, | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
although these are send around as Vecs essentially they are sets so it would probably be cleaner to just implement Eq on ActiveLeavesUpdate?
node/network/bridge/src/lib.rs
Outdated
| event_producers.values(), | ||
| &mut ctx, | ||
| ).await { | ||
| log::warn!("Aborting - Failure to dispatch messages to overseer"); |
There was a problem hiding this comment.
we probably should use targets in all logging
node/network/bridge/src/lib.rs
Outdated
| if let Some(view_update) | ||
| = update_view(&peers, &live_heads, &mut net, &mut local_view).await? | ||
| { | ||
| if let Err(e) = dispatch_update_to_all( |
There was a problem hiding this comment.
One of the motivations of this change was to do only one update_view/dispatch_update_to_all when a new relay-parent comes in. That can be extracted to after the live_heads updates done by activated/deactivated.
| /// | ||
| /// If there are fewer than this number of slots, then we've wasted some stack space. | ||
| /// If there are greater than this number of slots, then we fall back to a heap vector. | ||
| const ACTIVE_LEAVES_SMALLVEC_CAPACITY: usize = 8; |
There was a problem hiding this comment.
Could maybe go even smaller, although I doubt it will impact performance or memory in any meaningful way
There was a problem hiding this comment.
Note, that given vec optimizations like https://github.com/rust-lang/rust/pull/72227/files, using smallvec will only save us one allocation and indirection for small active sets. The hardest part is to come with a benchmark to justify it.
There was a problem hiding this comment.
Agree that the right way to tune this is with a benchmark. Agree that it would be hard to write a really good benchmark for this. I think we all agree that 8 is a plausible size here anyway? For now, I think I'd prefer to just leave it than to spend the effort to determine the actual optimal size.
This cleans up the code a bit and makes it easier in the future to do the right thing when comparing ALUs.
Closes #1418.