Fixes high CPU usage.

[rafaelbrizola]

Changes the collection implementation from ConcurrentSkipListSet to PriorityBlockingQueue in order to reduce the use of CPU and improve efficiency. In my preliminary tests, the CPU usage decreased around 50%. Attached is a screenshot of my VPS dashboard.

The current way of consuming the messages from the ConcurrentSkipListSet is very CPU inefficient because it loops through the list almost continuously (sometimes just sleeping 1 ms), even when the list is empty. In this case, it's better to use a queue implementation and block until a message arrives.

Important: when restarting the node, it will still consume almost maximum cpu because it will try to synchronise with the last milestone. Once it's synchronised, it should consume much less CPU.

[tactical-drone]

Interesting results so far.

Usually when I restart iri it just hangs on the base milestone for ages then suddenly goes into sync. After applying this patch it makes steady progress instead.

This specific node then also got stuck at some milestone that was the latest when iri started... lets wait and see if it fixes that problem too.

I changed my threads to spin 100 time per second instead of a 1000, but that seems to have had no effect. The signalling seems to make a huge difference as supposed to these old school while sleep loops. More than one would expect even. Good work!

[rafaelbrizola]

Hi @pompomJuice thanks for the feedback! Please, let me know if you have any problems syncing. Yeah, I noticed some improvement in other areas too, like the memory use for example. The memory use by the jvm is stable and below 50% of limit of 6GB I defined. My node has been running for almost 48 hours with this patch now. I was even able to add more neighbors. I have 50 at the moment via Nelson (Running in a VPS, 4 cores, 8GB of run). Last stats:

[tactical-drone]

No the node got stuck again.

I am rebuilding from scratch now. So far everything looks good.

[nuriel77]

Hi @rafaelbrizola !

I applied your fix on one of the nodes and immediately notice a difference:

[tactical-drone]

Was the node synced? From my experience when I restart iri it takes at least 30 minutes to an hour to go back into sync. Did you check?

[nuriel77]

The nodes where I applied this to were fully synced before applying, and after. It seems to always be the case that it takes a while to see progress from the "base milestone" 243000 after IRI restarts.

[BoxedBrain]

Hi, my node is showing a better performance too since the update.

[ghost]

Looks like the load dropped with the 470 fix.

[nuriel77]

@alon-e @paulhandy maybe worth looking into this.

[alon-e]

Hey @rafaelbrizola interesting analysis and solution, I'll be reviewing the code shortly - Thanks!

a few initial remarks:

1. re: pool() vs poolLast() when queue is full: so the reason behind this is quite important: in a high load regime, the network connection would be saturated and transactions w/ higher MWM would rise to the top of the priority queue, so if you want your translation to propagate faster in the network you would apply more PoW, hence the Comparator which sets priority based on MWM. so - when a new transaction is added to the queue, if it's full the transaction w/ the lowest priority should be dropped - while I agree that this currently is a rare occasion, this is an important aspect of the reference implementation.

2. the sleep(1) currently serves as a upper bound for TPS (you can see that a node cant handle more than 1kTPS due to these 1millisec sleeps), while this isn't a core "feature" of IRI, it does need to be noted that this change modifies this behavior as well.

[rafaelbrizola]

Hi @alon-e Thanks a lot for the feedback. Also:

1. Thanks for clarifying that. It makes more sense now. Sadly, the PriorityBlockingQueue doesn't have a poolLast() method and the iterator wouldn't help in this case. Either we will need to find a better implementation of a blocking queue or implement the removal "manually". Also, lets add some comments in the code with the clarification you did above, about why the order is important there :)

2. Would we still want to have an upper limit for TPS? isn't better to try to extract the best of what I node can do? :) If we need to keep the upper limit, probably is better to document in the code that those sleep(1) are there intentionally.

Cheers!

[rafaelbrizola]

Some people are mentioned in slack/discord that after patching 14.1.6 with this pull request, they could see improvements in the cpu usage, but it seems that the number of transactions "toRequest", as it is displayed in the iri.log, has been increasing slowly overtime. I'm still trying to figure out the reason. I'm not sure if this is a bug with this pull request or it was an existing bug that was not visible before, but it's now with this patch.

For those who don't know what the "toRequest" queue is, in the node perspective it means: "my neighbour sent me a request for this transaction, which I don't know (I don't have it in my tangle/database). So I will put it aside in this queue and request it regularly to my neighbours. Later when I receive that transaction, I'll remove it from these queue and broadcast it to all my neighbours." But if all your neighbours don't have this transaction too, the message will stay there forever. It seems that for those who are using only static neighbours this problem can be even more drastic. Nelson at least keeps "rotating" neighbours, so the chance to get that response from a new neighbour is much higher. (btw, now that Nelson is mature, in my opinion it is much better to use it than having static neighbours)

For those that patched to 470 and are facing high numbers in the "transactions toRequest", please, could test this? If you are using nelson, change the IRIProtocol to udp in nelson. Also, add the number of neighbours to incomingMax = 20, outgoingMax = 16, if your server can handle it. And restart Nelson (just nelson, not IRI). And observe IRI for some minutes (or 1 hour, depending on how big is the toProcess queue). See if it starts to decrease after new neighbours are connected.

I noticed that the "toRequest" was high in my node because I just had 9 neighbours in total and 1/3 of them where TCP with 0 transactions sent to me (not sure exactly why). So when I increased the number of neighbours and changed them to UDP (which works better in my node), after 1 hour the "toRequest" queue was back to less than 10. (from a highest of 2000+)

[miili]

@alon-e Is this PR still considered?

[GalRogozinski]

Hey @rafaelbrizola,

1. The reason this PR isn't merged yet is because of the missing pollLast() method. Do you have time to fix this via inheritance? If not do comment here and someone else will do it.

2. Regarding the `sleep(1)', yes it can be removed.

3. Good job on description and comments of the PR! Anyone who wishes to contribute to IOTA or any other open source project should look at this example from you!

[rafaelbrizola]

Hey @GalRogozinski, thank you very much for your feedback! Sorry, I can't at the moment. But to be honest, I couldn't think of a "elegant" solution. The last thing I could think was, in the case of high load and full queue, we just "invert" the order of the queue and call the pool() method. This way it would have the same effect, but it's a very ugly solution. And also, to be honest again, personally I'm still not convinced about the necessity of that ordering at all, mainly because of the nature of distributed data and because it's an edge case. But I'm probably missing something and/or not understanding the real importance of that piece of code.

[GalRogozinski]

@rafaelbrizola I think @alon-e explained pretty well. If you would like to have farther discussion you can take it to #tanglemath channel on Discord.

Anyhows this PR will be merged once someone will put the time to implement a priority queue with pollLast().