-
Notifications
You must be signed in to change notification settings - Fork 134
perf: refactor to reduce memory allocations #1086
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
base: master
Are you sure you want to change the base?
Conversation
f1dca83
to
1429c31
Compare
oops, used |
1429c31
to
cf8f32d
Compare
let dots = if toplevels.is_empty() { | ||
(0..1) | ||
.map(|_| { | ||
container(vertical_space().height(Length::Fixed(0.0))) | ||
.padding(app_icon.dot_radius) | ||
.into() | ||
}) | ||
.collect_vec() | ||
} else { | ||
(0..1) | ||
.map(|_| { | ||
container(if toplevels.len() == 1 { | ||
vertical_space().height(Length::Fixed(0.0)) | ||
} else { | ||
match applet.anchor { | ||
PanelAnchor::Left | PanelAnchor::Right => { | ||
vertical_space().height(app_icon.bar_size) | ||
} | ||
PanelAnchor::Top | PanelAnchor::Bottom => { | ||
horizontal_space().width(app_icon.bar_size) | ||
} | ||
} | ||
}) | ||
.padding(app_icon.dot_radius) | ||
.class(theme::style::Container::Custom(Box::new(move |theme| { | ||
let dot_constructor = || { | ||
let space = if toplevels.len() <= 1 { | ||
vertical_space().height(Length::Fixed(0.0)) | ||
} else { | ||
match applet.anchor { | ||
PanelAnchor::Left | PanelAnchor::Right => { | ||
vertical_space().height(app_icon.bar_size) | ||
} | ||
PanelAnchor::Top | PanelAnchor::Bottom => { | ||
horizontal_space().width(app_icon.bar_size) | ||
} | ||
} | ||
}; | ||
let mut container = container(space).padding(app_icon.dot_radius); | ||
|
||
if !toplevels.is_empty() { | ||
container = | ||
container.class(theme::style::Container::Custom(Box::new(move |theme| { |
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.
I refactored this section completely, so I'd like a more thorough check of this part in particular.
I moved the logic of making the Element
s into its own closure (better variable names are very welcome!), which should make the same Element
s as before, just separate to the collection part and hopefully clearer. Then just make an iterator with std::iter::repeat_with
, which avoids the whole mess of mapping and collecting an iterator.
772262f
to
9f821d9
Compare
v2: I have now split the PR into two commits, one "safe" commit that should not change any behaviour, and one that uses unstable sorting, which could theoretically change behaviour but shouldn't in the places I've used them in. This PR probably still needs testing, but this should hopefully make that easier to do. |
Unstable sorting should be slightly faster than stable sorting, and if the vector was built asynchronously then preserving the initial order doesn't matter too much. Continue to use stable sorting where the vector is not built asynchronously, or if the vector is partially sorted (e.g. when new elements are pushed to a sorted vector).
9f821d9
to
f552e0d
Compare
Some minor refactors like removing
clone()
s, replacing with references, avoiding allocatingVec
s when they weren't necessary, etc.There's also a new
std::cmp::Ord
impl forActiveConnectionInfo
, which should be heaps better than usingformat!
. I had to also implstd::cmp::PartialCmp
manually to do that, but the actual logic there shouldn't matter as long as it agrees withstd::cmp::Ord
.