Thank you for the patches. The Slim RW locks should be a performance improvement from the documentation. I'm not so sure about the POSIX RWlocks though. Did you do any performance testing - does the change in behaviour of the reconRow and framefilter locks improve performance?
On Sun, Apr 3, 2016 at 2:00 PM, Ximing Cheng <[email protected]> wrote: > # HG changeset patch > # User Ximing Cheng <[email protected]> > # Date 1459672199 -28800 > # Sun Apr 03 16:29:59 2016 +0800 > # Node ID 601877ef465c549efe24063afa0479a39e369010 > # Parent 5b01678f6fb4e89e23cd41295592a9aa5d51d4ba > ThreadSafeInteger: change default lock into read write lock > > diff -r 5b01678f6fb4 -r 601877ef465c source/common/frame.cpp > --- a/source/common/frame.cpp Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/common/frame.cpp Sun Apr 03 16:29:59 2016 +0800 > @@ -30,6 +30,7 @@ > > Frame::Frame() > { > + m_reconRowCount.init(); > m_bChromaExtended = false; > m_lowresInit = false; > m_reconRowCount.set(0); > @@ -55,6 +56,8 @@ > X265_CHECK((m_reconColCount == NULL), "m_reconColCount was > initialized"); > m_numRows = (m_fencPic->m_picHeight + g_maxCUSize - 1) / > g_maxCUSize; > m_reconColCount = new ThreadSafeInteger[m_numRows]; > + for (int i = 0; i < m_numRows; i++) > + m_reconColCount[i].init(true); > > if (quantOffsets) > { > diff -r 5b01678f6fb4 -r 601877ef465c source/common/threading.h > --- a/source/common/threading.h Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/common/threading.h Sun Apr 03 16:29:59 2016 +0800 > @@ -174,73 +174,127 @@ > class ThreadSafeInteger > { > public: > - > - ThreadSafeInteger() > + /* useReadWriteLock is useless, just be compatible with pthread > version */ > + void init(bool useReadWriteLock = true) > { > + /* disable MSVC warnnings */ > + (void)useReadWriteLock; > m_val = 0; > + InitializeConditionVariable(&m_cv); > + /* Slim Reader/Writer (SRW) Locks minimum supported in Windows > Vista */ > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > InitializeCriticalSection(&m_cs); > - InitializeConditionVariable(&m_cv); > +#else > + InitializeSRWLock(&m_rwlock); > +#endif > } > > ~ThreadSafeInteger() > { > + /* SRW locks do not need to be explicitly destroyed */ > +#if defined(_WIN32_WINNT) && _WIN32_WINNT < _WIN32_WINNT_VISTA > DeleteCriticalSection(&m_cs); > +#endif > XP_CONDITION_VAR_FREE(&m_cv); > } > > int waitForChange(int prev) > { > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > - if (m_val == prev) > + while (m_val == prev) > SleepConditionVariableCS(&m_cv, &m_cs, INFINITE); > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockShared(&m_rwlock); > + while (m_val == prev) > + SleepConditionVariableSRW(&m_cv, &m_rwlock, INFINITE, > CONDITION_VARIABLE_LOCKMODE_SHARED); > + ReleaseSRWLockShared(&m_rwlock); > +#endif > return m_val; > } > > int get() > { > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > int ret = m_val; > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockShared(&m_rwlock); > + int ret = m_val; > + ReleaseSRWLockShared(&m_rwlock); > +#endif > return ret; > } > > int getIncr(int n = 1) > { > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > int ret = m_val; > m_val += n; > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockExclusive(&m_rwlock); > + int ret = m_val; > + m_val += n; > + ReleaseSRWLockExclusive(&m_rwlock); > +#endif > return ret; > } > > void set(int newval) > { > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > m_val = newval; > WakeAllConditionVariable(&m_cv); > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockExclusive(&m_rwlock); > + m_val = newval; > + WakeAllConditionVariable(&m_cv); > + ReleaseSRWLockExclusive(&m_rwlock); > +#endif > } > > void poke(void) > { > /* awaken all waiting threads, but make no change */ > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > WakeAllConditionVariable(&m_cv); > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockExclusive(&m_rwlock); > + WakeAllConditionVariable(&m_cv); > + ReleaseSRWLockExclusive(&m_rwlock); > +#endif > } > > void incr() > { > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > EnterCriticalSection(&m_cs); > m_val++; > WakeAllConditionVariable(&m_cv); > LeaveCriticalSection(&m_cs); > +#else > + AcquireSRWLockExclusive(&m_rwlock); > + m_val++; > + WakeAllConditionVariable(&m_cv); > + ReleaseSRWLockExclusive(&m_rwlock); > +#endif > } > > protected: > > +#if _WIN32_WINNT < _WIN32_WINNT_VISTA > CRITICAL_SECTION m_cs; > +#else > + SRWLOCK m_rwlock; > +#endif > CONDITION_VARIABLE m_cv; > int m_val; > }; > @@ -369,27 +423,38 @@ > class ThreadSafeInteger > { > public: > - > - ThreadSafeInteger() > + /* pthread_cond_wait only accept mutex param */ > + void init(bool useReadWriteLock = false) > { > + m_useReadWriteLock = useReadWriteLock; > m_val = 0; > - if (pthread_mutex_init(&m_mutex, NULL) || > - pthread_cond_init(&m_cond, NULL)) > + if (!m_useReadWriteLock && (pthread_mutex_init(&m_mutex, NULL) || > + pthread_cond_init(&m_cond, NULL))) > { > x265_log(NULL, X265_LOG_ERROR, "fatal: unable to initialize > conditional variable\n"); > } > + else if (m_useReadWriteLock && pthread_rwlock_init(&m_rwlock, > NULL)) > + { > + x265_log(NULL, X265_LOG_ERROR, "fatal: unable to initialize > read write lock\n"); > + } > } > > ~ThreadSafeInteger() > { > - pthread_cond_destroy(&m_cond); > - pthread_mutex_destroy(&m_mutex); > + if (!m_useReadWriteLock) > + { > + pthread_cond_destroy(&m_cond); > + pthread_mutex_destroy(&m_mutex); > + } > + else > + pthread_rwlock_destroy(&m_rwlock); > } > > int waitForChange(int prev) > { > + X265_CHECK(!m_useReadWriteLock, "ThreadSafeInteger with > waitForChange should disable read write lock!\n"); > pthread_mutex_lock(&m_mutex); > - if (m_val == prev) > + while (m_val == prev) > pthread_cond_wait(&m_cond, &m_mutex); > pthread_mutex_unlock(&m_mutex); > return m_val; > @@ -397,31 +462,62 @@ > > int get() > { > - pthread_mutex_lock(&m_mutex); > - int ret = m_val; > - pthread_mutex_unlock(&m_mutex); > + int ret; > + if (!m_useReadWriteLock) > + { > + pthread_mutex_lock(&m_mutex); > + ret = m_val; > + pthread_mutex_unlock(&m_mutex); > + } > + else > + { > + pthread_rwlock_rdlock(&m_rwlock); > + ret = m_val; > + pthread_rwlock_unlock(&m_rwlock); > + } > return ret; > } > > int getIncr(int n = 1) > { > - pthread_mutex_lock(&m_mutex); > - int ret = m_val; > - m_val += n; > - pthread_mutex_unlock(&m_mutex); > + int ret; > + if (!m_useReadWriteLock) > + { > + pthread_mutex_lock(&m_mutex); > + ret = m_val; > + m_val += n; > + pthread_mutex_unlock(&m_mutex); > + } > + else > + { > + pthread_rwlock_wrlock(&m_rwlock); > + ret = m_val; > + m_val += n; > + pthread_rwlock_unlock(&m_rwlock); > + } > return ret; > } > > void set(int newval) > { > - pthread_mutex_lock(&m_mutex); > - m_val = newval; > - pthread_cond_broadcast(&m_cond); > - pthread_mutex_unlock(&m_mutex); > + if (!m_useReadWriteLock) > + { > + pthread_mutex_lock(&m_mutex); > + m_val = newval; > + pthread_cond_broadcast(&m_cond); > + pthread_mutex_unlock(&m_mutex); > + } > + else > + { > + pthread_rwlock_wrlock(&m_rwlock); > + m_val = newval; > + pthread_rwlock_unlock(&m_rwlock); > + } > } > > void poke(void) > { > + X265_CHECK(!m_useReadWriteLock, "ThreadSafeInteger with poke > should disable read write lock!\n"); > /* awaken all waiting threads, but make no change */ > pthread_mutex_lock(&m_mutex); > pthread_cond_broadcast(&m_cond); > @@ -430,17 +526,28 @@ > > void incr() > { > - pthread_mutex_lock(&m_mutex); > - m_val++; > - pthread_cond_broadcast(&m_cond); > - pthread_mutex_unlock(&m_mutex); > + if (!m_useReadWriteLock) > + { > + pthread_mutex_lock(&m_mutex); > + m_val++; > + pthread_cond_broadcast(&m_cond); > + pthread_mutex_unlock(&m_mutex); > + } > + else > + { > + pthread_rwlock_wrlock(&m_rwlock); > + m_val++; > + pthread_rwlock_unlock(&m_rwlock); > + } > } > > protected: > > - pthread_mutex_t m_mutex; > - pthread_cond_t m_cond; > - int m_val; > + pthread_mutex_t m_mutex; > + pthread_rwlock_t m_rwlock; > + pthread_cond_t m_cond; > + int m_val; > + bool m_useReadWriteLock; > }; > > #endif // ifdef _WIN32 > diff -r 5b01678f6fb4 -r 601877ef465c source/common/threadpool.h > --- a/source/common/threadpool.h Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/common/threadpool.h Sun Apr 03 16:29:59 2016 +0800 > @@ -129,7 +129,7 @@ > int m_jobTotal; > int m_jobAcquired; > > - BondedTaskGroup() { m_bondedPeerCount = m_jobTotal = m_jobAcquired = > 0; } > + BondedTaskGroup() { m_bondedPeerCount = m_jobTotal = m_jobAcquired = > 0; m_exitedPeerCount.init(); } > > /* Do not allow the instance to be destroyed before all bonded peers > have > * exited processTasks() */ > diff -r 5b01678f6fb4 -r 601877ef465c source/encoder/framefilter.h > --- a/source/encoder/framefilter.h Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/encoder/framefilter.h Sun Apr 03 16:29:59 2016 +0800 > @@ -82,6 +82,9 @@ > , m_encData(NULL) > , m_prevRow(NULL) > { > + m_lastCol.init(true); > + m_allowedCol.init(true); > + m_lastDeblocked.init(true); > } > > ~ParallelFilter() > diff -r 5b01678f6fb4 -r 601877ef465c source/encoder/ratecontrol.cpp > --- a/source/encoder/ratecontrol.cpp Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/encoder/ratecontrol.cpp Sun Apr 03 16:29:59 2016 +0800 > @@ -166,6 +166,7 @@ > > RateControl::RateControl(x265_param& p) > { > + m_startEndOrder.init(); > m_param = &p; > int lowresCuWidth = ((m_param->sourceWidth / 2) + X265_LOWRES_CU_SIZE > - 1) >> X265_LOWRES_CU_BITS; > int lowresCuHeight = ((m_param->sourceHeight / 2) + > X265_LOWRES_CU_SIZE - 1) >> X265_LOWRES_CU_BITS; > diff -r 5b01678f6fb4 -r 601877ef465c source/input/y4m.cpp > --- a/source/input/y4m.cpp Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/input/y4m.cpp Sun Apr 03 16:29:59 2016 +0800 > @@ -43,6 +43,8 @@ > > Y4MInput::Y4MInput(InputFileInfo& info) > { > + readCount.init(); > + writeCount.init(); > for (int i = 0; i < QUEUE_SIZE; i++) > buf[i] = NULL; > > diff -r 5b01678f6fb4 -r 601877ef465c source/input/yuv.cpp > --- a/source/input/yuv.cpp Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/input/yuv.cpp Sun Apr 03 16:29:59 2016 +0800 > @@ -41,6 +41,8 @@ > > YUVInput::YUVInput(InputFileInfo& info) > { > + readCount.init(); > + writeCount.init(); > for (int i = 0; i < QUEUE_SIZE; i++) > buf[i] = NULL; > > diff -r 5b01678f6fb4 -r 601877ef465c source/output/reconplay.cpp > --- a/source/output/reconplay.cpp Sat Apr 02 19:08:49 2016 +0100 > +++ b/source/output/reconplay.cpp Sun Apr 03 16:29:59 2016 +0800 > @@ -54,7 +54,8 @@ > if (signal(SIGPIPE, sigpipe_handler) == SIG_ERR) > general_log(¶m, "exec", X265_LOG_ERROR, "Unable to register > SIGPIPE handler: %s\n", strerror(errno)); > #endif > - > + readCount.init(); > + writeCount.init(); > width = param.sourceWidth; > height = param.sourceHeight; > colorSpace = param.internalCsp; > > > _______________________________________________ > x265-devel mailing list > [email protected] > https://mailman.videolan.org/listinfo/x265-devel > -- Deepthi Nandakumar Engineering Manager, x265 Multicoreware, Inc
_______________________________________________ x265-devel mailing list [email protected] https://mailman.videolan.org/listinfo/x265-devel
