New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idle CPU usage improvements #593

Merged
merged 8 commits into from Jan 3, 2019

Conversation

Projects
None yet
2 participants
@fedux
Copy link
Contributor

fedux commented Dec 30, 2018

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

This comment has been minimized.

Copy link
Member

hugbug commented Dec 30, 2018

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

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

@fedux fedux force-pushed the fedux:idle-pr branch from bada6ca to 173274a Jan 1, 2019

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

@fedux fedux force-pushed the fedux:idle-pr branch from 173274a to 6e52d30 Jan 2, 2019

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

fedux added some commits Dec 23, 2018

Use std::mutex instead of custom class Mutex
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
Just use Mutex and remove the need to call Thread::Init()
Replace custom Guard class by std::lock_guard and std::unique_lock
Use a typedef to maintain the name and use Guard for simple scoped lock
guards and UniqueLock when it needs to be movable.

@fedux fedux force-pushed the fedux:idle-pr branch from 6e52d30 to 016438a Jan 2, 2019

@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 2, 2019

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.

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

This comment has been minimized.

@hugbug

hugbug Jan 2, 2019

Member

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.

This comment has been minimized.

@fedux

fedux Jan 3, 2019

Author Contributor

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.

fedux added some commits Dec 30, 2018

Use std::atomic for Thread class members
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.
Use std::thread instead of platform implementation
Simplify thread handling by using std::thread.
Pause DoMainLoop until stop/reload signal in daemon mode
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
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
Loop every second waiting on a condition variable to reduce the number
of CPU wake ups and keep responsiveness.

@fedux fedux force-pushed the fedux:idle-pr branch from 016438a to 85995ad Jan 3, 2019

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fedux fedux deleted the fedux:idle-pr branch Jan 3, 2019

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

@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Member

hugbug commented Jan 4, 2019

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

@fedux

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 4, 2019

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

Which compiler version is failing?

@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 4, 2019

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.

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

#593: 794f240: fixed compiling error
in GCC 5 for armel. Promise/future are not supported there and were
replaced with condition_variable.

@hugbug hugbug referenced this pull request Jan 5, 2019

Closed

Incompatibility issues caused by #593 #597

14 of 14 tasks complete

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

#593: 794f240: fixed potential lock up
due to race condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment