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

Incompatibility issues caused by #593 #597

Closed
hugbug opened this Issue Jan 5, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@hugbug
Copy link
Member

hugbug commented Jan 5, 2019

With PR #593 by @fedux the code for creating and synchronising threads has been reworked to use standard facilities of C++11: std::thread, std::mutex, std::atomic, std::condition_variable. That was a nice and welcoming change allowing to reduce the amount of OS specific code in NZBGet code base.

All seemed fine: compilation worked on Mac, Windows and on Linux (tested via Travis CI). Compatibility with many systems is a major priority for NZBGet project. To catch potential compatibility issues we have Travis CI tests that compile nzbget using GCC 4.8 (among other versions), which is the oldest supported compiler. These tests succeeded.

After merging PR #593 I was making performance tests on ARM platform (ARMv7 CPU, armhf target). Unfortunately that didn't go well. The program crashed on start. After investigating the issue the problematic places were:

  • creating new threads with std::thread
  • working with std::condition_variable.

In both cases the library code throwed exception std::system_error and terminated the program.

Before that another issue was found: the compilation for armel target failed. In that case the compiler feature promise/future wasn't implemented in GCC for armel target. That particular bug seemed to be fixed in newer versions of GCC. The issue in nzbget was fixed by reworking the code to use std::condition_variable instead of std::promise/std::future.

I could possibly update build machine to newer GCC version which would hopefully fix issues with std::thread and std::condition_variable. However the fact that GCC 5.2 can't compile nzbget successfully is frustrating. Our Linux installer provides binaries for popular platforms but not for all CPUs. There are sure other devices whose users need to compile nzbget themselves. Many devices come with old toolchains.

Increasing system requirements of nzbget must be adjusted with a very big gain. The rework of thread management code was initiated by #351. Decrease idle cpu/power usage has however far less importance than compatibility with many platforms. Moreover the changes in #593 which directly address idle CPU usage rely on only one new (previously not available in nzbget code base) thread management facility - condition variables.

The plan to fix compatibility issues but keep improvements addressing idle CPU usage is: revert all threads management code back (use OS specific code) and implement condition variables using OS-specific code.

Tasks:

  • revert changes to unit Thread.cpp/.h
  • implement condition variable class for POSIX
  • implement condition variable class for Windows (Windows Vista provides system facility "condition variable" which we can use. For Windows XP an alternative implementation is needed):
    • Windows Vista and above
    • Windows XP
  • replace usage of std::condition_variable with new class
  • test compilation of Linux installer targets
  • test on all available platforms including:
    • Mac
    • Linux ARMv7
    • Windows 10
    • Windows XP
    • Android ARM
    • FreeBSD x86_64.

@hugbug hugbug added the testing-bug label Jan 5, 2019

@hugbug hugbug added this to the v21 milestone Jan 5, 2019

hugbug added a commit that referenced this issue Jan 6, 2019

#597: reverted changes to Thread-unit
Due to compatibility issues on older platforms (issues discovered on
ARMv7 with GCC 5.2 but may not be limited to this platform) the usage
of C++11 thread- and synchronisation facilities has been reverted to
previous custom OS-specific implementation.

hugbug added a commit that referenced this issue Jan 6, 2019

#597: implemented OS-specific condition variable class
Currently for POSIX only; Windows implementation follows.

hugbug added a commit that referenced this issue Jan 6, 2019

@fedux

This comment has been minimized.

Copy link
Contributor

fedux commented Jan 6, 2019

After merging PR #593 I was making performance tests on ARM platform (ARMv7 CPU, armhf target). Unfortunately that didn't go well. The program crashed on start. After investigating the issue the problematic places were:

  • creating new threads with std::thread
  • working with std::condition_variable.

In both cases the library code throwed exception std::system_error and terminated the program.

Maybe this https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1228201 ?

In any case, my interest in working in this project was to solve the CPU idle usage and learn/practice the C++11/14 features at the same time, but now that these features can't be used it makes more sense for me to continue work in my own fork. I will try to send you any fixes that I see while at that. :)

hugbug added a commit that referenced this issue Jan 6, 2019

#597: implemented condition variable class for Windows
works on Windows Vista and newer.

hugbug added a commit that referenced this issue Jan 6, 2019

#597: implemented condition variable class for Windows
works on Windows Vista and newer.

hugbug added a commit that referenced this issue Jan 6, 2019

#597: use std::mutex and std::condition_variable on Windows
That’s the easiest way to get compatibility with Windows XP yet better
performance on Windows Vista and above.

hugbug added a commit that referenced this issue Jan 7, 2019

#597: use std::mutex and std::condition_variable on all platforms
wrapped them in custom classes for easier replacements, just in case.

hugbug added a commit that referenced this issue Jan 7, 2019

@hugbug

This comment has been minimized.

Copy link
Member Author

hugbug commented Jan 7, 2019

Maybe this https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1228201 ?

I wasn't getting the message "Enable multithreading to use std::thread: Operation not permitted" and the flags didn't help. But the last post mentioned static linking and that got my attention because static linking is usually troublesome. I then checked if x86_64-build worked and it didn't. So the problem wasn't specific to ARM-target and I knew for sure that x86_64 should not have troubles with std::thread.

After further googling I found this post. Apparently linking std::thread statically requires some magic compiler flags:

-Wl,--whole-archive -lpthread -Wl,--no-whole-archive

GCC devs think:

This is not a GCC or glibc bug.

😢
I would think if GCC got flags-static and -pthread it could figure out on its own how to link properly.

Anyway with the new flags the targets x86_64 and armhf started to work!
I can't test other targets unfortunately.

With the hope that all other platforms work as well I've brought back std::thread, std::mutex and std::condition_variable. This time I decided to keep interface part of Thread-unit unchanged. That means Guard, Mutex and new ConditionVar are used through the rest of the code and they are wrappers for std-classes instead of simple typedefs. That way I'm prepared for easier replacement of std-classes in a case a problem is encountered again (hopefully not). BTW I also preferred general/simple interface of Guard and glad to have it back actually :).

I need to do more tests on all available platforms, just to be sure, before merging into develop.

@fedux: Thank you for the hint and for all original changes regarding idle CPU usage. You whole work is preserved, don't worry! I'm also going to merge your changes to article cache manager. Although you closed the PR I still have access to changes; maybe you can post a new PR if you want, once branch 597-concurrency (where I make current changes) is merged into develop.

@fedux

This comment has been minimized.

Copy link
Contributor

fedux commented Jan 8, 2019

Anyway with the new flags the targets x86_64 and armhf started to work!

🎉

I have several other changes in my fork but I'm trying to only do internal changes and keep it compatible with your main work. Eventually I will probably get bored but you will continue with the main project. Some of the changes could be cherry-picked and maybe you could also get some ideas.

Anyway, I'll do the new PR after your changes. 👍

@hugbug

This comment has been minimized.

Copy link
Member Author

hugbug commented Jan 9, 2019

Merged.

@hugbug hugbug closed this Jan 9, 2019

fedux added a commit to fedux/nzbget that referenced this issue Jan 12, 2019

hugbug added a commit that referenced this issue Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment