Skip to content

Bugfix/fix linear time search for registrations#157

Merged
zyclonite merged 4 commits intovert-x3:masterfrom
mutexd:bugfix/fix-linear-time-search-for-registrations
Apr 8, 2026
Merged

Bugfix/fix linear time search for registrations#157
zyclonite merged 4 commits intovert-x3:masterfrom
mutexd:bugfix/fix-linear-time-search-for-registrations

Conversation

@mutexd
Copy link
Copy Markdown
Contributor

@mutexd mutexd commented Mar 26, 2026

Motivation:

I'm switching between different cluster-managers in Vertx and I found out that in one situation, IgniteClusterManager performs way slower than HazelClusterManager. I have one Verticle frequently publishing to addresses that have not been subscribed by any consumer yet. Vertx continues to invoke clusterManager->getRegistrations(), and SubsMapHelper->get() takes up lots of cpu time. Having a look at the implementation between HazelClusterManager and IgniteClusterManager suggests that the IgniteClusterManager->SubsMapHelper->get() has performance issue when the number of registrations are large.

Solution:

  • The original implementation flatten all the registrations into the keys of the Map. When we need to search for all registrations of an address, it uses query() and the complexity essentially grows with the number of registrations linearly.
  • Instead, we should make use of the constant time assess in Map that we store address as the key so that when we search for all registrations, we directly get the result and we don't have to do linear time search among all registrations.

- The original implementation flatten all the registrations into the keys
  of the Map. When we need to search for all registrations of an
  address, it uses query() and the complexity essentially grows with the
  number of registrations linearly.
- Instead, we should make use of the constant time asscess in Map that we store address as the key so
  that when we search for all registrations, we directly get the result
  and we don't have to do linear time search among all registrations.
@zyclonite
Copy link
Copy Markdown
Member

thank you for your contribution, i will take a look

do you have some performance number before and after?

@zyclonite zyclonite added the bug label Mar 26, 2026
@zyclonite zyclonite self-assigned this Mar 26, 2026
@mutexd
Copy link
Copy Markdown
Contributor Author

mutexd commented Mar 27, 2026

Yes. I profile the system for like 15 seconds.
IgniteClusterManager.getRegistrations

test cpu time % of total
before 230164 67.54%
after 1729 2.43 %

Before:
Screenshot from 2026-03-27 08-31-23
After:

Screenshot from 2026-03-27 08-32-13

Comment thread src/main/java/io/vertx/spi/cluster/ignite/impl/SubsMapHelper.java Outdated
Comment thread src/main/java/io/vertx/spi/cluster/ignite/impl/SubsMapHelper.java
Comment thread src/main/java/io/vertx/spi/cluster/ignite/impl/SubsMapHelper.java Outdated
Comment thread src/main/java/io/vertx/spi/cluster/ignite/impl/SubsMapHelper.java Outdated
@zyclonite
Copy link
Copy Markdown
Member

maybe something like that would work to keep atomicity 690f977

@mutexd
Copy link
Copy Markdown
Contributor Author

mutexd commented Mar 31, 2026

maybe something like that would work to keep atomicity 690f977

That should be the way to go. Thanks for showing me how to use ignite.
Would you like to create another PR? I could close this one afterward.

@mutexd
Copy link
Copy Markdown
Contributor Author

mutexd commented Mar 31, 2026

In case you would like to collaborate on this PR.
I just imported your change and also improve the removeAllNode's filter.

@mutexd
Copy link
Copy Markdown
Contributor Author

mutexd commented Mar 31, 2026

I reverted the ScanQuery change to removeAllNode()
I realized that we shouldn't scan only related entries in removeAllNode(). Your RemoveNodeRegistrationsProcessor is where we operate entries atomically. It's not performant but that's the trade-off we have to take in my opinion.

@mutexd
Copy link
Copy Markdown
Contributor Author

mutexd commented Apr 1, 2026

The failed test case succeeds in my local environment. I'm not sure if I can provide any help for the CI.

@zyclonite
Copy link
Copy Markdown
Member

let me restart the build

@zyclonite
Copy link
Copy Markdown
Member

looks good, may i ask you do open a PR for the 5.0 branch as well
not sure how long it will take until 5.1.x gets released

@zyclonite zyclonite added this to the 5.1.0 milestone Apr 2, 2026
@zyclonite zyclonite changed the title fix getRegistration linear time search Bugfix/fix linear time search for registrations Apr 7, 2026
@zyclonite zyclonite merged commit 07bc312 into vert-x3:master Apr 8, 2026
4 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants