svp: normalize DoYield

I somehow missed / forgot, that SvpInstance::DoYield was now also yielding on the main thread and doesn't try to do "funky" multi- threaded event processing anymore (because it's no GUI), since commit 0efd06de8f ("vcl: fix hangs in SvpSalInstance"), So this just moves the main thread part into ImplYield and implements DoYield like on all other architectures, as described in README.scheduler. I've tried to fix the LOK poll to be more sensible. Change-Id: I4323685aa250e9d62a2f448cef358a7aa8ae862c Reviewed-on: https://gerrit.libreoffice.org/c/core/+/117899 Tested-by: Jenkins Reviewed-by: Jan-Marek Glogowski <glogow@fbihome.de>
2021-11-30 20:39:33 +01:00
parent 2a323bbbfa
commit 5fbc7ab55a
4 changed files with 96 additions and 83 deletions
@@ -2729,16 +2729,17 @@ static gboolean timeout_wakeup(void *)
 // integrate our mainloop with LOK's
 static int lok_poll_callback(void*, int timeoutUs)
 {
-    if (timeoutUs)
+    bool bWasEvent(false);
+    if (timeoutUs > 0)
    {
        guint timeout = g_timeout_add(timeoutUs / 1000, timeout_wakeup, nullptr);
-        g_main_context_iteration(nullptr, true);
+        bWasEvent = g_main_context_iteration(nullptr, true);
        g_source_remove(timeout);
    }
    else
-        g_main_context_iteration(nullptr, FALSE);
+        bWasEvent = g_main_context_iteration(nullptr, timeoutUs < 0);

-    return 0;
+    return bWasEvent ? 1 : 0;
 }

 // thread-safe wakeup of our mainloop
@@ -130,11 +130,16 @@ There are three types of events, with different priority:
 They should be processed according to the following code:

 ```
-bool DoYield( bool bWait, bool bAllCurrent )
+bool ImplYield(bool bWait, bool bAllCurrent)
 {
+    DBG_TESTSOLARMUTEX();
+    assert(IsMainThread());
+
    bool bWasEvent = ProcessUserEvents( bAllCurrent );
    if ( !bAllCurrent && bWasEvent )
        return true;
+
+    SolarMutexReleaser();
    bWasEvent = ProcessSystemEvents( bAllCurrent, &bWasSchedulerEvent ) || bWasEvent;
    if ( !bWasSchedulerEvent && IsSchedulerEvent() )
    {
@@ -56,6 +56,7 @@
 // FIXME: remove when we re-work the svp mainloop
 #include <unx/salunxtime.h>
 #include <comphelper/lok.hxx>
+#include <tools/debug.hxx>

 SvpSalInstance* SvpSalInstance::s_pDefaultInstance = nullptr;

@@ -404,7 +405,7 @@ sal_uInt32 SvpSalYieldMutex::doRelease(bool const bUnlockAll)
            if (vcl::lok::isUnipoll())
            {
                if (pInst)
-                    pInst->Wakeup(SvpRequest::NONE);
+                    pInst->Wakeup();
            }
            else
            {
@@ -420,13 +421,9 @@ sal_uInt32 SvpSalYieldMutex::doRelease(bool const bUnlockAll)
 bool SvpSalYieldMutex::IsCurrentThread() const
 {
    if (GetSalData()->m_pInstance->IsMainThread() && m_bNoYieldLock)
-    {
        return true;
-    }
    else
-    {
        return SalYieldMutex::IsCurrentThread();
-    }
 }

 bool SvpSalInstance::IsMainThread() const
@@ -443,94 +440,103 @@ void SvpSalInstance::updateMainThread()
    }
 }

+bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents)
+{
+    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;
+
+    sal_Int64 nTimeoutMicroS = 0;
+    if (bMustSleep)
+    {
+        if (m_aTimeout.tv_sec) // Timer is started.
+        {
+            timeval Timeout;
+            // determine remaining timeout.
+            gettimeofday (&Timeout, nullptr);
+            if (m_aTimeout > Timeout)
+                nTimeoutMicroS = ((m_aTimeout.tv_sec - Timeout.tv_sec) * 1000 * 1000 +
+                                  (m_aTimeout.tv_usec - Timeout.tv_usec));
+        }
+        else
+            nTimeoutMicroS = -1; // wait until something happens
+    }
+
+    SolarMutexReleaser aReleaser;
+
+    if (vcl::lok::isUnipoll())
+    {
+        ImplSVData* pSVData = ImplGetSVData();
+        if (pSVData->mpPollClosure)
+        {
+            int nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS);
+            if (nPollResult < 0)
+                pSVData->maAppData.mbAppQuit = true;
+            bWasEvent = bWasEvent || (nPollResult != 0);
+        }
+    }
+    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)
+        {
+            pMutex->m_WakeUpMainCond.wait(g,
+                    [pMutex]() { return pMutex->m_wakeUpMain; });
+        }
+        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; });
+        }
+        // here no need to check m_Request because Acquire will do it
+    }
+
+    return bWasEvent;
+}
+
 bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
 {
    DBG_TESTSVPYIELDMUTEX();
+    DBG_TESTSOLARMUTEX();

-    // first, process current user events
-    bool bEvent = DispatchUserEvents(bHandleAllCurrentEvents);
-    if (!bHandleAllCurrentEvents && bEvent)
-        return true;
-
-    ImplSVData* pSVData = ImplGetSVData();
-
-    bool bTimeout = CheckTimeout();
-    bool bSkipPoll = bEvent;
-    if (pSVData->mpPollCallback == nullptr)
-        bSkipPoll = bEvent || bTimeout;
-    // else - give the poll-callback visibility into waiting timeouts too.
-
+    bool bWasEvent(false);
    SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex()));

    if (IsMainThread())
    {
-        // in kit case
-        if (bWait && !bSkipPoll)
-        {
-            sal_Int64 nTimeoutMicroS = 0;
-            if (m_aTimeout.tv_sec) // Timer is started.
-            {
-                timeval Timeout;
-                // determine remaining timeout.
-                gettimeofday (&Timeout, nullptr);
-                if (m_aTimeout > Timeout)
-                    nTimeoutMicroS = ((m_aTimeout.tv_sec - Timeout.tv_sec) * 1000 * 1000 +
-                                      (m_aTimeout.tv_usec - Timeout.tv_usec));
-            }
-            else
-                nTimeoutMicroS = -1; // wait until something happens
-
-            SolarMutexReleaser aReleaser;
-
-            if (pSVData->mpPollCallback)
-            {
-                // Poll for events from the LOK client.
-                if (nTimeoutMicroS < 0)
-                    nTimeoutMicroS = 5000 * 1000;
-
-                // External poll.
-                if (pSVData->mpPollClosure != nullptr &&
-                    pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS) < 0)
-                    pSVData->maAppData.mbAppQuit = true;
-            }
-            else
-            {
-                std::unique_lock<std::mutex> g(pMutex->m_WakeUpMainMutex);
-                // wait for doRelease() or Wakeup() to set the condition
-                if (nTimeoutMicroS == -1)
-                {
-                    pMutex->m_WakeUpMainCond.wait(g,
-                            [pMutex]() { return pMutex->m_wakeUpMain; });
-                }
-                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; });
-                }
-                // here no need to check m_Request because Acquire will do it
-            }
-        }
-        else
-        {
-            if (bSkipPoll)
-                pMutex->m_NonMainWaitingYieldCond.set(); // wake up other threads
-        }
+        bWasEvent = ImplYield(bWait, bHandleAllCurrentEvents);
+        if (bWasEvent)
+            pMutex->m_NonMainWaitingYieldCond.set(); // wake up other threads
    }
-    else // !IsMainThread()
+    else
    {
+        // TODO: use a SolarMutexReleaser here and drop the m_bNoYieldLock usage
        Wakeup(bHandleAllCurrentEvents
                ? SvpRequest::MainThreadDispatchAllEvents
                : SvpRequest::MainThreadDispatchOneEvent);

-        bool bDidWork(false);
        // blocking read (for synchronisation)
-        auto const nRet = read(pMutex->m_FeedbackFDs[0], &bDidWork, sizeof(bool));
+        auto const nRet = read(pMutex->m_FeedbackFDs[0], &bWasEvent, sizeof(bool));
        assert(nRet == 1); (void) nRet;
-        if (!bDidWork && bWait)
+        if (!bWasEvent && bWait)
        {
            // block & release YieldMutex until the main thread does something
            pMutex->m_NonMainWaitingYieldCond.reset();
@@ -539,7 +545,7 @@ bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
        }
    }

-    return bSkipPoll;
+    return bWasEvent;
 }

 bool SvpSalInstance::AnyInput( VclInputFlags nType )
@@ -101,6 +101,7 @@ class VCL_DLLPUBLIC SvpSalInstance : public SalGenericInstance, public SalUserEv

    virtual void            TriggerUserEventProcessing() override;
    virtual void            ProcessEvent( SalUserEvent aEvent ) override;
+    bool ImplYield(bool bWait, bool bHandleAllCurrentEvents);

 public:
    static SvpSalInstance*  s_pDefaultInstance;