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

Std thread #13721

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@jimfcarroll
Member

jimfcarroll commented Mar 31, 2018

This change moves the base functionality for CThread to std::thread.

Description

This change moves the base functionality for CThread to std::thread. It introduces a proper Join function and thereby solves an intermittent crash on exit. Please note:

@FernetMenta , @MrMC This change removes the custom programmatic setting of the per-thread stacksize. That code was originally added as a result of a JSON parsing crash. This PR also fixes that original JSON parsing problem by setting the default parser to be iterative rather than recursive. See the discussion here for more information: MrMC/mrmc@441d1b4

Motivation and Context

This is a step toward moving toward implementing an Executor service to be used to replace the Job functionality as well as form the basis for message passing in a more "actor" based software design.

How Has This Been Tested?

Tests were added and all tests still run.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@jimfcarroll jimfcarroll requested review from FernetMenta, fritsch and da-anda Mar 31, 2018

@ghost

This comment has been minimized.

ghost commented Mar 31, 2018

if (lthread->joinable())
m_thread->join();
return true;
} else

This comment has been minimized.

@FernetMenta

FernetMenta Mar 31, 2018

Member

this is none of your Java code :)
please check entire pr, there is more incorrect coding style

iTime *= 10000; // convert into 100ns tics
// only update every 1 second
if( iTime < m_iLastTime + 1000*10000 ) return m_fLastUsage;

This comment has been minimized.

@FernetMenta

FernetMenta Mar 31, 2018

Member

only single expression on a line

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Mar 31, 2018

Okay. I fixed the code style issues. I also swapped my last commit for @MrMC 's which, as he noted, contains some pertinent comments

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Mar 31, 2018

This needs proper runtime testing with looking at thread priorities. I will do some tests over the weekend.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Mar 31, 2018

Okay. The goal was to leave the priority functionality intact so if I screwed something up along those lines, let me know.

@da-anda da-anda removed their request for review Mar 31, 2018

@da-anda

This comment has been minimized.

Member

da-anda commented Mar 31, 2018

I'm happy to test if this fixes the (rather frequent) crashes on my PI, if @MilhouseVH is including this PR in his nightlies. But due to lack of any lower level C(++) knowledge, I unfortunately can't do the requested code review.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 1, 2018

First test on Linux: thread names are gone

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 1, 2018

Okay. Thanks. Looking now.

@Rechi

This comment has been minimized.

Member

Rechi commented Apr 1, 2018

@jimfcarroll looks like you haven't rebased on #13569 properly.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 1, 2018

Yup. Thanks @Rechi that's it. Working on it now. I ran across another issue. I should have a fix up shortly.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 1, 2018

Okay. It should all be resolved. I checked that the threads were named by pausing the debugger.

@@ -1,4 +1,3 @@
set(HEADERS ThreadImpl.h)
if(NOT CORE_SYSTEM_NAME STREQUAL windowsstore)
set(SOURCES Win32Exception.cpp)
set(HEADERS ${HEADERS} Win32Exception.h)

This comment has been minimized.

@Rechi

Rechi Apr 1, 2018

Member

${HEADERS} should be removed

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 1, 2018

Thread priorities are wrong. I started kodi with nice -1. The only thread with prio is AESink that should get nice of -2. ActiveAE has nice -2 and no background priorities for thread like peripherials

image

Should be:
image

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 1, 2018

@FernetMenta So priorities are being set (ActiveAE has a higher priority than the default), but the particular thread has the wrong priority (AESink, not ActiveAE should have a non-default higher priority)? I'm not sure I could have done that intentionally if I wanted to by changing things at the base level. :-) What tool are you using to inspect LWP priorities and hierarchies?

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 1, 2018

I am using htop

Not only AESink has wrong prio. Note that SetPriority may be called from other threads! That's why the current code has a lock that you removed. Unintentionally ofc :)

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 1, 2018

thread priorities higher then 0 (negative values) won't work on 99% of linux systems, because those are not allowed by default, so we should not use them. 0 is low enough.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 1, 2018

Kodi's thread priorities are relative to the priority of the app.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 2, 2018

my point is that trying to set a prio of -1 is not only going to fail, in some cases (e.g. snap packages) this causes kodi to be killed by OS security layers. If you insist on using negative values, this should be configurable. At least as a cmake option.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 2, 2018

we don't set anything that is going to fail.

EDIT: this code has been running for many years. please point me to a single case where anything related to thread priorities failed.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 2, 2018

We do, under certain circumstances. As said above, e.g. snap packages enforce strict security rules with appparmor and seccomp. Even using a forbidden syscall causes your app to be killed. And one such rule is that negative thread priorities are not allowed.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 2, 2018

I'll make sure it works the same way it used to. Thanks.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 2, 2018

the way it used to work is a problem. We should never try to set a negative prio(we currently do without this PR).

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 2, 2018

@wsnipex this is nonsense. we don't set higher prio than the system allows for a user.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 2, 2018

@FernetMenta you aren't listening and obviously are not familiar how seccomp works. If doesn't care about any /etc/security/limits.conf settings.
If you still don't believe me, configure a seccomp filter for kodi with the following rule, which snap uses:

setpriority PRIO_PROCESS 0 <=19

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Apr 2, 2018

In any case, I had no intention of changing the behavior with this PR (with the one exception of removing the ability to set the stack size). I certainly don't mind following this up with another change based on whatever you guys work out. I've never had much cause for changing priorities in my day-job so I'm not really qualified to comment, not that that always stops me :-)

@afedchin afedchin referenced this pull request Apr 4, 2018

Merged

Move all platform specific code to platform's folders. #13739

2 of 10 tasks complete
{
CSingleLock l(m_CriticalSection);
std::thread* lthread = m_thread;
if (lthread != nullptr) {

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

curly bracket on new line

int64_t iUsage = GetAbsoluteUsage();
if (m_iLastUsage > 0 && m_iLastTime > 0)
m_fLastUsage = (float)( iUsage - m_iLastUsage ) / (float)( iTime - m_iLastTime );

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

c++ style casts

/* This shouldn't be very busy and timing is important so increase priority */
CThread::GetCurrentThread()->SetPriority(CThread::GetCurrentThread()->GetPriority()+1);
CThread thread = CThread::GetCurrentThread();
if (thread != nullptr) {

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

curly bracket on new line

return 0;
bool CThread::IsRunning() const
{
return m_thread != nullptr ? true : false;

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

return m_thread != nullptr; does the same

//////////////////////////////////////////////////////////////////////
// Construction/Destruction
//////////////////////////////////////////////////////////////////////
CThread::CThread(const char* ThreadName)
: m_StopEvent(true,true), m_TermEvent(true), m_StartEvent(true)
: m_StopEvent(true,true), m_StartEvent(true), m_thread(nullptr), m_lwpId(0)

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

default member initialisation is preferred

@@ -56,12 +57,11 @@ CThread::CThread(const char* ThreadName)
}
CThread::CThread(IRunnable* pRunnable, const char* ThreadName)
: m_StopEvent(true,true), m_TermEvent(true), m_StartEvent(true)
: m_StopEvent(true,true), m_StartEvent(true), m_thread(nullptr), m_lwpId(0)

This comment has been minimized.

@Rechi

Rechi May 13, 2018

Member

default member initialisation is preferred

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented May 13, 2018

jenkins build this please

@@ -45,8 +45,7 @@ void lock_callback(int mode, int type, const char* file, int line)
unsigned long thread_id()
{
// C-style cast required due to vastly differing native ID return types

This comment was marked as outdated.

@Rechi

Rechi May 13, 2018

Member

still valid

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented May 14, 2018

jenkins build this please

if (stacksize > PTHREAD_STACK_MIN)
pthread_attr_setstacksize(&attr, stacksize);
#ifdef TARGET_FREEBSD
#if __FreeBSD_version < 900031

This comment has been minimized.

@Rechi

Rechi May 14, 2018

Member

no need for this, FreeBSD 9.0 is already EOL
seems like you reintroduced this during rebasing, I already removed it at e658aa1 from master

@@ -214,7 +214,7 @@ bool CLog::WriteLogString(int logLevel, const std::string& logString)
minute,
second,
static_cast<int>(millisecond),
(uint64_t)CThread::GetCurrentThreadId(),
static_cast<uint64_t>(CThread::GetCurrentThreadNativeHandle()),

This comment has been minimized.

@Rechi

Rechi May 14, 2018

Member

c-style cast is here needed too

This comment has been minimized.

@ksooo

ksooo May 14, 2018

Member

Instead of adding the c-style cast everywhere, shouldn't this be done in CThread::GetCurrentThreadNativeHandle()?

This comment has been minimized.

@Rechi

Rechi May 14, 2018

Member

What type should CThread::GetCurrentThreadNativeHandle return?

This comment has been minimized.

@ksooo

ksooo May 14, 2018

Member

uint64_t?

This comment has been minimized.

@jimfcarroll

jimfcarroll May 14, 2018

Member

I actually like that. I suggested it in the channel. I'll put something out there tonight.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Jul 7, 2018

I haven't had time to work on this (I have a new job that's been keeping me busy). I'm going to close this for now so it doesn't stand out there and take people's time.

@jimfcarroll jimfcarroll closed this Jul 7, 2018

@hudokkow

This comment has been minimized.

Member

hudokkow commented Jul 7, 2018

No need to close it. Just stick an On-Hold label to it.

@jimfcarroll

This comment has been minimized.

Member

jimfcarroll commented Jul 17, 2018

Done. Thanks.

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