Eliminate O(n^2) status removal in DashboardTreeTableModel#316
Conversation
updateChannelNodes called statuses.remove(status) inside a loop over child nodes. With an ArrayList, each remove is O(n), making the update O(channels × statuses). Collected matched statuses in a HashSet and called removeAll once after the loop, reducing to O(n + m). Signed-off-by: Nico Piel <nico.piel@hotmail.de>
NicoPiel
requested review from
a team,
gibson9583,
jonbartels,
kayyagari,
mgaffigan,
pacmano1,
ssrowe and
tonygermano
and removed request for
a team
|
Is there an existing unit test that covers this method? If no, can you create one? |
mgaffigan
left a comment
mgaffigan
left a comment
There was a problem hiding this comment.
This looks fine, but was this showing up on a profiler? There's going to be a lot of changes if you are not chasing based on actual performance issues.
kryskool
left a comment
kryskool
left a comment
There was a problem hiding this comment.
Good catch, works for me
tonygermano
left a comment
tonygermano
left a comment
There was a problem hiding this comment.
Similar to Mitch's comment, is there a measurable improvement from this change? The old code wasn't wrong. I don't want to turn down improvements, but I also don't want to encourage optimizations with no measurable impact, as they take up PR review bandwidth that could be used elsewhere.
|
I haven't measured anything; probably only measurable with a huge dashboard. I was looking for potential optimisations to the dashboard and stumbled upon this. |
6 participants
updateChannelNodes called statuses.remove(status) inside a loop over
child nodes. With an ArrayList, each remove is O(n), making the update
O(channels × statuses). Collected matched statuses in a HashSet and
called removeAll once after the loop, reducing to O(n + m).