Possible perfomance issue in NotificationDispatcher

[bogdanul2003]

Hi,

While debugging with GDB I noticed that a lot of threads are created and destroyed while running my app that writes some ads variables and listens on some modification of other variables. By breaking on pthread_create() I realized that NotificationDispatcher::Notify() uses std::async() to call the callbacks for the changed variables. My use case is that PLC can modify variables 100 times per second (maximum value, it could be lower on average but there are multiple varaibles) this means that OS will have to create and destroy threads at a very high rate which is not something great from performance point of view because not only my app has to suffer but other processes as well. Are there any plans to improve this ?

Thank you.

[pbruenn]

Do you have any benchmarks, which can be used to visualize and optimize on this issue? The only effective alternative I see would add an API to provide your own threads for the callbacks.

[bogdanul2003]

Unforunatly I don't have a benchmark between the existing code and a library with a fix for this issue because I haven't written yet that code. But here is a hint why this is not that good from a performance point of view https://theburningmonk.com/2010/03/threading-using-the-threadpool-vs-creating-your-own-threads/ It talks about .NET but the main idea is the same. it's not that cheap to create and kill threads. It may be the case that the CPU spends more time on this thread overhead then in executing the actual callback code (http://www.bitsnbites.eu/benchmarking-os-primitives/ to make an idea what are CPU costs ). As suggested in the article, a solution could be a thread pool. Here you have two options, either you create the thread pool and manage it or you leave this to the user of the library and you provide an API, as you suggested, so that the user can run the callbacks in his predefined threads (one implementation could be a multiple producer - multiple consumer queue that is filled by the library with callback requests and one or more threads of the user read this queue and execute the callbacks)

[pbruenn]

Well, that‘s all true but without a benchmark any try to optimize something is futile. std::async behavior depends on your libc++. A sane implementation would already use a thread-pool. And a benchmark would be pretty helpful to easily find a good implementation.

[pbruenn]

Okay, finally I had time to look into the code. There is one thread per received notification packet. That means if your variable changes 100 times per second there is a high chance you call async just once per second.

[bogdanul2003]

I don't really understand why you claim that async will be called once per second if a a variable changes 100 times per second, I'm not that familiar with the details of ADS protocol and I didn't dive yet that deep in the code. But i've did some test runs using gdb and those are the results: I have 2 ADS variables that I monitor through callbacks and I generate 50 changes/second/variable for 2 seconds and from gdb output I can see that there are 200 threads creted and exited (pthread_create gets called 200 times) during this time, which means that for every modification on the PLC side a thread will be created on the ADSlib side. This is how the call stack looks like:

#0  __pthread_create_2_1 (newthread=0x7fffed0762b0, attr=0x0, start_routine=0x7ffff654d570, arg=0x7fffe0000bb0) at pthread_create.c:610

#1  0x00007ffff654d835 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6

#2  0x00007ffff6832f64 in NotificationDispatcher::Notify() () from /home/pnode/Documents/run/libs/libAdsClient.so

#3  0x00007ffff682a264 in AmsConnection::ReceiveNotification(AoEHeader const&) () from /home/pnode/Documents/run/libs/libAdsClient.so

#4  0x00007ffff682baae in AmsConnection::Recv() () from /home/pnode/Documents/run/libs/libAdsClient.so

#5  0x00007ffff682c873 in AmsConnection::TryRecv() () from /home/pnode/Documents/run/libs/libAdsClient.so

I'm using the standard c++ library, I don't know how the situation is on windows but I doubt that there will be any difference.

[pogojotz]

Hi @bogdanul2003, in fact it seems the situation on Windows is different. According to https://en.cppreference.com/w/cpp/thread/async, if std::async is called with std::launch::async the function is executed on "a new thread of execution (with all thread-locals initialized)" but also states that this thread "may be part of a thread pool".

Someone tested this on Windows and found, that in fact the pool variant was used (https://stackoverflow.com/a/42832368).

@pbruenn You could avoid the massive thread spawning, by having an own thread pool (or maybe just one thread), where you run the callbacks on and not use std::async. I don't really see a need to have every callback run on its own thread. As a library user I also would not expect to get an all fresh thread every time the callback is run.

[pbruenn]

Finally I see your point! After digging into #100 I finally see this async spawning is totally pointless, because we are limited by just one ring buffer. I will change that to a single static thread per buffer/notification, as soon as I am back from vacation. Sorry for taking so long to realize...

[pbruenn]

Well, the problem was even worse... The std::async as it was introduce was complete garbage. See the fix: https://github.com/Beckhoff/ADS/commit/6684d9308dc79993924b91c62fac4941d2d68f69 And this bug history for details: https://github.com/home-assistant/core/issues/38150