break infinite settings loop #176
Conversation
notoriaga
force-pushed
the
steve/perf-fix
branch
from
4e5fc91 to
fa78d7d
Compare
console_backend/src/settings_tab.rs
Outdated
| while let Ok(state) = recv.wait() { | ||
| tab.shared_state.set_settings_state(Default::default()); | ||
| tick(tab, state); | ||
| while let Ok(mut state) = recv.wait_mut() { |
There was a problem hiding this comment.
This loop waits for the settings state to change, when it does it runs tick which does whatever action is queued up. I called tab.shared_state.set_settings_state(Default::default()); to clear the state so things are only done once. But that of course queues up another event so effectively it was busy waiting. Adding a variant of wait that returns the locked value rather than a clone lets you mutate in place avoiding sending another notificaition
| fn clone(&self) -> WatchReceiver<T> { | ||
| WatchReceiver { | ||
| shared: Weak::clone(&self.shared), | ||
| shared: Arc::clone(&self.shared), |
There was a problem hiding this comment.
Can't be a weak ref if we want to return borrowed data. I don't think it was actually needed anyway
There was a problem hiding this comment.
As long as the watched value doesn't somehow hold a reference to the watcher then we should be fine.
There was a problem hiding this comment.
Yeah I think that should be impossible with the current setup. My original idea was that if you drop the watched value the receivers are useless so might as well free the memory. In practice I think the watched values and the receivers are going to be dropped at the same time anyway (when process_messages ends)
console_backend/src/settings_tab.rs
Outdated
| tick(tab, state); | ||
| while let Ok(mut state) = recv.wait_mut() { | ||
| let s = std::mem::take(&mut *state); | ||
| drop(state); |
There was a problem hiding this comment.
I still have trouble understanding these early drops, I suppose the take take allows us to reset state to a default value without trigger another update, but why is the early drop needed?
There was a problem hiding this comment.
I think otherwise we'd be holding the lock while tick is going, which can run for a while. By dropping the guard before tick the settings state can get updated by other threads while tick is running
There was a problem hiding this comment.
And yeah that's the idea of the take as well
There was a problem hiding this comment.
This is one area where rust gets a little magical with the deref trait. I guess the key here is we're dealing with two different objects, the guard and the object. We extract the object from the guard with the take (but that's made possible by the deref trait) and then we drop guard, to drop our hold on the lock.
There was a problem hiding this comment.
Yes definitely not super intuitive. Added the comment and also split the state and guard into two variables which I think makes it more clear what's being taken and what's being dropped
| fn clone(&self) -> WatchReceiver<T> { | ||
| WatchReceiver { | ||
| shared: Weak::clone(&self.shared), | ||
| shared: Arc::clone(&self.shared), |
There was a problem hiding this comment.
As long as the watched value doesn't somehow hold a reference to the watcher then we should be fine.
notoriaga
force-pushed
the
steve/perf-fix
branch
from
c321a48 to
903c6f0
Compare
3 participants
With this change
cargo prod-runseems to hover around 5-6% cpu usage with a full tracking tab. The perf change wasn't as drastic on my machine initially, so if someone with windows could take a peek that would be greatStill need to figure out whats up with the benchmarks