From 5883410354c48d30d4d262bf66030c87eaea098b Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 5 Jan 2016 23:30:29 +0100 Subject: [PATCH 1/2] Rework the thread utilities so that: We don't pay for a recursive mutex when we don't need the debugging capabilities, a race condition in the reader/writer locks is removed and the interfaces and (still inlined) implementations are separated for improved readability. --- rtgui/CMakeLists.txt | 2 +- rtgui/guiutils.cc | 5 - rtgui/threadutils.cc | 292 +++++++++++++++ rtgui/threadutils.h | 855 ++++++++++++------------------------------- 4 files changed, 528 insertions(+), 626 deletions(-) create mode 100644 rtgui/threadutils.cc diff --git a/rtgui/CMakeLists.txt b/rtgui/CMakeLists.txt index 5f5dbc1c1..1adc6ddd4 100644 --- a/rtgui/CMakeLists.txt +++ b/rtgui/CMakeLists.txt @@ -21,7 +21,7 @@ set (BASESOURCEFILES preferences.cc profilepanel.cc saveasdlg.cc saveformatpanel.cc soundman.cc splash.cc thumbnail.cc tonecurve.cc toolbar.cc - guiutils.cc zoompanel.cc toolpanelcoord.cc + guiutils.cc threadutils.cc zoompanel.cc toolpanelcoord.cc thumbbrowserentrybase.cc batchqueueentry.cc batchqueue.cc lwbutton.cc lwbuttonset.cc batchqueuebuttonset.cc browserfilter.cc exiffiltersettings.cc diff --git a/rtgui/guiutils.cc b/rtgui/guiutils.cc index 9ca9af3e9..d3af3c597 100644 --- a/rtgui/guiutils.cc +++ b/rtgui/guiutils.cc @@ -29,11 +29,6 @@ using namespace std; -#if TRACE_MYRWMUTEX==1 && !defined NDEBUG -unsigned int MyReaderLock::readerLockCounter = 0; -unsigned int MyWriterLock::writerLockCounter = 0; -#endif - Glib::RefPtr MyExpander::inconsistentPBuf; Glib::RefPtr MyExpander::enabledPBuf; Glib::RefPtr MyExpander::disabledPBuf; diff --git a/rtgui/threadutils.cc b/rtgui/threadutils.cc new file mode 100644 index 000000000..7ba296081 --- /dev/null +++ b/rtgui/threadutils.cc @@ -0,0 +1,292 @@ +/* + * This file is part of RawTherapee. + * + * Copyright (c) 2016 Adam Reichold + * + * RawTherapee is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * RawTherapee is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with RawTherapee. If not, see . + */ +#include "threadutils.h" + +#include +#include + +#ifdef WIN32 +#include +#endif + +#if STRICT_MUTEX && !NDEBUG + +void MyMutex::checkLock () +{ + if (locked) { + std::cerr << "MyMutex already locked!" << std::endl; + +#ifdef WIN32 + DebugBreak (); +#else + raise (SIGTRAP); +#endif + } + + locked = true; +} + +void MyMutex::checkUnlock () +{ + if (!locked) { + std::cerr << "MyMutex already unlocked!" << std::endl; + +#ifdef WIN32 + DebugBreak (); +#else + raise (SIGTRAP); +#endif + } + + locked = false; +} + +#endif + +#if !TRACE_MYRWMUTEX + +void MyReaderLock::acquire () +{ + if (locked) { + return; + } + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + if (mutex.writerCount == 0) { + // There's no writer operating, we can increment the writer count which will lock writers. + ++mutex.writerCount; + } else if (mutex.readerCount == 0) { + // The writer count is non null, but a reader can be the owner of the writer lock, + // which will be the case if the reader count is not zero too. + while (mutex.writerCount != 0) { + mutex.cond.wait(mutex.mutex); + } + + // Then, we can increment the writer count. + ++mutex.writerCount; + } + + // Finally, we can increment the reader count as well. + ++mutex.readerCount; + + locked = true; +} + +void MyReaderLock::release () +{ + if (!locked) { + return; + } + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // decrement the writer number first... + --mutex.readerCount; + + if (mutex.readerCount == 0) { + // ...if no more reader, we decrement the writer count as well... + --mutex.writerCount; + + // ...and signal the next waiting reader/writer that it's free + mutex.cond.broadcast (); + } + + locked = false; +} + +void MyWriterLock::acquire () +{ + if (locked) { + return; + } + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // The writer count is not zero, so we have to wait for it to be zero again... + while (mutex.writerCount != 0) { + mutex.cond.wait (mutex.mutex); + } + + // ...then we can increment the writer count. + ++mutex.writerCount; + + locked = true; +} + +void MyWriterLock::release () +{ + if (!locked) { + return; + } + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // Decrement the writer number first... + if (--mutex.writerCount == 0) { + // ...and if the writer count is zero again, we can wake up the next writer or reader. + mutex.cond.broadcast (); + } + + locked = false; +} + +#else + +namespace +{ + +std::ostream& trace (const char* file, int line) +{ + const auto currentThread = Glib::Threads::Thread::self (); + + return std::cout << currentThread << ":" << file << ":" << line << ": "; +} + +} + +void MyReaderLock::acquire (const char* file, int line) +{ + if (locked) { + trace (file, line) << "MyReaderLock is already locked." << std::endl; + return; + } + + trace (file, line) << "Acquiring MyReaderLock..." << std::endl; + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + if (mutex.writerCount == 0) { + // There's no writer operating, we can increment the writer count which will lock writers. + ++mutex.writerCount; + } else if (mutex.readerCount == 0) { + // The writer count is non null, but a reader can be the owner of the writer lock, + // which will be the case if the reader count is not zero too. + while (mutex.writerCount != 0) { + trace (file, line) << "Waiting for current owner of MyWriterLock..." << std::endl + << "\tOwner thread: " << mutex.ownerThread << std::endl + << "\tLast writer file: " << mutex.lastWriterFile << std::endl + << "\tLast writer line: " << mutex.lastWriterLine << std::endl; + + mutex.cond.wait(mutex.mutex); + } + + // Then, we can increment the writer count. + ++mutex.writerCount; + + mutex.ownerThread = Glib::Threads::Thread::self (); + mutex.lastWriterFile = file; + mutex.lastWriterLine = line; + } + + // Finally, we can increment the reader count as well. + ++mutex.readerCount; + + trace (file, line) << "MyReaderLock is now locked, reader count is " << mutex.readerCount << ", writer count is " << mutex.writerCount << "." << std::endl; + locked = true; +} + +void MyReaderLock::release (const char* file, int line) +{ + if (!locked) { + trace (file, line) << "MyReaderLock is already unlocked." << std::endl; + return; + } + + trace (file, line) << "Releasing MyReaderLock..." << std::endl; + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // decrement the writer number first... + --mutex.readerCount; + + if (mutex.readerCount == 0) { + // ...if no more reader, we decrement the writer count as well... + --mutex.writerCount; + + // ...and signal the next waiting reader/writer that it's free + mutex.cond.broadcast (); + + mutex.ownerThread = nullptr; + mutex.lastWriterFile = ""; + mutex.lastWriterLine = 0; + } + + trace (file, line) << "MyReaderLock is now unlocked, reader count is " << mutex.readerCount << ", writer count is " << mutex.writerCount << "." << std::endl; + locked = false; +} + +void MyWriterLock::acquire (const char* file, int line) +{ + if (locked) { + trace (file, line) << "MyWriterLock is already locked." << std::endl; + return; + } + + trace (file, line) << "Acquiring MyWriterLock..." << std::endl; + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // The writer count is not zero, so we have to wait for it to be zero again... + while (mutex.writerCount != 0) { + trace (file, line) << "Waiting for current owner of MyWriterLock..." << std::endl + << "\tOwner thread: " << mutex.ownerThread << std::endl + << "\tLast writer file: " << mutex.lastWriterFile << std::endl + << "\tLast writer line: " << mutex.lastWriterLine << std::endl; + + mutex.cond.wait (mutex.mutex); + } + + // ...then we can increment the writer count. + ++mutex.writerCount; + + mutex.ownerThread = Glib::Threads::Thread::self (); + mutex.lastWriterFile = file; + mutex.lastWriterLine = line; + + trace (file, line) << "MyWriterLock is now locked, reader count is " << mutex.readerCount << ", writer count is " << mutex.writerCount << "." << std::endl; + locked = true; +} + +void MyWriterLock::release (const char* file, int line) +{ + if (!locked) { + trace (file, line) << "MyWriterLock is already unlocked." << std::endl; + return; + } + + trace (file, line) << "Releasing MyWriterLock..." << std::endl; + + Glib::Threads::Mutex::Lock lock (mutex.mutex); + + // Decrement the writer number first... + if (--mutex.writerCount == 0) { + // ...and if the writer count is zero again, we can wake up the next writer or reader. + mutex.cond.broadcast (); + + mutex.ownerThread = nullptr; + mutex.lastWriterFile = ""; + mutex.lastWriterLine = 0; + } + + trace (file, line) << "MyWriterLock is now unlocked, reader count is " << mutex.readerCount << ", writer count is " << mutex.writerCount << "." << std::endl; + locked = false; +} + +#endif diff --git a/rtgui/threadutils.h b/rtgui/threadutils.h index e8ef2f29e..d48bfcf01 100644 --- a/rtgui/threadutils.h +++ b/rtgui/threadutils.h @@ -16,30 +16,10 @@ * You should have received a copy of the GNU General Public License * along with RawTherapee. If not, see . */ - - #ifndef _THREADUTILS_ #define _THREADUTILS_ -#include -#include // for raise() -#include - -#ifdef WIN32 -#include -#endif - - -#ifdef NDEBUG -// We don't trace mutex -#undef TRACE_MYRWMUTEX -#define TRACE_MYRWMUTEX 0 -#endif - - -// Uncomment this if you want to bypass the CMakeList options and force the values -// Of course, DO NOT COMMIT! - +// Uncomment this if you want to bypass the CMakeList options and force the values, but do not commit! //#undef PROTECT_VECTORS //#define PROTECT_VECTORS 1 //#undef TRACE_MYRWMUTEX @@ -47,650 +27,285 @@ //#undef STRICT_MUTEX //#define STRICT_MUTEX 1 -/** - * @brief Custom Mutex to replace Glib::Threads::Mutex, which behave differently on windows (recursive) and linux (non-recursive), by a recursive and "debugable" one - * - * This implementation will behave like a Glib::Threads::RecMutex (STRICT_MUTEX=0) or a Glib::Threads::Mutex (STRICT_MUTEX=1), but in this case, the application will - * crash instead of freezing. - * - * In Debug builds, a printf will let you know that the MyMutex was already locked - * - * The default and recommended mode is STRICT_MUTEX=1 - */ +#include -#ifdef WIN32 -class MyMutex : public Glib::RecMutex -{ +#if STRICT_MUTEX && NDEBUG +using MyMutexBase = Glib::Threads::Mutex; #else -class MyMutex : public Glib::Threads::RecMutex +using MyMutexBase = Glib::Threads::RecMutex; +#endif + +/** + * @brief Custom implementation to replace Glib::Threads::Mutex. + * + * Glib::Threads::Mutex shows different behaviour on Windows (recursive) and Linux (non-recursive). + * We therefore use a custom implementation that is optionally recursive and instrumented. + * It will behave like Glib::Threads::RecMutex (STRICT_MUTEX=0) or Glib::Threads::Mutex (STRICT_MUTEX=1). + * Debug builds with strict mutexes, will emit a message and crash immediately upon recursive locking. + */ +class MyMutex : private MyMutexBase { -#endif - -#if STRICT_MUTEX || !defined(NDEBUG) -private: - bool alreadyLocked; -#endif - public: class MyLock; -#if STRICT_MUTEX || !defined(NDEBUG) - MyMutex() : alreadyLocked(false) {} -#else - MyMutex() {} -#endif + MyMutex () = default; + MyMutex (const MyMutex&) = delete; + MyMutex& operator= (const MyMutex&) = delete; - void lock() - { -#ifdef WIN32 - Glib::RecMutex::lock(); -#else - Glib::Threads::RecMutex::lock(); -#endif -#if STRICT_MUTEX || !defined(NDEBUG) + void lock (); + bool trylock (); + void unlock (); - if (alreadyLocked) { -#ifndef NDEBUG - std::cout << "Warning: MyMutex already locked!" << std::endl; // breakpoint +#if STRICT_MUTEX && !NDEBUG +private: + bool locked = false; + void checkLock (); + void checkUnlock (); #endif -#if STRICT_MUTEX -#ifndef NDEBUG -#ifdef WIN32 - DebugBreak(); -#else - raise(SIGTRAP); -#endif -#else - raise(SIGINT); -#endif -#endif - } - - alreadyLocked = true; -#endif - } - - bool trylock() - { -#ifdef WIN32 - - if (Glib::RecMutex::trylock()) -#else - if (Glib::Threads::RecMutex::trylock()) -#endif - { -#if STRICT_MUTEX || !defined(NDEBUG) - - if (alreadyLocked) { -#ifndef NDEBUG - std::cout << "Warning: MyMutex already locked!" << std::endl; // breakpoint -#endif -#if STRICT_MUTEX -#ifndef NDEBUG -#ifdef WIN32 - DebugBreak(); -#else - raise(SIGTRAP); -#endif -#else - raise(SIGINT); -#endif -#endif - } - - alreadyLocked = true; -#endif - return true; - } - - return false; - } - - // Warning: the base class of MyMutex is RecMutex, but the mutex is said "unlocked" on first occurrence of "unlock", to avoid overhead. - void unlock() - { -#if STRICT_MUTEX || !defined(NDEBUG) - alreadyLocked = false; -#endif -#ifdef WIN32 - Glib::RecMutex::unlock(); -#else - Glib::Threads::RecMutex::unlock(); -#endif - } }; - -// Class copied from the Glibmm source code, to provide a workaround of the behavior's difference between Linux and Windows class MyMutex::MyLock { public: - explicit inline MyLock(MyMutex& mutex) : mutex_ (mutex), locked_ (true) - { - mutex_.lock(); - } -#ifdef WIN32 - inline MyLock(MyMutex& mutex, Glib::NotLock) : mutex_ (mutex), locked_ (false) {} - inline MyLock(MyMutex& mutex, Glib::TryLock) : mutex_ (mutex), locked_ (mutex.trylock()) {} -#else - inline MyLock(MyMutex& mutex, Glib::Threads::NotLock) : mutex_ (mutex), locked_ (false) {} - inline MyLock(MyMutex& mutex, Glib::Threads::TryLock) : mutex_ (mutex), locked_ (mutex.trylock()) {} -#endif - inline ~MyLock() - { - if(locked_) { - mutex_.unlock(); - } - } + explicit MyLock (MyMutex& mutex); + MyLock (MyMutex& mutex, Glib::Threads::NotLock); + MyLock (MyMutex& mutex, Glib::Threads::TryLock); - inline void acquire() - { - mutex_.lock(); - locked_ = true; - } - inline bool try_acquire() - { - locked_ = mutex_.trylock(); - return locked_; - } - inline void release() - { - mutex_.unlock(); - locked_ = false; - } - inline bool locked() const - { - return locked_; - } + ~MyLock (); + + MyLock (const MyLock&) = delete; + MyLock& operator= (const MyLock&) = delete; + + void acquire (); + bool try_acquire (); + void release (); private: - MyMutex& mutex_; - bool locked_; - - // noncopyable - MyLock(const MyMutex::Lock&); - MyMutex::Lock& operator=(const MyMutex::Lock&); + MyMutex& mutex; + bool locked; }; - /** - * @brief Custom RWLock with debugging feature, to replace the buggy Glib::RWLock (can have negative reader_count value!) - * - * It may be slower, but thread safe! + * @brief Custom implementation to replace Glib::Threads::RWLock */ class MyRWMutex { public: -#ifdef WIN32 - Glib::Mutex handlerMutex; // Having a recursive or non-recursive mutex is not important here, so we can use Glib::Mutex - Glib::Cond access; -#else - Glib::Threads::Mutex handlerMutex; // Having a recursive or non-recursive mutex is not important here, so we can use Glib::Threads::Mutex - Glib::Threads::Cond access; -#endif - size_t writerCount; - size_t readerCount; -#if TRACE_MYRWMUTEX - Glib::ustring lastWriterFile; - int lastWriterLine; - // Unfortunately, ownerThread may not be the culprit of a deadlock, it can be another concurrent Reader... - void* ownerThread; + MyRWMutex () = default; + MyRWMutex (const MyRWMutex&) = delete; + MyRWMutex& operator= (const MyRWMutex&) = delete; - MyRWMutex() : writerCount(0), readerCount(0), lastWriterLine(0), ownerThread(NULL) {} -#else - MyRWMutex() : writerCount(0), readerCount(0) {} + friend class MyReaderLock; + friend class MyWriterLock; + +private: + Glib::Threads::Mutex mutex; + Glib::Threads::Cond cond; + + std::size_t writerCount = 0; + std::size_t readerCount = 0; + +#if TRACE_MYRWMUTEX + Glib::Threads::Thread* ownerThread = nullptr; + const char* lastWriterFile = ""; + int lastWriterLine = 0; #endif }; /** - * @brief Custom ReaderLock with debugging feature, to replace the buggy Glib::RWLock (can have negative reader_count value!) - * + * @brief Custom implementation to replace Glib::Threads::RWLock::ReaderLock */ class MyReaderLock { +public: + ~MyReaderLock (); - MyRWMutex& rwMutex; + MyReaderLock (const MyReaderLock&) = delete; + MyReaderLock& operator= (const MyReaderLock&) = delete; + +#if !TRACE_MYRWMUTEX + explicit MyReaderLock (MyRWMutex& mutex); + + void acquire (); + void release (); +#else + explicit MyReaderLock (MyRWMutex& mutex, const char* file, int line); + + void acquire (const char* file, int line); + void release (const char* file, int line); +#endif + +private: + MyRWMutex& mutex; bool locked; - -#if TRACE_MYRWMUTEX - static unsigned int readerLockCounter; - int locknumber; - -public: - inline MyReaderLock(MyRWMutex& mutex, const char* name, const char* file, const int line) : rwMutex(mutex), locked(false), locknumber(0) -#else -public: - inline MyReaderLock(MyRWMutex & mutex) : rwMutex(mutex) -#endif - - { - // to operate safely - rwMutex.handlerMutex.lock(); - -#if TRACE_MYRWMUTEX - locknumber = readerLockCounter++; - void* thread = Glib::Thread::self(); - std::cout << thread << "/" << locknumber << ":" << name << " / " << file << " : " << line << " - locking - R"; -#endif - - if (!rwMutex.writerCount) { - // There's no writer operating, we can increment the writer count which will lock writers - ++rwMutex.writerCount; -#if TRACE_MYRWMUTEX - std::cout << " ++ new owner"; -#endif - } else { - // The writer count is non null, but we can be the owner of the writer lock - // It will be the case if the reader count is non null too. - if (!rwMutex.readerCount) { - // the mutex is in real write mode, we're waiting to see it null -#if TRACE_MYRWMUTEX - std::cout << " waiting..." << std::endl << "Current writer owner: " << rwMutex.lastWriterFile << " : " << rwMutex.lastWriterLine << std::endl; -#endif - - while (rwMutex.writerCount) { - rwMutex.access.wait(rwMutex.handlerMutex); - } - - ++rwMutex.writerCount; -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = file; - rwMutex.lastWriterLine = line; - rwMutex.ownerThread = thread; - std::cout << thread << "/" << locknumber << ":" << name << " / " << file << " : " << line << " - locking - R ++ new owner"; -#endif - } - } - - // then we can increment the reader count - ++rwMutex.readerCount; - -#if TRACE_MYRWMUTEX - std::cout << " - ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = true; - } -#if TRACE_MYRWMUTEX - // locks the MyRWMutex with Read access if this MyReaderLock has not already locked it, otherwise return safely - inline void acquire(const char* file, const int line) -#else - // locks the MyRWMutex with Read access if this MyReaderLock has not already locked it, otherwise return safely - inline void acquire() -#endif - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (!locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << ":" << file << " : " << line << " - locking - R (lock)"; -#endif - - if (!rwMutex.writerCount) { - // There's no writer operating, we can increment the writer count which will lock writers - ++rwMutex.writerCount; -#if TRACE_MYRWMUTEX - std::cout << " ++ new owner"; -#endif - } else { - // The writer count is non null, but a reader can be the owner of the writer lock, - // it will be the case if the reader count is non null too. - if (!rwMutex.readerCount) { - // the mutex is in real write mode, we're waiting to see it null -#if TRACE_MYRWMUTEX - std::cout << " waiting..." << std::endl << "Current writer owner: " << rwMutex.lastWriterFile << " : " << rwMutex.lastWriterLine << std::endl; -#endif - - while (rwMutex.writerCount) { - rwMutex.access.wait(rwMutex.handlerMutex); - } - - ++rwMutex.writerCount; -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = file; - rwMutex.lastWriterLine = line; - rwMutex.ownerThread = thread; - std::cout << thread << "/" << locknumber << ":" << file << " : " << line << " - locking - R (lock) ++ new owner"; -#endif - } - } - - // then we can increment the reader count - ++rwMutex.readerCount; - -#if TRACE_MYRWMUTEX - std::cout << " - ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = true; - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already locked by this object - R (lock)" << std::endl; - } - -#endif - } - inline ~MyReaderLock() - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - - // decrement the writer number first - --rwMutex.readerCount; - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << " / unlocking - R - ReaderCount: " << rwMutex.readerCount; -#endif - - if (!rwMutex.readerCount) { - // no more reader, so we decrement the writer count - --rwMutex.writerCount; -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = ""; - rwMutex.lastWriterLine = 0; - rwMutex.ownerThread = NULL; - std::cout << " -- new owner possible!" << " >>> ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount; -#endif - // and signal the next waiting reader/writer that it's free - rwMutex.access.broadcast(); - } - -#if TRACE_MYRWMUTEX - std::cout << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already unlocked by this object - R" << std::endl; - } - -#endif - } -#if TRACE_MYRWMUTEX - // releases the MyRWMutex with Write access if this MyWriterLock has already locked it, otherwise return safely - inline void release(const char* file, const int line) -#else - // releases the MyRWMutex with Write access if this MyWriterLock has already locked it, otherwise return safely - inline void release() -#endif - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - - // decrement the writer number first - --rwMutex.readerCount; - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << " / unlocking - R (release) - ReaderCount: " << rwMutex.readerCount; -#endif - - if (!rwMutex.readerCount) { - // no more reader, so we decrement the writer count - --rwMutex.writerCount; -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = ""; - rwMutex.lastWriterLine = 0; - rwMutex.ownerThread = NULL; - std::cout << " -- new owner possible!" << " >>> ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount; -#endif - // and signal the next waiting reader/writer that it's free - rwMutex.access.broadcast(); - } - -#if TRACE_MYRWMUTEX - std::cout << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = false; - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already unlocked - R (release)" << std::endl; - } - -#endif - } }; /** - * @brief Custom WriterLock with debugging feature, to replace the buggy Glib::RWLock (can have negative reader_count value!) - * + * @brief Custom implementation to replace Glib::Threads::RWLock::WriterLock */ class MyWriterLock { +public: + ~MyWriterLock (); - MyRWMutex& rwMutex; + MyWriterLock (const MyWriterLock&) = delete; + MyWriterLock& operator= (const MyWriterLock&) = delete; + +#if !TRACE_MYRWMUTEX + explicit MyWriterLock (MyRWMutex& mutex); + + void acquire (); + void release (); +#else + MyWriterLock (MyRWMutex& mutex, const char* file, int line); + + void acquire (const char* file, int line); + void release (const char* file, int line); +#endif + +private: + MyRWMutex& mutex; bool locked; - -#if TRACE_MYRWMUTEX - static unsigned int writerLockCounter; - int locknumber; -public: - inline MyWriterLock(MyRWMutex& mutex, const char* name, const char* file, const int line) : rwMutex(mutex), locked(false), locknumber(0) -#else -public: - inline MyWriterLock(MyRWMutex & mutex) : rwMutex(mutex) -#endif - { - // to operate safely - rwMutex.handlerMutex.lock(); - -#if TRACE_MYRWMUTEX - locknumber = writerLockCounter++; - void* thread = Glib::Thread::self(); - std::cout << thread << "/" << locknumber << ":" << name << " / " << file << " : " << line << " - locking - W"; -#endif - - if (rwMutex.writerCount) { - // The writer count is non null, so we have to wait for it to be null again -#if TRACE_MYRWMUTEX - std::cout << " waiting..." << std::endl << "Current writer owner: " << rwMutex.lastWriterFile << " : " << rwMutex.lastWriterLine << std::endl; -#endif - - while (rwMutex.writerCount) { - rwMutex.access.wait(rwMutex.handlerMutex); - } - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << ":" << file << " : " << line << " - locking - W"; -#endif - } - - // then we can increment the writer count - ++rwMutex.writerCount; - -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = file; - rwMutex.lastWriterLine = line; - rwMutex.ownerThread = thread; - std::cout << " ++ new owner <<< ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = true; - } -#if TRACE_MYRWMUTEX - // locks the MyRWMutex with Read access if this MyReaderLock has not already locked it, otherwise return safely - inline void acquire(const char* file, const int line) -#else - // locks the MyRWMutex with Read access if this MyReaderLock has not already locked it, otherwise return safely - inline void acquire() -#endif - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (!locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << ":" << file << " : " << line << " - locking - W (lock)"; -#endif - - if (rwMutex.writerCount) { - // The writer count is non null, so we have to wait for it to be null again -#if TRACE_MYRWMUTEX - std::cout << " waiting..." << std::endl << "Current writer owner: " << rwMutex.lastWriterFile << " : " << rwMutex.lastWriterLine << std::endl; -#endif - - while (rwMutex.writerCount) { - rwMutex.access.wait(rwMutex.handlerMutex); - } - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << ":" << file << " : " << line << " - locking - W (lock)"; -#endif - } - - // then we can increment the reader count - ++rwMutex.writerCount; - -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = file; - rwMutex.lastWriterLine = line; - rwMutex.ownerThread = thread; - std::cout << " ++ new owner <<< ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = true; - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already locked by this object - W (lock)" << std::endl; - } - -#endif - } - inline ~MyWriterLock() - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - - // decrement the writer number first - --rwMutex.writerCount; - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << " / unlocking - W"; -#endif - - if (!rwMutex.writerCount) { -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = ""; - rwMutex.lastWriterLine = 0; - rwMutex.ownerThread = NULL; - std::cout << " -- new owner possible!"; -#endif - // The writer count is null again, so we can wake up the next writer or reader - rwMutex.access.broadcast(); - } - -#if TRACE_MYRWMUTEX - std::cout << " <<< ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already unlocked by this object - W" << std::endl; - } - -#endif - } -#if TRACE_MYRWMUTEX - // releases the MyRWMutex with Write access if this MyWriterLock has already locked it, otherwise return safely - inline void release(const char* file, const int line) -#else - // releases the MyRWMutex with Write access if this MyWriterLock has already locked it, otherwise return safely - inline void release() -#endif - { -#if TRACE_MYRWMUTEX - void* thread = Glib::Thread::self(); -#endif - - if (locked) { - // to operate safely - rwMutex.handlerMutex.lock(); - - // decrement the writer number first - --rwMutex.writerCount; - -#if TRACE_MYRWMUTEX - std::cout << thread << "/" << locknumber << " / unlocking - W (release)"; -#endif - - if (!rwMutex.writerCount) { -#if TRACE_MYRWMUTEX - rwMutex.lastWriterFile = ""; - rwMutex.lastWriterLine = 0; - rwMutex.ownerThread = NULL; - std::cout << " -- new owner possible!"; -#endif - // The writer count is null again, so we can wake up the next writer or reader - rwMutex.access.broadcast(); - } - -#if TRACE_MYRWMUTEX - std::cout << " <<< ReaderCount: " << rwMutex.readerCount << " - WriterCount: " << rwMutex.writerCount << std::endl; -#endif - - rwMutex.handlerMutex.unlock(); - - locked = false; - } - -#if TRACE_MYRWMUTEX - else { - std::cout << thread << "/" << locknumber << " / already unlocked by this object - W (release)" << std::endl; - } - -#endif - } }; +inline void MyMutex::lock () +{ + MyMutexBase::lock (); + +#if STRICT_MUTEX && !NDEBUG + checkLock (); +#endif +} + +inline bool MyMutex::trylock () +{ + if (MyMutexBase::trylock ()) { +#if STRICT_MUTEX && !NDEBUG + checkLock (); +#endif + + return true; + } + + return false; +} + +inline void MyMutex::unlock () +{ +#if STRICT_MUTEX && !NDEBUG + checkUnlock (); +#endif + + MyMutexBase::unlock (); +} + +inline MyMutex::MyLock::MyLock (MyMutex& mutex) + : mutex (mutex) + , locked (true) +{ + mutex.lock(); +} + +inline MyMutex::MyLock::MyLock (MyMutex& mutex, Glib::Threads::NotLock) + : mutex (mutex) + , locked (false) +{ +} + +inline MyMutex::MyLock::MyLock (MyMutex& mutex, Glib::Threads::TryLock) + : mutex (mutex) + , locked (mutex.trylock ()) +{ +} + +inline MyMutex::MyLock::~MyLock () +{ + if (locked) { + mutex.unlock (); + } +} + +inline void MyMutex::MyLock::acquire () +{ + mutex.lock (); + locked = true; +} +inline bool MyMutex::MyLock::try_acquire () +{ + return locked = mutex.trylock (); +} + +inline void MyMutex::MyLock::release () +{ + mutex.unlock (); + locked = false; +} + +#if !TRACE_MYRWMUTEX + +inline MyReaderLock::MyReaderLock (MyRWMutex& mutex) + : mutex (mutex) + , locked (false) +{ + acquire (); +} + +inline MyWriterLock::MyWriterLock (MyRWMutex& mutex) + : mutex (mutex) + , locked (false) +{ + acquire (); +} + +inline MyReaderLock::~MyReaderLock () +{ + if (locked) { + release (); + } +} + +inline MyWriterLock::~MyWriterLock () +{ + if (locked) { + release (); + } +} + +#else + +inline MyReaderLock::MyReaderLock (MyRWMutex& mutex, const char* file, int line) + : mutex (mutex) + , locked (false) +{ + acquire (file, line); +} + +inline MyWriterLock::MyWriterLock (MyRWMutex& mutex, const char* file, int line) + : mutex (mutex) + , locked (false) +{ + acquire (file, line); +} + +inline MyReaderLock::~MyReaderLock () +{ + if (locked) { + release (__FILE__, __LINE__); + } +} + +inline MyWriterLock::~MyWriterLock () +{ + if (locked) { + release (__FILE__, __LINE__); + } +} + +#endif + #if TRACE_MYRWMUTEX -#define MYREADERLOCK(ln, e) MyReaderLock ln(e, #e, __FILE__, __LINE__); -#define MYWRITERLOCK(ln, e) MyWriterLock ln(e, #e, __FILE__, __LINE__); +#define MYREADERLOCK(ln, e) MyReaderLock ln(e, __FILE__, __LINE__); +#define MYWRITERLOCK(ln, e) MyWriterLock ln(e, __FILE__, __LINE__); #define MYREADERLOCK_ACQUIRE(ln) ln.acquire(__FILE__, __LINE__); #define MYWRITERLOCK_ACQUIRE(ln) ln.acquire(__FILE__, __LINE__); #define MYREADERLOCK_RELEASE(ln) ln.release(__FILE__, __LINE__); From 17d9309f1c3c5fa22bf3882c3f3c230293d3464b Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 5 Jan 2016 23:59:14 +0100 Subject: [PATCH 2/2] Remove PROTECT_VECTORS option since either program is correct without locking or it is not, especially since std::vector is definitely not thread-safe on all major platforms. --- CMakeLists.txt | 7 ---- rtgui/batchqueue.cc | 41 ++------------------ rtgui/batchqueue.h | 4 -- rtgui/batchqueueentry.cc | 3 -- rtgui/filebrowser.cc | 70 +--------------------------------- rtgui/filebrowserentry.cc | 3 -- rtgui/threadutils.h | 18 --------- rtgui/thumbbrowserbase.cc | 69 +++------------------------------ rtgui/thumbbrowserentrybase.cc | 11 ------ 9 files changed, 11 insertions(+), 215 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7234c6efe..646dbf46f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -99,7 +99,6 @@ option (WITH_BZIP "Build with Bzip2 support" ON) option (WITH_MYFILE_MMAP "Build using memory mapped file" ON) option (WITH_LTO "Build with link-time optimizations" OFF) option (OPTION_OMP "Build with OpenMP support" ON) -option (PROTECT_VECTORS "Protect critical vectors by custom R/W Mutex, recommanded even if your std::vector is thread safe" ON) option (STRICT_MUTEX "True (recommended): MyMutex will behave like POSIX Mutex; False: MyMutex will behave like POSIX RecMutex; Note: forced to ON for Debug builds" ON) option (TRACE_MYRWMUTEX "Trace RT's custom R/W Mutex (Debug builds only); redirecting std::out to a file is strongly recommended!" OFF) option (AUTO_GDK_FLUSH "Use gdk_flush on all gdk_thread_leave other than the GUI thread; set it ON if you experience X Server warning/errors" OFF) @@ -204,13 +203,7 @@ else (STRICT_MUTEX OR UPPER_CMAKE_BUILD_TYPE STREQUAL "DEBUG") endif (STRICT_MUTEX OR UPPER_CMAKE_BUILD_TYPE STREQUAL "DEBUG") # MyRWMutex -if (PROTECT_VECTORS) - add_definitions (-DPROTECT_VECTORS=1) -else (PROTECT_VECTORS) - add_definitions (-DPROTECT_VECTORS=0) -endif (PROTECT_VECTORS) if (TRACE_MYRWMUTEX) - # Note: it will be set to 0 for Debug builds (rtgui/guiutils.h) add_definitions (-DTRACE_MYRWMUTEX=1) else (TRACE_MYRWMUTEX) add_definitions (-DTRACE_MYRWMUTEX=0) diff --git a/rtgui/batchqueue.cc b/rtgui/batchqueue.cc index e235d1168..c7d5f87e5 100644 --- a/rtgui/batchqueue.cc +++ b/rtgui/batchqueue.cc @@ -90,9 +90,7 @@ BatchQueue::BatchQueue (FileCatalog* aFileCatalog) : processing(NULL), fileCatal BatchQueue::~BatchQueue () { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif // The listener merges parameters with old values, so delete afterwards for (size_t i = 0; i < fd.size(); i++) { @@ -104,9 +102,7 @@ BatchQueue::~BatchQueue () void BatchQueue::resizeLoadedQueue() { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif const auto height = getThumbnailHeight (); @@ -175,9 +171,7 @@ bool BatchQueue::keyPressed (GdkEventKey* event) void BatchQueue::addEntries (const std::vector& entries, bool head, bool save) { { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (const auto entry : entries) { @@ -229,9 +223,8 @@ bool BatchQueue::saveBatchQueue () return false; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif + if (fd.empty ()) return true; @@ -268,9 +261,7 @@ bool BatchQueue::loadBatchQueue () if (file.is_open ()) { // Yes, it's better to get the lock for the whole file reading, // to update the list in one shot without any other concurrent access! -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif std::string row, column; std::vector values; @@ -402,9 +393,7 @@ int cancelItemUI (void* data) void BatchQueue::cancelItems (const std::vector& items) { { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (const auto item : items) { @@ -444,9 +433,8 @@ void BatchQueue::cancelItems (const std::vector& items) void BatchQueue::headItems (const std::vector& items) { { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif + for (auto item = items.rbegin(); item != items.rend(); ++item) { const auto entry = static_cast (*item); @@ -476,9 +464,7 @@ void BatchQueue::headItems (const std::vector& items) void BatchQueue::tailItems (const std::vector& items) { { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (const auto item : items) { @@ -506,10 +492,7 @@ void BatchQueue::tailItems (const std::vector& items) void BatchQueue::selectAll () { { - // TODO: Check for Linux -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif lastClicked = NULL; selected.clear (); @@ -529,10 +512,7 @@ void BatchQueue::selectAll () void BatchQueue::openLastSelectedItemInEditor() { { - // TODO: Check for Linux -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (selected.size() > 0) { openItemInEditor(selected.back()); @@ -554,10 +534,7 @@ void BatchQueue::startProcessing () { if (!processing) { - // TODO: Check for Linux -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty()) { BatchQueueEntry* next; @@ -579,9 +556,7 @@ void BatchQueue::startProcessing () processing->selected = false; } -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // remove button set next->removeButtonSet (); @@ -657,10 +632,7 @@ rtengine::ProcessingJob* BatchQueue::imageReady (rtengine::IImage16* img) bool remove_button_set = false; { - // TODO: Check for Linux -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif delete processing; processing = NULL; @@ -704,15 +676,11 @@ rtengine::ProcessingJob* BatchQueue::imageReady (rtengine::IImage16* img) // Delete all files in directory \batch when finished, just to be sure to remove zombies // Not sure that locking is necessary, but it should be safer - // TODO: Check for Linux -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if( fd.empty() ) { -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif + std::vector names; Glib::ustring batchdir = Glib::build_filename(options.rtdir, "batch"); Glib::RefPtr dir = Gio::File::create_for_path (batchdir); @@ -955,10 +923,7 @@ void BatchQueue::notifyListener (bool queueEmptied) NLParams* params = new NLParams; params->listener = listener; { - // TODO: Check for Linux -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif params->qsize = fd.size(); } params->queueEmptied = queueEmptied; diff --git a/rtgui/batchqueue.h b/rtgui/batchqueue.h index 447f1bb51..b5a3c2167 100644 --- a/rtgui/batchqueue.h +++ b/rtgui/batchqueue.h @@ -83,11 +83,7 @@ public: bool hasJobs () { - // not sure that this lock is necessary, but it's safer to keep it... - // TODO: Check for Linux -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif return (!fd.empty()); } diff --git a/rtgui/batchqueueentry.cc b/rtgui/batchqueueentry.cc index f5ca6a6d5..94117252d 100644 --- a/rtgui/batchqueueentry.cc +++ b/rtgui/batchqueueentry.cc @@ -244,10 +244,7 @@ void BatchQueueEntry::_updateImage (guint8* img, int w, int h) { if (preh == h) { - -#if PROTECT_VECTORS MYWRITERLOCK(l, lockRW); -#endif prew = w; assert (preview == NULL); diff --git a/rtgui/filebrowser.cc b/rtgui/filebrowser.cc index fc0b3b9ae..73fd4b4d6 100644 --- a/rtgui/filebrowser.cc +++ b/rtgui/filebrowser.cc @@ -222,7 +222,7 @@ FileBrowser::FileBrowser () /*********************** * external programs * *********************/ -#if defined(WIN32) && defined(PROTECT_VECTORS) +#if defined(WIN32) Gtk::manage(miOpenDefaultViewer = new Gtk::MenuItem (M("FILEBROWSER_OPENDEFAULTVIEWER"))); #else miOpenDefaultViewer = NULL; @@ -483,9 +483,7 @@ void FileBrowser::rightClicked (ThumbBrowserEntryBase* entry) { { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif trash->set_sensitive (false); untrash->set_sensitive (false); @@ -622,9 +620,7 @@ void FileBrowser::addEntry_ (FileBrowserEntry* entry) // find place in abc order { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif std::vector::iterator i = fd.begin(); @@ -644,9 +640,7 @@ void FileBrowser::addEntry_ (FileBrowserEntry* entry) FileBrowserEntry* FileBrowser::delEntry (const Glib::ustring& fname) { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (std::vector::iterator i = fd.begin(); i != fd.end(); i++) if ((*i)->filename == fname) { @@ -655,9 +649,7 @@ FileBrowserEntry* FileBrowser::delEntry (const Glib::ustring& fname) fd.erase (i); std::vector::iterator j = std::find (selected.begin(), selected.end(), entry); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif if (j != selected.end()) { if (checkFilter (*j)) { @@ -694,21 +686,15 @@ void FileBrowser::close () fbih->pending = 0; { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif selected.clear (); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); // notifySelectionListener will need read access! -#endif notifySelectionListener (); -#if PROTECT_VECTORS MYWRITERLOCK_ACQUIRE(l); -#endif // The listener merges parameters with old values, so delete afterwards for (size_t i = 0; i < fd.size(); i++) { @@ -740,9 +726,7 @@ void FileBrowser::menuItemActivated (Gtk::MenuItem* m) std::vector mselected; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (size_t i = 0; i < selected.size(); i++) { mselected.push_back (static_cast(selected[i])); @@ -812,9 +796,7 @@ void FileBrowser::menuItemActivated (Gtk::MenuItem* m) } else if (m == selall) { lastClicked = NULL; { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif selected.clear (); @@ -1034,9 +1016,7 @@ void FileBrowser::menuItemActivated (Gtk::MenuItem* m) void FileBrowser::copyProfile () { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (selected.size() == 1) { clipboard.setProcParams ((static_cast(selected[0]))->thumbnail->getProcParams()); @@ -1049,9 +1029,7 @@ void FileBrowser::pasteProfile () if (clipboard.hasProcParams()) { std::vector mselected; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (unsigned int i = 0; i < selected.size(); i++) { mselected.push_back (static_cast(selected[i])); @@ -1091,9 +1069,7 @@ void FileBrowser::partPasteProfile () std::vector mselected; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (unsigned int i = 0; i < selected.size(); i++) { mselected.push_back (static_cast(selected[i])); @@ -1142,9 +1118,7 @@ void FileBrowser::openDefaultViewer (int destination) bool success = true; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (selected.size() == 1) { success = (static_cast(selected[0]))->thumbnail->openDefaultViewer(destination); @@ -1378,10 +1352,7 @@ int FileBrowser::getThumbnailHeight () void FileBrowser::applyMenuItemActivated (ProfileStoreLabel *label) { - -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif const rtengine::procparams::PartialProfile* partProfile = profileStore.getProfile (label->entry); @@ -1406,9 +1377,7 @@ void FileBrowser::applyPartialMenuItemActivated (ProfileStoreLabel *label) { { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (!tbl || selected.empty()) { return; @@ -1420,9 +1389,7 @@ void FileBrowser::applyPartialMenuItemActivated (ProfileStoreLabel *label) if (srcProfiles->pparams) { if (partialPasteDlg.run() == Gtk::RESPONSE_OK) { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (bppcl) { bppcl->beginBatchPParamsChange(selected.size()); @@ -1459,9 +1426,7 @@ void FileBrowser::applyFilter (const BrowserFilter& filter) bool selchanged = false; numFiltered = 0; { -#if PROTECT_VECTORS - MYWRITERLOCK(l, entryRW); // Don't make this a writer lock! HOMBRE: Why? 'selected' is modified here -#endif + MYWRITERLOCK(l, entryRW); if (filter.showOriginal) { findOriginalEntries(fd); @@ -1712,9 +1677,7 @@ void FileBrowser::requestRanking(int rank) { std::vector mselected; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (size_t i = 0; i < selected.size(); i++) { mselected.push_back (static_cast(selected[i])); @@ -1728,9 +1691,7 @@ void FileBrowser::requestColorLabel(int colorlabel) { std::vector mselected; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (size_t i = 0; i < selected.size(); i++) { mselected.push_back (static_cast(selected[i])); @@ -1770,9 +1731,7 @@ void FileBrowser::buttonPressed (LWButton* button, int actionCode, void* actionD void FileBrowser::openNextImage () { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty() && selected.size() > 0 && !options.tabbedUI) { @@ -1794,16 +1753,12 @@ void FileBrowser::openNextImage () selected.push_back (fd[k]); //queue_draw (); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // this will require a read access notifySelectionListener (); -#if PROTECT_VECTORS MYWRITERLOCK_ACQUIRE(l); -#endif // scroll to the selected position double h1, v1; @@ -1815,9 +1770,7 @@ void FileBrowser::openNextImage () Thumbnail* thumb = (static_cast(fd[k]))->thumbnail; int minWidth = get_width() - fd[k]->getMinimalWidth(); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // scroll only when selected[0] is outside of the displayed bounds if (h2 + minWidth - h1 > get_width()) { @@ -1843,9 +1796,7 @@ void FileBrowser::openNextImage () void FileBrowser::openPrevImage () { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty() && selected.size() > 0 && !options.tabbedUI) { @@ -1867,16 +1818,12 @@ void FileBrowser::openPrevImage () selected.push_back (fd[k]); //queue_draw (); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // this will require a read access notifySelectionListener (); -#if PROTECT_VECTORS MYWRITERLOCK_ACQUIRE(l); -#endif // scroll to the selected position double h1, v1; @@ -1888,9 +1835,7 @@ void FileBrowser::openPrevImage () Thumbnail* thumb = (static_cast(fd[k]))->thumbnail; int minWidth = get_width() - fd[k]->getMinimalWidth(); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // scroll only when selected[0] is outside of the displayed bounds if (h2 + minWidth - h1 > get_width()) { @@ -1919,10 +1864,7 @@ void FileBrowser::selectImage (Glib::ustring fname) { // need to clear the filter in filecatalog - -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty() && !options.tabbedUI) { for (size_t i = 0; i < fd.size(); i++) { @@ -1941,24 +1883,18 @@ void FileBrowser::selectImage (Glib::ustring fname) selected.push_back (fd[i]); queue_draw (); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif // this will require a read access notifySelectionListener (); -#if PROTECT_VECTORS MYWRITERLOCK_ACQUIRE(l); -#endif // scroll to the selected position double h = selected[0]->getStartX(); double v = selected[0]->getStartY(); -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif setScrollPosition(h, v); @@ -2009,9 +1945,7 @@ void FileBrowser::notifySelectionListener () { if (tbl) { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif std::vector thm; diff --git a/rtgui/filebrowserentry.cc b/rtgui/filebrowserentry.cc index 4e31c2a25..2e2b0ccb1 100644 --- a/rtgui/filebrowserentry.cc +++ b/rtgui/filebrowserentry.cc @@ -236,10 +236,7 @@ void FileBrowserEntry::updateImage (rtengine::IImage8* img, double scale, rtengi void FileBrowserEntry::_updateImage (rtengine::IImage8* img, double s, rtengine::procparams::CropParams cropParams) { - -#if PROTECT_VECTORS MYWRITERLOCK(l, lockRW); -#endif redrawRequests--; scale = s; diff --git a/rtgui/threadutils.h b/rtgui/threadutils.h index d48bfcf01..03203db82 100644 --- a/rtgui/threadutils.h +++ b/rtgui/threadutils.h @@ -20,8 +20,6 @@ #define _THREADUTILS_ // Uncomment this if you want to bypass the CMakeList options and force the values, but do not commit! -//#undef PROTECT_VECTORS -//#define PROTECT_VECTORS 1 //#undef TRACE_MYRWMUTEX //#define TRACE_MYRWMUTEX 1 //#undef STRICT_MUTEX @@ -319,20 +317,4 @@ inline MyWriterLock::~MyWriterLock () #define MYWRITERLOCK_RELEASE(ln) ln.release(); #endif -#ifdef PROTECT_VECTORS -#define IFPV_MYREADERLOCK(l, e) MYREADERLOCK(l, e) -#define IFPV_MYWRITERLOCK(l, e) MYWRITERLOCK(l, e) -#define IFPV_MYREADERLOCK_ACQUIRE(l) MYREADERLOCK_ACQUIRE(l) -#define IFPV_MYWRITERLOCK_ACQUIRE(l) MYWRITERLOCK_ACQUIRE(l) -#define IFPV_MYREADERLOCK_RELEASE(l) MYREADERLOCK_RELEASE(l) -#define IFPV_MYWRITERLOCK_RELEASE(l) MYWRITERLOCK_RELEASE(l) -#else -#define IFPV_MYREADERLOCK(l, e) -#define IFPV_MYWRITERLOCK(l, e) -#define IFPV_MYREADERLOCK_ACQUIRE(l) -#define IFPV_MYWRITERLOCK_ACQUIRE(l) -#define IFPV_MYREADERLOCK_RELEASE(l) -#define IFPV_MYWRITERLOCK_RELEASE(l) -#endif - #endif /* _THREADUTILS_ */ diff --git a/rtgui/thumbbrowserbase.cc b/rtgui/thumbbrowserbase.cc index 3536cc9b7..60f990d6d 100644 --- a/rtgui/thumbbrowserbase.cc +++ b/rtgui/thumbbrowserbase.cc @@ -61,9 +61,7 @@ ThumbBrowserBase::ThumbBrowserBase () void ThumbBrowserBase::scrollChanged () { { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); i++) { fd[i]->setOffset ((int)(hscroll.get_value()), (int)(vscroll.get_value())); @@ -206,9 +204,7 @@ void ThumbBrowserBase::selectPrev (int distance, bool enlarge) getScrollPosition (h, v); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!selected.empty ()) { std::vector::iterator front = std::find (fd.begin (), fd.end (), selected.front ()); @@ -263,9 +259,7 @@ void ThumbBrowserBase::selectPrev (int distance, bool enlarge) } } -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif selectionChanged (); } @@ -278,9 +272,7 @@ void ThumbBrowserBase::selectNext (int distance, bool enlarge) getScrollPosition (h, v); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!selected.empty ()) { std::vector::iterator front = std::find (fd.begin (), fd.end (), selected.front ()); @@ -335,9 +327,7 @@ void ThumbBrowserBase::selectNext (int distance, bool enlarge) } } -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif selectionChanged (); } @@ -350,9 +340,7 @@ void ThumbBrowserBase::selectFirst (bool enlarge) getScrollPosition (h, v); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty ()) { // find first unfiltered entry @@ -403,9 +391,7 @@ void ThumbBrowserBase::selectFirst (bool enlarge) } } -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif selectionChanged (); } @@ -418,9 +404,7 @@ void ThumbBrowserBase::selectLast (bool enlarge) getScrollPosition (h, v); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif if (!fd.empty ()) { // find last unfiltered entry @@ -473,9 +457,7 @@ void ThumbBrowserBase::selectLast (bool enlarge) } } -#if PROTECT_VECTORS MYWRITERLOCK_RELEASE(l); -#endif selectionChanged (); } @@ -546,10 +528,7 @@ void ThumbBrowserBase::configScrollBars () void ThumbBrowserBase::arrangeFiles () { - -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif // GUI already locked by ::redraw, the only caller of this method for now. // We could lock it one more time, there's no harm excepted (negligible) speed penalty @@ -610,9 +589,7 @@ void ThumbBrowserBase::arrangeFiles () currx += maxw; } -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif // This will require a Writer access resizeThumbnailArea (currx, numOfRows * rowHeight); } else { @@ -689,9 +666,7 @@ void ThumbBrowserBase::arrangeFiles () } } -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif // This will require a Writer access resizeThumbnailArea (colsWidth, curry); } @@ -733,9 +708,7 @@ bool ThumbBrowserBase::Internal::on_query_tooltip (int x, int y, bool keyboard_t Glib::ustring ttip = ""; { -#if PROTECT_VECTORS MYREADERLOCK(l, parent->entryRW); -#endif for (size_t i = 0; i < parent->fd.size(); i++) if (parent->fd[i]->drawable && parent->fd[i]->inside (x, y)) { @@ -808,7 +781,7 @@ void ThumbBrowserBase::buttonPressed (int x, int y, int button, GdkEventType typ bool handled = false; { - IFPV_MYREADERLOCK(l, entryRW); + MYREADERLOCK(l, entryRW); for (size_t i = 0; i < fd.size(); i++) if (fd[i]->drawable) { @@ -826,7 +799,7 @@ void ThumbBrowserBase::buttonPressed (int x, int y, int button, GdkEventType typ } { - IFPV_MYWRITERLOCK(l, entryRW); + MYWRITERLOCK(l, entryRW); if (selected.size() == 1 && type == GDK_2BUTTON_PRESS && button == 1) { doubleClicked (selected[0]); @@ -839,18 +812,18 @@ void ThumbBrowserBase::buttonPressed (int x, int y, int button, GdkEventType typ selectSingle (fileDescr); lastClicked = fileDescr; - IFPV_MYWRITERLOCK_RELEASE(l); + MYWRITERLOCK_RELEASE(l); selectionChanged (); } else if (fileDescr && button == 3 && type == GDK_BUTTON_PRESS) { if (!fileDescr->selected) { selectSingle (fileDescr); lastClicked = fileDescr; - IFPV_MYWRITERLOCK_RELEASE(l); + MYWRITERLOCK_RELEASE(l); selectionChanged (); } - IFPV_MYWRITERLOCK_RELEASE(l); + MYWRITERLOCK_RELEASE(l); rightClicked (fileDescr); } } // end of MYWRITERLOCK(l, entryRW); @@ -874,9 +847,7 @@ bool ThumbBrowserBase::Internal::on_expose_event(GdkEventExpose* event) context->set_font_description (get_style()->get_font()); { -#if PROTECT_VECTORS MYWRITERLOCK(l, parent->entryRW); -#endif for (size_t i = 0; i < parent->fd.size() && !dirty; i++) { // if dirty meanwhile, cancel and wait for next redraw if (!parent->fd[i]->drawable || !parent->fd[i]->insideWindow (0, 0, w, h)) { @@ -897,21 +868,15 @@ bool ThumbBrowserBase::Internal::on_button_release_event (GdkEventButton* event) int w = get_width(); int h = get_height(); -#if PROTECT_VECTORS MYREADERLOCK(l, parent->entryRW); -#endif for (size_t i = 0; i < parent->fd.size(); i++) if (parent->fd[i]->drawable && parent->fd[i]->insideWindow (0, 0, w, h)) { ThumbBrowserEntryBase* tbe = parent->fd[i]; -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif // This will require a Writer access... tbe->releaseNotify (event->button, event->type, event->state, (int)event->x, (int)event->y); -#if PROTECT_VECTORS MYREADERLOCK_ACQUIRE(l); -#endif } return true; @@ -923,15 +888,10 @@ bool ThumbBrowserBase::Internal::on_motion_notify_event (GdkEventMotion* event) int w = get_width(); int h = get_height(); -#if PROTECT_VECTORS MYREADERLOCK(l, parent->entryRW); -#endif for (size_t i = 0; i < parent->fd.size(); i++) if (parent->fd[i]->drawable && parent->fd[i]->insideWindow (0, 0, w, h)) { - /*#if PROTECT_VECTORS - MYREADERLOCK_RELEASE(l); // motionNotify calls the queue, which locks - #endif*/ parent->fd[i]->motionNotify ((int)event->x, (int)event->y); } @@ -983,9 +943,7 @@ void ThumbBrowserBase::zoomChanged (bool zoomIn) saveThumbnailHeight(newHeight); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); i++) { fd[i]->resize (previewHeight); @@ -1003,9 +961,7 @@ void ThumbBrowserBase::refreshThumbImages () int previewHeight = getThumbnailHeight(); { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); i++) { fd[i]->resize (previewHeight); @@ -1017,9 +973,7 @@ void ThumbBrowserBase::refreshThumbImages () void ThumbBrowserBase::refreshQuickThumbImages () { -#if PROTECT_VECTORS MYWRITERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); ++i) { fd[i]->refreshQuickThumbnailImage (); @@ -1031,9 +985,7 @@ void ThumbBrowserBase::refreshEditedState (const std::set& efiles editedFiles = efiles; { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); i++) { fd[i]->framed = editedFiles.find (fd[i]->filename) != editedFiles.end(); @@ -1056,9 +1008,8 @@ void ThumbBrowserBase::enableTabMode(bool enable) arrangement = enable ? ThumbBrowserBase::TB_Horizontal : ThumbBrowserBase::TB_Vertical; if ((!options.sameThumbSize && (options.thumbSizeTab != options.thumbSize)) || (options.showFileNames || options.filmStripShowFileNames)) { -#if PROTECT_VECTORS + MYWRITERLOCK(l, entryRW); -#endif for (size_t i = 0; i < fd.size(); i++) { fd[i]->resize (getThumbnailHeight()); @@ -1070,22 +1021,16 @@ void ThumbBrowserBase::enableTabMode(bool enable) // Scroll to selected position if going into ribbon mode or back // Tab mode is horizontal, file browser is vertical { -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif if (!selected.empty()) { if (enable) { double h = selected[0]->getStartX(); -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif hscroll.set_value (min(h, hscroll.get_adjustment()->get_upper())); } else { double v = selected[0]->getStartY(); -#if PROTECT_VECTORS MYREADERLOCK_RELEASE(l); -#endif vscroll.set_value (min(v, vscroll.get_adjustment()->get_upper())); } } @@ -1114,9 +1059,7 @@ int ThumbBrowserBase::getEffectiveHeight() { int h = hscroll.get_height() + 2; // have 2 pixels rounding error for scroll bars to appear -#if PROTECT_VECTORS MYREADERLOCK(l, entryRW); -#endif // Filtered items do not change in size, so take a non-filtered for (size_t i = 0; i < fd.size(); i++) diff --git a/rtgui/thumbbrowserentrybase.cc b/rtgui/thumbbrowserentrybase.cc index d1d7adc51..b924655c5 100644 --- a/rtgui/thumbbrowserentrybase.cc +++ b/rtgui/thumbbrowserentrybase.cc @@ -321,10 +321,7 @@ void ThumbBrowserEntryBase::getTextSizes (int& infow, int& infoh) void ThumbBrowserEntryBase::resize (int h) { - -#if PROTECT_VECTORS MYWRITERLOCK(l, lockRW); -#endif height = h; int old_preh = preh, old_width = width; @@ -446,9 +443,7 @@ void ThumbBrowserEntryBase::draw () return; } -#if PROTECT_VECTORS MYREADERLOCK(l, lockRW); // No resizes, position moves etc. inbetween -#endif int bbWidth, bbHeight; @@ -486,10 +481,7 @@ void ThumbBrowserEntryBase::draw () void ThumbBrowserEntryBase::setPosition (int x, int y, int w, int h) { - -#if PROTECT_VECTORS MYWRITERLOCK(l, lockRW); -#endif exp_width = w; exp_height = h; @@ -503,10 +495,7 @@ void ThumbBrowserEntryBase::setPosition (int x, int y, int w, int h) void ThumbBrowserEntryBase::setOffset (int x, int y) { - -#if PROTECT_VECTORS MYWRITERLOCK(l, lockRW); -#endif ofsX = -x; ofsY = -y;