Fix #629: Adding subscriptions is very slow #630

hylkevds · 2021-09-15T08:37:26Z

Sort CNode.children by token
- made Token comparable
- made CNode.token final
- Changed all searches to use Collections.binarySearch()
- Changed inserts to respect sorting order
Optimised away all instances that loop over all children
Optimised away double lookup caused by "contains then get"

hylkevds · 2023-05-29T10:19:13Z

This not only massively speeds up adding subscriptions, also matching publishes to subscriptions is much more efficient when there are many topics with subscriptions.

andsel

I think the idea to switch to binarySearch is good, however, due to the facts that some optimizations are not easy to grasp, maybe each one could be separated in its own PS, describing well the optimization and why.

I would appreciate if the code could be accompanied with a test that with a huge set of sibling subscriptions shows the slowness. For example an insert or a search that fails after 5 minutes.
In this way just switching the linear scan to binary search shows that time is reduced a lot.

andsel · 2023-06-17T14:31:04Z

broker/src/test/java/io/moquette/integration/PublishToManySubscribersUseCaseTest.java

@@ -162,7 +162,7 @@ void onePublishTriggerManySubscriptionsNotifications() throws MqttException, Int
    }

    private void segmentedParallelSubscriptions(BiConsumer<IMqttAsyncClient, IMqttActionListener> biConsumer) throws InterruptedException {
-        int openSlotCount = COMMAND_QUEUE_SIZE;


I don't understand the reason why of this

Timing...
Because the code is now faster, it can flood the queue since each action generates more than one command on the Queue.
It used to work because the slowness of the insert delayed the creation of commands enough for the worker threads to catch up.

It surprised me too, but you should try it out for yourself :)

broker/src/main/java/io/moquette/broker/subscriptions/CNode.java

andsel · 2023-06-17T14:40:49Z

broker/src/main/java/io/moquette/broker/subscriptions/CNode.java

+        if (idx < 0)
+            return null;


In previous code when childOf wasn't able to find an INode associated to a Token it raised an IllegalArgumentException, I don't understand the reason to move to a null return.

That's part of the "optimised away double lookup" commit. Often the code did a anyChildrenMatch() followed by a childOf() meaning it did the expensive lookup twice. That's really inefficient of course. So to remove this superfluous anyChildrenMatch() call, we make childOf() return null if there is no such child. We could do an Optional, but since it's not a public interface that seemed redundant.

Going step by step is much easier and now I can see the motivation of the changes. Maybe a more descriptive comment for the "Optimised away all instances that loop over all children" case. Something like:

Not all next level children has to be verified recursively, but only the multi level any (#) the single level one (+) or the only matching node with the next head token part.

Sure, I can update the commit message with a longer explanation.

broker/src/main/java/io/moquette/broker/subscriptions/CNode.java

hylkevds · 2023-06-17T20:32:59Z

I think the idea to switch to binarySearch is good, however, due to the facts that some optimizations are not easy to grasp, maybe each one could be separated in its own PS, describing well the optimization and why.

That's what the separate commits are for. Separating those into their own PRs makes no sense, since they don't work separately. Just go through the separate commits. You can even check them out individually with GIT to try them out locally.

I would appreciate if the code could be accompanied with a test that with a huge set of sibling subscriptions shows the slowness. For example an insert or a search that fails after 5 minutes. In this way just switching the linear scan to binary search shows that time is reduced a lot.

That should be possible :)

hylkevds · 2023-06-17T21:47:32Z

I would appreciate if the code could be accompanied with a test that with a huge set of sibling subscriptions shows the slowness. For example an insert or a search that fails after 5 minutes.

I've added two tests that together run in ~4 seconds with the new code on my machine, but time out after 5 minutes with the old code.

andsel

Thank's @hylkevds for your patience. The PR is pretty good, just a couple of final touches.

andsel · 2023-06-19T14:51:35Z

broker/src/main/java/io/moquette/broker/subscriptions/CNode.java

-            }
+        int idx = findIndexForToken(token);
+        if (idx < 0) {
+            return null;


I think would better to be more explicit. nullis speedy but could drive us to the NPE. I think would be better to explicitly return an Optional, it's designed for cases such this.

No Problem, updated!

andsel · 2023-06-19T14:52:07Z

broker/src/test/java/io/moquette/broker/subscriptions/CTrieSpeedTest.java

@@ -0,0 +1,118 @@
+/*
+ * Copyright (c) 2012-2018 The original author or authors


Suggested change

* Copyright (c) 2012-2018 The original author or authors

* Copyright (c) 2012-2023 The original author or authors

Good point, I've updated it.

In FROST-Server we're now using spotless-maven-plugin to automatically check the licence header and code formatting. When something isn't how it should be it can also automatically apply the corrections.

andsel · 2023-06-19T14:55:50Z

broker/src/test/java/io/moquette/broker/subscriptions/CTrieSpeedTest.java

+        for (int b = 0; b < TOTAL_SUBSCRIPTIONS / 10; b++) {
+            for (int a = 0; a < 10; a++) {
+                count++;
+                results.add(clientSubOnTopic("Client-" + a, "mainTopic-" + b));
+            }
+        }


Suggested change

for (int b = 0; b < TOTAL_SUBSCRIPTIONS / 10; b++) {

for (int a = 0; a < 10; a++) {

count++;

results.add(clientSubOnTopic("Client-" + a, "mainTopic-" + b));

}

}

for (int v = 0; topicIdx < TOTAL_SUBSCRIPTIONS / 10; topicIdx++) {

for (int clientIdx = 0; clientIdx < 10; clientIdx++) {

count++;

results.add(clientSubOnTopic("Client-" + clientIdx, "mainTopic-" + topicIdx));

}

}

Instead of a and b, using something more expressive would help readability

Sure, no problem. Also for the other method.

andsel · 2023-06-19T15:00:11Z

broker/src/test/java/io/moquette/broker/subscriptions/CTrieSpeedTest.java

+        for (int a = 0; a < countPerLevel; a++) {
+            for (int b = 0; b < countPerLevel; b++) {
+                for (int c = 0; c < countPerLevel; c++) {
+                    for (int d = 0; d < countPerLevel; d++) {
+                        count++;
+                        results.add(clientSubOnTopic("Client-" + a, "mainTopic-" + b + "/subTopic-" + c + "/subSubTopic" + d));
+                        if (count >= TOTAL_SUBSCRIPTIONS) {
+                            break outerloop;
+                        }
+                    }
+                }
+            }
+        }


Suggested change

for (int a = 0; a < countPerLevel; a++) {

for (int b = 0; b < countPerLevel; b++) {

for (int c = 0; c < countPerLevel; c++) {

for (int d = 0; d < countPerLevel; d++) {

count++;

results.add(clientSubOnTopic("Client-" + a, "mainTopic-" + b + "/subTopic-" + c + "/subSubTopic" + d));

if (count >= TOTAL_SUBSCRIPTIONS) {

break outerloop;

}

}

}

}

}

for (int clientIdx = 0; clientIdx < countPerLevel; clientIdx++) {

for (int rootLayerIdx = 0; rootLayerIdx < countPerLevel; rootLayerIdx++) {

for (int firstLayerIdx = 0; firstLayerIdx < countPerLevel; firstLayerIdx++) {

for (int secondLayerIdx = 0; secondLayerIdx < countPerLevel; secondLayerIdx++) {

count++;

results.add(clientSubOnTopic("Client-" + clientIdx, "mainTopic-" + rootLayerIdx + "/subTopic-" + firstLayerIdx + "/subSubTopic" + secondLayerIdx));

if (count >= TOTAL_SUBSCRIPTIONS) {

break outerloop;

}

}

}

}

}

The same spirit could be applied here, with better naming could increase readability

… final.

Only 3 children can possibly match: +, # and the exact match. Instead of looping over all children trying to mach each, we specifically fetch these three and only handle them.

The calls to anyChildrenMatch are always followed by a call to childOf.

When updating we must first get the cnode, then analyse how to do the update and then compareAndSet the new cnode. If we first analyse and then get the cnode, the cnode may have changed in the meantime, and the analysis result may no longer be valid.

Moquette also generates commands on the queue internally, so pushing too many concurrent subscriptions can overflow the queue.

andsel

Great work LGTM!

hylkevds mentioned this pull request Sep 15, 2021

62w subscriptions cost about 85min #629

Closed

hylkevds mentioned this pull request May 28, 2023

Fix #629: Adding subscriptions is very slow #633

Closed

hylkevds force-pushed the fix629_CNode-Performance branch from 2f77a27 to a816d01 Compare May 28, 2023 17:15

hylkevds marked this pull request as ready for review May 28, 2023 17:17

hylkevds requested a review from andsel May 28, 2023 17:17

andsel mentioned this pull request Jun 10, 2023

Connection filter interface for IP Bans #747

Open

andsel added the v0.17.0 label Jun 17, 2023

andsel requested changes Jun 17, 2023

View reviewed changes

hylkevds force-pushed the fix629_CNode-Performance branch from 511b07e to 4059f20 Compare June 17, 2023 21:46

hylkevds force-pushed the fix629_CNode-Performance branch from 4570ad8 to 49da768 Compare June 17, 2023 22:06

andsel self-requested a review June 19, 2023 14:49

andsel requested changes Jun 19, 2023

View reviewed changes

hylkevds force-pushed the fix629_CNode-Performance branch from 49da768 to e46fb2d Compare June 19, 2023 16:46

hylkevds added 10 commits June 19, 2023 18:53

Added tests to demonstrate slowness of CTrie

8e86360

Sort CNode.children by token, made Token comparable, mode CNode.token…

54963c1

… final.

Fixed add method

22fa5da

Optimised away loop over all children when matching subscriptions

c60aa44

Only 3 children can possibly match: +, # and the exact match. Instead of looping over all children trying to mach each, we specifically fetch these three and only handle them.

Optimised away double lookup

ece8c44

The calls to anyChildrenMatch are always followed by a call to childOf.

Replace TNodes when hitting them during subscription insert

6b83816

Reduce open slots in Test

86095c7

Moquette also generates commands on the queue internally, so pushing too many concurrent subscriptions can overflow the queue.

Consolidated binarySearch calls into one method, cleaned up generics

a8d102e

Converted returned null and null checks to Optional

c24d92b

hylkevds force-pushed the fix629_CNode-Performance branch from e46fb2d to c24d92b Compare June 19, 2023 16:53

andsel self-requested a review June 20, 2023 18:58

andsel approved these changes Jun 20, 2023

View reviewed changes

andsel merged commit 62cb2b3 into moquette-io:main Jun 20, 2023
4 checks passed

andsel added a commit to andsel/moquette that referenced this pull request Jun 20, 2023

Minor, adds comments to changelog for moquette-io#630

c929718

andsel added a commit that referenced this pull request Jun 20, 2023

Minor, adds comments to changelog for #630 (#757)

c7017bf

hylkevds deleted the fix629_CNode-Performance branch April 30, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #629: Adding subscriptions is very slow #630

Fix #629: Adding subscriptions is very slow #630

hylkevds commented Sep 15, 2021 •

edited

Loading

hylkevds commented May 29, 2023

andsel left a comment

andsel Jun 17, 2023

hylkevds Jun 17, 2023

andsel Jun 17, 2023

hylkevds Jun 17, 2023

andsel Jun 19, 2023

hylkevds Jun 19, 2023

hylkevds commented Jun 17, 2023

hylkevds commented Jun 17, 2023

andsel left a comment

andsel Jun 19, 2023

hylkevds Jun 19, 2023

andsel Jun 19, 2023

hylkevds Jun 19, 2023

andsel Jun 19, 2023

hylkevds Jun 19, 2023

andsel Jun 19, 2023

andsel left a comment

		@@ -0,0 +1,118 @@
		/*
		* Copyright (c) 2012-2018 The original author or authors

	* Copyright (c) 2012-2018 The original author or authors
	* Copyright (c) 2012-2023 The original author or authors

Fix #629: Adding subscriptions is very slow #630

Fix #629: Adding subscriptions is very slow #630

Conversation

hylkevds commented Sep 15, 2021 • edited Loading

hylkevds commented May 29, 2023

andsel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hylkevds commented Jun 17, 2023

hylkevds commented Jun 17, 2023

andsel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andsel left a comment

Choose a reason for hiding this comment

hylkevds commented Sep 15, 2021 •

edited

Loading