Idle CPU usage improvements #593

fedux · 2018-12-30T22:29:25Z

This PRs simplifies mutex and threads, fixes some unprotected members and includes a few changes to improve CPU usage when idle.

Part of a solution to #351

hugbug · 2018-12-30T23:20:55Z

That's a brilliant work. I have some questions which I'll post inline.

daemon/queue/DownloadInfo.h

daemon/util/Thread.cpp

daemon/util/Thread.h

daemon/feed/FeedCoordinator.h

Basically this is just removing the custom class and using a typedef to keep the name. Most of the changes are just case for the lock/unlock methods.

Just use Mutex and remove the need to call Thread::Init()

Use a typedef to maintain the name and use Guard for simple scoped lock guards and UniqueLock when it needs to be movable.

hugbug · 2019-01-02T21:35:28Z

Thanks for applying the changes and a special thank for updating the commits instead of adding new commits with small corrections. I like the clean history.

I have one more small thing - will post an inline comment.

hugbug · 2019-01-02T21:42:57Z

daemon/postprocess/PrePostProcessor.cpp

+ else
+ {
+ // Wait until we get the stop signal or more jobs in the queue
+ std::unique_lock<std::mutex> lk(m_pauseMutex);


Should probably be:

UniqueLock lk(m_pauseMutex);

Maybe also better to use name guard just like in all other places? Not sure about that though.

Consequently m_pauseMutex should be declared as Mutex instead of std::mutex.

And similarly in FeedCoordinator.cpp/.h.

Maybe also better to use name guard just like in all other places? Not sure about that though.

Yeah, I thought about that but as the type is std::unique_lock lock I left it like that because it better matches the real type.

Changed the rest.

daemon/postprocess/PrePostProcessor.h

Before only m_threadCount was protected by a mutex but not the rest of the bools that are read/written from different threads. This replaces them by atomic values removing the need for the mutex and protecting the previously unprotected bools, except for m_autoDestroy that it's only set before starting the thread.

Simplify thread handling by using std::thread.

When in deamon mode, just wait for the Stop signal instead of looping constantly, in a way that doesn't affect responsiveness.

Wait until new jobs or a Stop signal instead of looping, in a way that doesn't reduce responsiveness.

Loop every second waiting on a condition variable to reduce the number of CPU wake ups and keep responsiveness.

hugbug · 2019-01-03T23:29:19Z

I've run into an issue when compiling for linux installer package (cross compiling for many CPU architectures).

The compilation for armel breaks with:

daemon/main/nzbget.cpp:226:21: error: field ‘m_stopSignal’ has incomplete type ‘std::promise<void>’
  std::promise<void> m_stopSignal;
                     ^
In file included from daemon/main/nzbget.h:229:0,
                 from daemon/main/nzbget.cpp:22:
/mnt/build/toolchain/armel/output/host/usr/arm-buildroot-linux-uclibcgnueabi/include/c  /5.2.0/future:124:11: note: declaration of ‘class std::promise<void>’
     class promise;
           ^

After searching found this explanation. In short (read the link for details):

any C 11 program linked to libstdc which uses future facilities or transfer exceptions will fail to compile on armel.

Do you have an alternative solution without promise/future maybe? If you don't I would need to ifdef promise/future and need to extend the configure-script with detection of promise/future-support.

hugbug · 2019-01-04T01:28:30Z

I think we can use std::condition_variable here. I'll try this as an exercise.

fedux · 2019-01-04T10:42:34Z

Yes, it can be replaced by condition_variable, but it's a shame that it doesn't work for armel because that means we won't be able to use async or packed_task either probably. I'll see what I can find about it.

fedux · 2019-01-04T11:01:32Z

Apparently it's already fixed in gcc https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64735

Which compiler version is failing?

hugbug · 2019-01-04T11:59:30Z

It's GCC 5.2.

Even though the problem seems to be fixed in GCC 7.0 I cannot easily drop support of older GCC versions. Armel is an important platform. It was a pain for some users when I migrated to C 11/14 with requirement for GCC 4.9. Even GCC 4.8 supported (but only when compiling in release mode) and I intentionally don't use some C features not available on GCC 4.8 in order to maintain compatibility.

Changing system requirements of NZBGet to higher GCC version is possible but it has to bring a big advantage to the project. A simple "there is one place where we could use a new compiler feature" isn't such case. The switch from C 98 to C 11/14 was such case.

If promise/future or async or packed_task will allow to for example increase performance to 10% then the switch may be worth it. If these features just allow to save a couple of lines of code then no.

Maintaining compatibility with many platforms is indeed painful and has costs.

fedux · 2019-01-04T14:46:36Z

debian has backported it to gcc-6 but I see your point. It's a shame because it's not really a requirement of a higher c standard but a bug in gcc.

In any case, it's not a big deal to use condition_variable instead.

in GCC 5 for armel. Promise/future are not supported there and were replaced with condition_variable.

due to race condition

hugbug reviewed Dec 30, 2018

View reviewed changes

daemon/queue/DownloadInfo.h Outdated Show resolved Hide resolved

hugbug reviewed Dec 30, 2018

View reviewed changes

daemon/util/Thread.cpp Show resolved Hide resolved

hugbug reviewed Jan 1, 2019

View reviewed changes

daemon/util/Thread.h Outdated Show resolved Hide resolved

hugbug reviewed Jan 2, 2019

View reviewed changes

daemon/feed/FeedCoordinator.h Outdated Show resolved Hide resolved

fedux added 3 commits January 2, 2019 20:46

Use std::mutex instead of custom class Mutex

b29131f

Basically this is just removing the custom class and using a typedef to keep the name. Most of the changes are just case for the lock/unlock methods.

Replace m_threadMutex by static Mutex

fb3a27f

Just use Mutex and remove the need to call Thread::Init()

Replace custom Guard class by std::lock_guard and std::unique_lock

2c85def

Use a typedef to maintain the name and use Guard for simple scoped lock guards and UniqueLock when it needs to be movable.

hugbug reviewed Jan 2, 2019

View reviewed changes

daemon/postprocess/PrePostProcessor.h Outdated Show resolved Hide resolved

fedux added 5 commits January 3, 2019 12:27

Use std::thread instead of platform implementation

49e8fea

Simplify thread handling by using std::thread.

Pause DoMainLoop until stop/reload signal in daemon mode

794f240

When in deamon mode, just wait for the Stop signal instead of looping constantly, in a way that doesn't affect responsiveness.

Pause PrePostProcessor::Run thread using condition variables

1f3067c

Wait until new jobs or a Stop signal instead of looping, in a way that doesn't reduce responsiveness.

Pause FeedCoordination::Run thread for 1 sec using condition variables

85995ad

Loop every second waiting on a condition variable to reduce the number of CPU wake ups and keep responsiveness.

hugbug merged commit 009cf9e into nzbget:develop Jan 3, 2019

hugbug mentioned this pull request Jan 3, 2019

Decrease idle cpu/power usage #351

Closed

fedux deleted the idle-pr branch January 3, 2019 15:08

hugbug added a commit that referenced this pull request Jan 3, 2019

#593: use platform independent type

0602e9d

hugbug added a commit that referenced this pull request Jan 4, 2019

#593: 794f240: fixed compiling error

2cb4196

in GCC 5 for armel. Promise/future are not supported there and were replaced with condition_variable.

hugbug mentioned this pull request Jan 5, 2019

Incompatibility issues caused by #593 #597

Closed

14 tasks

hugbug added a commit that referenced this pull request Jan 21, 2019

#593: 794f240: fixed potential lock up

05c8418

due to race condition

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Idle CPU usage improvements #593

Idle CPU usage improvements #593

fedux commented Dec 30, 2018

hugbug commented Dec 30, 2018

hugbug commented Jan 2, 2019

hugbug Jan 2, 2019

fedux Jan 3, 2019

hugbug commented Jan 3, 2019

hugbug commented Jan 4, 2019

fedux commented Jan 4, 2019

fedux commented Jan 4, 2019

hugbug commented Jan 4, 2019

fedux commented Jan 4, 2019

Idle CPU usage improvements #593

Idle CPU usage improvements #593

Conversation

fedux commented Dec 30, 2018

hugbug commented Dec 30, 2018

hugbug commented Jan 2, 2019

hugbug Jan 2, 2019

Choose a reason for hiding this comment

fedux Jan 3, 2019

Choose a reason for hiding this comment

hugbug commented Jan 3, 2019

hugbug commented Jan 4, 2019

fedux commented Jan 4, 2019

fedux commented Jan 4, 2019

hugbug commented Jan 4, 2019

fedux commented Jan 4, 2019