Holoscape Debug View Improvements #1954
Conversation
…ll in that thread
…tions that can be big
…ain-rust into debug-view-improvements
lucksus
changed the title
Co-Authored-By: Eric Harris-Braun <eric@harris-braun.com>
| if let Some(broadcaster) = broadcasters.get(&interface.id) { | ||
| if let Err(error) = broadcaster.send(SignalWrapper { | ||
| signal: stats_signal.clone(), | ||
| instance_id: String::new(), |
There was a problem hiding this comment.
Does this need a // TODO?
| @@ -341,6 +347,8 @@ impl Conductor { | |||
| println!("INTERFACEs for SIGNAL: {:?}", interfaces); | |||
| interfaces | |||
| } | |||
|
|
|||
| Signal::Stats(_) => unreachable!(), | |||
There was a problem hiding this comment.
This doesn't seem great, any code that might happen to send a Stats signal to one of these receivers will crash the system. This creates a special case for Stats signals, where they are taboo to send outside of this thread.
Why break with the existing pattern here? This thread is for receiving signals and sending them out across the appropriate interface. There is duplicated code below to pick out the interface and generate the signal. It's not strictly wrong but it feels like it's hijacking this thread for a separate concern, and also breaking the assumption that someone can send a signal to a receiver and have it transmitted properly.
There was a problem hiding this comment.
Also, the calculation of stats could potentially take a significant amount of time, and this seems like it should be a tight loop so that the other signals can get delivered in a timely fashion, especially when we get into user-defined signals. I'd propose to move the generation of signals outside of this loop, and send it in across the channel like all the other signals.
There was a problem hiding this comment.
That unreachable! is an explicit statement about an invariant: namely that Stats signals are not send by any code. And if somebody would add that to any code, they would find this unreachable and at least know that they start using it differently to how it was meant to be used.
That at least is a common pattern that I use when adding explicit panics like this one - that is the semantic I connect to explict panics.
The Why:
I want Stats signals to be created by the conductor and not by instances so that we only have one signal for all instances. That is why I've decided to put it here instead of the instance.
There was a problem hiding this comment.
I really wish our code would reflect how it is intended to be used, rather than us relying on tribal knowledge of special cases. I know there's no foreseeable reason why anyone would send a Stats signal from elsewhere. It's not that I'm actually concerned about that unreachable code getting reachable. I'm concerned that we're hijacking a more specific pattern to be used for something more general, and I wish we would put in the bit of extra effort to generalize code when it needs to be generalized, rather than adding the minimum to get the special case working.
In this case the special case is a new type of signal that is associated with a conductor, and you're using a SignalWrapper which is intended for associating a Signal with an instance. To achieve that, you're introducing unreachable code, and ignoring a field of a struct that normally has semantic meaning. Everyone else who reads this code now has to apply the special knowledge that the instance_id field only matters sometimes, when it's not a Stats signal, and they have to find out the hard way if for whatever reason they wanted to send a Stats signal from an instance. For instance you could have at least made instance_id of SignalWrapper Optional.
I know that realistically this probably won't cause a crash, so my concern is not about proper functioning. It's about understanding the code, which is getting harder and harder to do. I feel that these special cases are slowly degrading our ability to reason about the code. We are fortunate enough to be using a language with a rich, expressive type system which in many cases can describe exactly how one should use a piece of code, and prevent anyone from using it the wrong way. I want to challenge us to actually make use of that, as a powerful documentation and intent-sharing tool.
Since this is a minor one I won't block this PR because of it. Can you just respond to the concern about potentially taking a long time in the loop? @lucksus
There was a problem hiding this comment.
You're right @maackle. This can and should be implemented by using the type system to make the impossible impossible. I just did that and pushed - please have another look.
I wasn't thinking the that calling .len() on some collections and iterating over held entries would block that thread in a noticeable manner and thought that adding a new thread might be more of a problem. But since the calculation of number of aspects is O(number of held entries) it can grow large over time. It is now done in a separate thread.
| number_held_entries: holding_map.keys().count(), | ||
| number_held_aspects: holding_map | ||
| .values() | ||
| .fold(0, |acc, aspect_set| acc + aspect_set.len()), |
There was a problem hiding this comment.
nice use of the fold here
Co-Authored-By: Purple Hair Rust Bard <AshantiMutinta@gmail.com>
Move run-time panic check to compile-time type power.
|
@maackle, thank you for holding for a cleaner approach. I was in a hurry when writing the first one and am much happier with this solution. I think I've addressed your concerns - if you agree feel free to merge it. |
PR summary
These are changes needed to improve the debug view inside Holoscape.
This is the corresponding PR there: holochain/holoscape#44
debug/state_dumpto select which portions of the state we want to gettesting/benchmarking notes
( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )
followups
( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )
changelog
documentation