-
-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ui-state): do not overcalculate bandwidth and connections #114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Good work, the code can be simplified just a little bit
src/display/ui_state.rs
Outdated
for state in self.utilization_data.iter().rev() { | ||
let connections_to_procs = &state.connections_to_procs; | ||
let network_utilization = &state.network_utilization; | ||
for (connection, connection_info) in &network_utilization.connections { | ||
let connection_previously_seen = seen_connections.contains(connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be replaced with
let connection_previously_seen = !seen_connections.insert(connection);
And the insert statement at the bottom can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right! Thanks for this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just marking as "needs work" to take into account @zhangxp1998 suggestion
So, yesterday I broke master with this: #77
What that PR essentially does is save a cache of the last 5 states and then display their average bandwidth.
I forgot to take the total bandwidth into account, so until this PR master shows ~5 times as much total bandwidth as there actually is.
I also forgot to take the connection count into account, and so in master the connections were counted once for every state.
The total bandwidth calculation was a fairly straightforward fix: just divide it by the amount of cached states like we do with the rest of the bandwidth.
With the connection count, dividing would not do since we don't want a connection average of the past N seconds, but an exact count (of the past N seconds). I did this here by only counting a connection the first time we see it.
So, umm... oops. :)