Skip to content
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

Optimize performance on DiscoveryNodeManager #18542

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

guhanjie
Copy link
Member

@guhanjie guhanjie commented Aug 4, 2023

Description

Avoid unnecessary nested loops O(N*N) on DiscoveryNodeManager#refreshNodesInternal

Additional context and related issues

It does nested loop O(N*N) when services filter out failure nodes on DiscoveryNodeManager#refreshNodesInternal, it's no big deal on small clusters with few nodes, however it consumes quite a lot of cpu cycles on large cluster with over 1000 nodes, that will do 1,000,000 cycles per round of refreshNodeInternal. According to acutal cpu profiling, we found jvm does not optimize the code automatically, so this commit optimize it and avoid nested loop.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# General
* Reduce unnecessary nested loops overhead on DiscoveryNodeManager#refreshNodesInternal.
   This can avoid unnecessary nested loops while services filtering out failure nodes on DiscoveryNodeManager.

@cla-bot cla-bot bot added the cla-signed label Aug 4, 2023
@guhanjie guhanjie requested a review from findepi August 4, 2023 13:48
…odesInternal

It does nested loop O(N*N) when services filter out failure nodes on DiscoveryNodeManager#refreshNodesInternal, jvm does not optimize the code automatically, so we optimize to avoid nested loop.
@guhanjie guhanjie force-pushed the fix/opt_node_failure_filter branch from 39b47a3 to a41ca71 Compare August 8, 2023 01:38
@guhanjie
Copy link
Member Author

@findepi please merge the approved PR

@wendigo
Copy link
Contributor

wendigo commented Aug 16, 2023

@guhanjie please improve the commit message so it renders in a github without ellipsis

@martint martint merged commit dbcce7b into trinodb:master Aug 16, 2023
@github-actions github-actions bot added this to the 424 milestone Aug 16, 2023
@guhanjie guhanjie changed the title Avoid unnecessary nested loop O(N*N) on DiscoveryNodeManager#refreshNodesInternal Optimize performance on DiscoveryNodeManager Aug 16, 2023
@guhanjie
Copy link
Member Author

guhanjie commented Aug 16, 2023

@guhanjie please improve the commit message so it renders in a github without ellipsis

PR title updated: Optimize performance on DiscoveryNodeManager
Do you mean PR title or commit message, which one will introduced into release notes?
the code has been merged, i cannot modify any more..

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

Successfully merging this pull request may close these issues.

4 participants