Skip to content

Eliminate O(n^2) status removal in DashboardTreeTableModel - #316

Open
NicoPiel wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
NicoPiel:fix/dashboard-status-removal
Open

Eliminate O(n^2) status removal in DashboardTreeTableModel#316
NicoPiel wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
NicoPiel:fix/dashboard-status-removal

Conversation

@NicoPiel

Copy link
Copy Markdown
Contributor

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).

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>
@github-actions

Copy link
Copy Markdown

Test Results

111 files ±0 214 suites ±0 7m 2s ⏱️ + 1m 18s
654 tests ±0 654 ✅ ±0 0 💤 ±0 0 ❌ ±0 
1 308 runs ±0 1 308 ✅ ±0 0 💤 ±0 0 ❌ ±0 

Results for commit c22b8ba. ± Comparison against base commit 2e0ff41.

@NicoPielNicoPiel self-assigned this May 27, 2026
@NicoPiel
NicoPiel requested review from a team, gibson9583, jonbartels, kayyagari, mgaffigan, pacmano1, ssrowe and tonygermano and removed request for a teamMay 27, 2026 21:03
@jonbartels

Copy link
Copy Markdown
Contributor

Is there an existing unit test that covers this method?

If no, can you create one?

@mgaffiganmgaffigan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jonbartelsjonbartels added this to the Next Release milestone May 27, 2026

@kryskoolkryskool left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, works for me

@tonygermanotonygermano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@NicoPiel

Copy link
Copy Markdown
ContributorAuthor

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@NicoPiel@jonbartels@kryskool@kpalang@mgaffigan@tonygermano