ODFstudio/odfstudio
2
0
Fork 0
Code Issues Packages Activity

svp: don't directly yield in main thread

Browse Source 
AKA svp: always release SolarMutex on yield, v2

Implement the TODO when yielding a non-main thread: "use a
SolarMutexReleaser here and drop the m_bNoYieldLock usage"

This whole concept of "deferred yield" is prone to spurious
"deadlocks", if DoYield spawns a nested event loop. This will
not only block one yielding threads, but all indefinitly. And
not releasing the SolarMutex is also not fair for any other
threads waiting. That whole m_nNoYieldLock hack is just needed
to defer GUI processing to the main thread without releasing
the SolarMutex, the exact opposite of what yielding does.

While we can't do anything for the main thread as long as the
whole nested event loop concept prevails, we can prevent the
yielding thread deadlock using conditionals.

With all the special m_nNoYieldLock gone from SvpSalYieldMutex
there isn't any reason to keep it around.

Change-Id: I87c006ad36b4959f7e0dd18dda99a58c4e25032d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/117900
Tested-by: Jenkins
Reviewed-by: Jan-Marek Glogowski <glogow@fbihome.de>
This commit is contained in:
Jan-Marek Glogowski
2022-06-22 12:06:39 +02:00
parent 0feeb94f97
commit d2de55c93f
6 changed files with 133 additions and 298 deletions
Download Patch File Download Diff File Expand all files Collapse all files
vcl/android/androidinst.cxx
+1 -1
View File
@@ -157,7 +157,7 @@ SalFrame *AndroidSalInstance::CreateFrame( SalFrame* pParent, SalFrameStyleFlags
extern "C" SalInstance *create_SalInstance()
{
LOGI("Android: create_SalInstance!");
AndroidSalInstance* pInstance = new AndroidSalInstance( std::make_unique<SvpSalYieldMutex>() );
AndroidSalInstance* pInstance = new AndroidSalInstance( std::make_unique<SalYieldMutex>() );
new SvpSalData();
return pInstance;
}
vcl/headless/headlessinst.cxx
+1 -1
View File
@@ -47,7 +47,7 @@ SalSystem *HeadlessSalInstance::CreateSalSystem()
extern "C" SalInstance *create_SalInstance()
{
HeadlessSalInstance* pInstance = new HeadlessSalInstance(std::make_unique<SvpSalYieldMutex>());
HeadlessSalInstance* pInstance = new HeadlessSalInstance(std::make_unique<SalYieldMutex>());
new SvpSalData();
return pInstance;
}
vcl/headless/svpinst.cxx
+68 -254
View File
@@ -56,115 +56,31 @@
#include <unx/salunxtime.h>
#include <comphelper/lok.hxx>
#include <tools/debug.hxx>
#include <tools/time.hxx>
SvpSalInstance* SvpSalInstance::s_pDefaultInstance = nullptr;
#ifndef NDEBUG
static bool g_CheckedMutex = false;
#define DBG_TESTSVPYIELDMUTEX() \
do { \
if (!g_CheckedMutex) \
{ \
assert(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex()) != nullptr \
&& "This SvpSalInstance function requires use of SvpSalYieldMutex"); \
g_CheckedMutex = true; \
} \
} while(false)
#else // NDEBUG
#define DBG_TESTSVPYIELDMUTEX() ((void)0)
#endif
#if !defined(ANDROID) && !defined(IOS)
static void atfork_child()
{
if (SvpSalInstance::s_pDefaultInstance != nullptr)
{
SvpSalInstance::s_pDefaultInstance->CloseWakeupPipe(false);
SvpSalInstance::s_pDefaultInstance->CreateWakeupPipe(false);
}
}
#endif
SvpSalInstance::SvpSalInstance( std::unique_ptr<SalYieldMutex> pMutex )
: SalGenericInstance( std::move(pMutex) )
, m_WaitCondition(m_NonMainYieldMutex)
, m_EventCondition(m_NonMainYieldMutex)
, m_MainYieldCondition(m_MainYieldMutex)
{
m_aTimeout.tv_sec = 0;
m_aTimeout.tv_usec = 0;
m_nTimeoutMS = 0;
m_MainThread = osl::Thread::getCurrentIdentifier();
CreateWakeupPipe(true);
if( s_pDefaultInstance == nullptr )
s_pDefaultInstance = this;
#if !defined(ANDROID) && !defined(IOS)
pthread_atfork(nullptr, nullptr, atfork_child);
#endif
m_bSupportsOpenGL = false;
}
SvpSalInstance::~SvpSalInstance()
{
if( s_pDefaultInstance == this )
s_pDefaultInstance = nullptr;
CloseWakeupPipe(true);
}
void SvpSalInstance::CloseWakeupPipe(bool log)
{
SvpSalYieldMutex *const pMutex(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex()));
if (!pMutex)
return;
if (pMutex->m_FeedbackFDs[0] != -1)
{
if (log)
{
SAL_INFO("vcl.headless", "CloseWakeupPipe: Closing inherited feedback pipe: [" << pMutex->m_FeedbackFDs[0] << "," << pMutex->m_FeedbackFDs[1] << "]");
}
close (pMutex->m_FeedbackFDs[0]);
close (pMutex->m_FeedbackFDs[1]);
pMutex->m_FeedbackFDs[0] = pMutex->m_FeedbackFDs[1] = -1;
}
}
void SvpSalInstance::CreateWakeupPipe(bool log)
{
SvpSalYieldMutex *const pMutex(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex()));
if (!pMutex)
return;
if (pipe (pMutex->m_FeedbackFDs) == -1)
{
if (log)
{
SAL_WARN("vcl.headless", "Could not create feedback pipe: " << strerror(errno));
std::abort();
}
}
else
{
if (log)
{
SAL_INFO("vcl.headless", "CreateWakeupPipe: Created feedback pipe: [" << pMutex->m_FeedbackFDs[0] << "," << pMutex->m_FeedbackFDs[1] << "]");
}
int flags;
// set close-on-exec descriptor flag.
if ((flags = fcntl (pMutex->m_FeedbackFDs[0], F_GETFD)) != -1)
{
flags |= FD_CLOEXEC;
(void) fcntl(pMutex->m_FeedbackFDs[0], F_SETFD, flags);
}
if ((flags = fcntl (pMutex->m_FeedbackFDs[1], F_GETFD)) != -1)
{
flags |= FD_CLOEXEC;
(void) fcntl(pMutex->m_FeedbackFDs[1], F_SETFD, flags);
}
// retain the default blocking I/O for feedback pipe
}
}
void SvpSalInstance::TriggerUserEventProcessing()
@@ -172,20 +88,21 @@ void SvpSalInstance::TriggerUserEventProcessing()
Wakeup();
}
void SvpSalInstance::Wakeup(SvpRequest const request)
void SvpSalInstance::Wakeup()
{
DBG_TESTSVPYIELDMUTEX();
if (IsMainThread())
return;
ImplSVData* pSVData = ImplGetSVData();
if (pSVData->mpWakeCallback && pSVData->mpPollClosure)
pSVData->mpWakeCallback(pSVData->mpPollClosure);
SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex()));
std::scoped_lock<std::mutex> g(pMutex->m_WakeUpMainMutex);
if (request != SvpRequest::NONE)
pMutex->m_Request = request;
pMutex->m_wakeUpMain = true;
pMutex->m_WakeUpMainCond.notify_one();
if (vcl::lok::isUnipoll())
{
ImplSVData* pSVData = ImplGetSVData();
if (pSVData->mpWakeCallback && pSVData->mpPollClosure)
pSVData->mpWakeCallback(pSVData->mpPollClosure);
}
else
{
m_MainYieldCondition.notify_all();
}
}
bool SvpSalInstance::CheckTimeout( bool bExecuteTimers )
@@ -204,8 +121,6 @@ bool SvpSalInstance::CheckTimeout( bool bExecuteTimers )
m_aTimeout = aTimeOfDay;
m_aTimeout += m_nTimeoutMS;
osl::Guard< comphelper::SolarMutex > aGuard( GetYieldMutex() );
// notify
ImplSVData* pSVData = ImplGetSVData();
if( pSVData->maSchedCtx.mpSalTimer )
@@ -311,8 +226,6 @@ std::shared_ptr<SalBitmap> SvpSalInstance::CreateSalBitmap()
void SvpSalInstance::ProcessEvent( SalUserEvent aEvent )
{
DBG_TESTSVPYIELDMUTEX();
aEvent.m_pFrame->CallCallback( aEvent.m_nEvent, aEvent.m_pData );
if( aEvent.m_nEvent == SalEvent::Resize )
{
@@ -320,109 +233,6 @@ void SvpSalInstance::ProcessEvent( SalUserEvent aEvent )
const SvpSalFrame* pSvpFrame = static_cast<const SvpSalFrame*>( aEvent.m_pFrame);
pSvpFrame->PostPaint();
}
SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex()));
pMutex->m_NonMainWaitingYieldCond.set();
}
SvpSalYieldMutex::SvpSalYieldMutex()
{
#ifndef IOS
m_FeedbackFDs[0] = m_FeedbackFDs[1] = -1;
#endif
}
SvpSalYieldMutex::~SvpSalYieldMutex()
{
}
void SvpSalYieldMutex::doAcquire(sal_uInt32 const nLockCount)
{
auto *const pInst = static_cast<SvpSalInstance*>(GetSalInstance());
if (pInst && pInst->IsMainThread())
{
if (m_bNoYieldLock)
return;
do
{
SvpRequest request = SvpRequest::NONE;
{
std::unique_lock<std::mutex> g(m_WakeUpMainMutex);
if (m_aMutex.tryToAcquire()) {
// if there's a request, the other thread holds m_aMutex
assert(m_Request == SvpRequest::NONE);
m_wakeUpMain = false;
break;
}
m_WakeUpMainCond.wait(g, [this]() { return m_wakeUpMain; });
m_wakeUpMain = false;
std::swap(m_Request, request);
}
if (request != SvpRequest::NONE)
{
// nested Yield on behalf of another thread
assert(!m_bNoYieldLock);
m_bNoYieldLock = true;
bool const bEvents = pInst->DoYield(false, request == SvpRequest::MainThreadDispatchAllEvents);
m_bNoYieldLock = false;
if (write(m_FeedbackFDs[1], &bEvents, sizeof(bool)) != sizeof(bool))
{
SAL_WARN("vcl.headless", "Could not write: " << strerror(errno));
std::abort();
}
}
}
while (true);
}
else
{
m_aMutex.acquire();
}
++m_nCount;
SalYieldMutex::doAcquire(nLockCount - 1);
}
sal_uInt32 SvpSalYieldMutex::doRelease(bool const bUnlockAll)
{
auto *const pInst = static_cast<SvpSalInstance*>(GetSalInstance());
if (pInst && pInst->IsMainThread())
{
if (m_bNoYieldLock)
return 1;
else
return SalYieldMutex::doRelease(bUnlockAll);
}
sal_uInt32 nCount;
{
// read m_nCount before doRelease
bool const isReleased(bUnlockAll || m_nCount == 1);
nCount = comphelper::SolarMutex::doRelease( bUnlockAll );
if (isReleased)
{
if (vcl::lok::isUnipoll())
{
if (pInst)
pInst->Wakeup();
}
else
{
std::scoped_lock<std::mutex> g(m_WakeUpMainMutex);
m_wakeUpMain = true;
m_WakeUpMainCond.notify_one();
}
}
}
return nCount;
}
bool SvpSalYieldMutex::IsCurrentThread() const
{
if (GetSalInstance()->IsMainThread() && m_bNoYieldLock)
return true;
else
return SalYieldMutex::IsCurrentThread();
}
bool SvpSalInstance::IsMainThread() const
@@ -439,25 +249,17 @@ void SvpSalInstance::updateMainThread()
}
}
bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents)
bool SvpSalInstance::ImplYield(bool bWait)
{
DBG_TESTSVPYIELDMUTEX();
DBG_TESTSOLARMUTEX();
assert(IsMainThread());
bool bWasEvent = DispatchUserEvents(bHandleAllCurrentEvents);
if (!bHandleAllCurrentEvents && bWasEvent)
return true;
bWasEvent = CheckTimeout() || bWasEvent;
const bool bMustSleep = bWait && !bWasEvent;
// This is wrong and must be removed!
// We always want to drop the SolarMutex on yield; that is the whole point of yield.
if (!bMustSleep)
return bWasEvent;
bool bWasEvent = DispatchUserEvents(true);
const bool bTimeout = CheckTimeout();
bWasEvent = bTimeout || bWasEvent;
sal_Int64 nTimeoutMicroS = 0;
const bool bMustSleep = bWait && !bWasEvent;
if (bMustSleep)
{
if (m_aTimeout.tv_sec) // Timer is started.
@@ -474,13 +276,21 @@ bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents)
}
SolarMutexReleaser aReleaser;
m_bWasEvent = bWasEvent;
if (vcl::lok::isUnipoll())
{
ImplSVData* pSVData = ImplGetSVData();
if (pSVData->mpPollClosure)
{
int nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS);
int nPollResult = 0;
if (!bMustSleep || (nTimeoutMicroS != 0))
nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, 0);
if (bMustSleep && nPollResult == 0)
{
m_EventCondition.notify_all();
nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS);
}
if (nPollResult < 0)
pSVData->maAppData.mbAppQuit = true;
bWasEvent = bWasEvent || (nPollResult != 0);
@@ -488,59 +298,63 @@ bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents)
}
else if (bMustSleep)
{
SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex()));
std::unique_lock<std::mutex> g(pMutex->m_WakeUpMainMutex);
// wait for doRelease() or Wakeup() to set the condition
if (nTimeoutMicroS == -1)
assert(!bWasEvent);
if (nTimeoutMicroS == 0)
{
pMutex->m_WakeUpMainCond.wait(g,
[pMutex]() { return pMutex->m_wakeUpMain; });
m_EventCondition.notify_all();
return bWasEvent;
}
auto wakeup = [this]() { m_EventCondition.notify_all(); return false; };
if (nTimeoutMicroS == -1)
m_MainYieldCondition.wait(wakeup);
else
{
int nTimeoutMS = nTimeoutMicroS / 1000;
if (nTimeoutMicroS % 1000)
nTimeoutMS += 1;
pMutex->m_WakeUpMainCond.wait_for(g,
std::chrono::milliseconds(nTimeoutMS),
[pMutex]() { return pMutex->m_wakeUpMain; });
m_MainYieldCondition.wait_for(std::chrono::milliseconds(nTimeoutMS), wakeup);
}
// here no need to check m_Request because Acquire will do it
}
if (bWasEvent)
m_WaitCondition.notify_all();
m_EventCondition.notify_all();
return bWasEvent;
}
bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
bool SvpSalInstance::DoYield(bool bWait, bool)
{
DBG_TESTSVPYIELDMUTEX();
DBG_TESTSOLARMUTEX();
bool bWasEvent(false);
SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex()));
if (IsMainThread())
{
bWasEvent = ImplYield(bWait, bHandleAllCurrentEvents);
if (bWasEvent)
pMutex->m_NonMainWaitingYieldCond.set(); // wake up other threads
}
bWasEvent = ImplYield(bWait);
else
{
// TODO: use a SolarMutexReleaser here and drop the m_bNoYieldLock usage
Wakeup(bHandleAllCurrentEvents
? SvpRequest::MainThreadDispatchAllEvents
: SvpRequest::MainThreadDispatchOneEvent);
// blocking read (for synchronisation)
auto const nRet = read(pMutex->m_FeedbackFDs[0], &bWasEvent, sizeof(bool));
assert(nRet == 1); (void) nRet;
if (!bWasEvent && bWait)
SolarMutexReleaser aReleaser;
if (bWait)
{
// block & release YieldMutex until the main thread does something
pMutex->m_NonMainWaitingYieldCond.reset();
SolarMutexReleaser aReleaser;
pMutex->m_NonMainWaitingYieldCond.wait();
m_WaitCondition.wait();
bWasEvent = true;
}
else
{
// prevent ABBA deadlock
std::unique_lock<std::mutex> g(m_MainYieldCondition.mutex());
const bool bIsSleeping = m_MainYieldCondition.isSleeping();
m_EventCondition.wait([bIsSleeping, &g]()
{
g.unlock();
return bIsSleeping;
});
if (bIsSleeping)
bWasEvent = false;
else
bWasEvent = m_bWasEvent;
}
}
vcl/inc/headless/svpinst.hxx
+61 -40
View File
@@ -29,6 +29,9 @@
#include <unx/genprn.h>
#include <condition_variable>
#include <mutex>
#include <functional>
#include <chrono>
#include <sys/time.h>
@@ -39,10 +42,56 @@
#define SvpSalInstance AquaSalInstance
#endif
class SvpYieldCondition final
{
bool m_bReady = false;
bool m_bSleeping = false;
std::mutex& m_rMutex;
std::condition_variable m_Conditional;
public:
SvpYieldCondition(std::mutex& rMutex) : m_rMutex(rMutex) {}
void notify_all()
{
{
std::lock_guard<std::mutex> g(m_rMutex);
m_bReady = true;
}
m_Conditional.notify_all();
}
void wait(std::function<bool()> func = [](){ return false; })
{
std::unique_lock<std::mutex> g(m_rMutex);
if (func())
return;
m_bSleeping = true;
m_Conditional.wait(g, [this]() { return m_bReady; });
m_bSleeping = false;
m_bReady = false;
}
void wait_for(std::chrono::milliseconds rel_time, std::function<bool()> func = [](){ return false; })
{
std::unique_lock<std::mutex> g(m_rMutex);
if (func())
return;
m_bSleeping = true;
m_Conditional.wait_for(g, rel_time, [this]() { return m_bReady; });
m_bSleeping = false;
m_bReady = false;
}
std::mutex& mutex() const { return m_rMutex; }
bool isSleeping() const { return m_bSleeping; }
};
class SvpSalInstance;
class SvpSalTimer final : public SalTimer
{
SvpSalInstance* m_pInstance;
public:
SvpSalTimer( SvpSalInstance* pInstance ) : m_pInstance( pInstance ) {}
virtual ~SvpSalTimer() override;
@@ -55,51 +104,25 @@ public:
class SvpSalFrame;
class GenPspGraphics;
enum class SvpRequest
{
NONE,
MainThreadDispatchOneEvent,
MainThreadDispatchAllEvents,
};
class SvpSalYieldMutex final : public SalYieldMutex
{
private:
// note: these members might as well live in SvpSalInstance, but there is
// at least one subclass of SvpSalInstance (GTK3) that doesn't use them.
friend class SvpSalInstance;
// members for communication from main thread to non-main thread
int m_FeedbackFDs[2];
osl::Condition m_NonMainWaitingYieldCond;
// members for communication from non-main thread to main thread
bool m_bNoYieldLock = false; // accessed only on main thread
std::mutex m_WakeUpMainMutex; // guard m_wakeUpMain & m_Request
std::condition_variable m_WakeUpMainCond;
bool m_wakeUpMain = false;
SvpRequest m_Request = SvpRequest::NONE;
virtual void doAcquire( sal_uInt32 nLockCount ) override;
virtual sal_uInt32 doRelease( bool bUnlockAll ) override;
public:
SvpSalYieldMutex();
virtual ~SvpSalYieldMutex() override;
virtual bool IsCurrentThread() const override;
};
// NOTE: the functions IsMainThread, DoYield and Wakeup *require* the use of
// SvpSalYieldMutex; if a subclass uses something else it must override these
// (Wakeup is only called by SvpSalTimer and SvpSalFrame)
class VCL_DLLPUBLIC SvpSalInstance : public SalGenericInstance, public SalUserEventList
{
timeval m_aTimeout;
sal_uLong m_nTimeoutMS;
oslThreadIdentifier m_MainThread;
// members for communication from main thread to non-main thread
std::mutex m_NonMainYieldMutex;
SvpYieldCondition m_WaitCondition;
SvpYieldCondition m_EventCondition;
bool m_bWasEvent;
// members for communication from non-main thread to main thread
std::mutex m_MainYieldMutex;
SvpYieldCondition m_MainYieldCondition;
virtual void TriggerUserEventProcessing() override;
virtual void ProcessEvent( SalUserEvent aEvent ) override;
bool ImplYield(bool bWait, bool bHandleAllCurrentEvents);
bool ImplYield(bool bWait);
public:
static SvpSalInstance* s_pDefaultInstance;
@@ -107,9 +130,7 @@ public:
SvpSalInstance( std::unique_ptr<SalYieldMutex> pMutex );
virtual ~SvpSalInstance() override;
void CloseWakeupPipe(bool log);
void CreateWakeupPipe(bool log);
void Wakeup(SvpRequest request = SvpRequest::NONE);
void Wakeup();
void StartTimer( sal_uInt64 nMS );
void StopTimer();
vcl/ios/iosinst.cxx
+1 -1
View File
@@ -146,7 +146,7 @@ void SalData::ensureThreadAutoreleasePool() {}
extern "C" SalInstance *create_SalInstance()
{
IosSalInstance* pInstance = new IosSalInstance( std::make_unique<SvpSalYieldMutex>() );
IosSalInstance* pInstance = new IosSalInstance( std::make_unique<SalYieldMutex>() );
new SvpSalData(pInstance);
return pInstance;
}
vcl/source/app/salplug.cxx
+1 -1
View File
@@ -82,7 +82,7 @@ namespace {
#if ENABLE_HEADLESS
SalInstance* svp_create_SalInstance()
{
SvpSalInstance* pInstance = new SvpSalInstance(std::make_unique<SvpSalYieldMutex>());
SvpSalInstance* pInstance = new SvpSalInstance(std::make_unique<SalYieldMutex>());
new SvpSalData();
return pInstance;
}