[Libreoffice-commits] core.git: include/vcl vcl/inc vcl/README.scheduler vcl/source

[Stephan Bergmann (via logerrit)]

 include/vcl/task.hxx         |    2 +-
 vcl/README.scheduler         |   14 ++++----------
 vcl/inc/schedulerimpl.hxx    |   17 -----------------
 vcl/inc/svdata.hxx           |    4 +++-
 vcl/source/app/scheduler.cxx |   22 ++--------------------
 5 files changed, 10 insertions(+), 49 deletions(-)

New commits:
commit 69a9b48d50d98130a65aa6c823dc6cc464fefd71
Author:     Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Tue Dec 8 16:04:48 2020 +0100
Commit:     Stephan Bergmann <sbergman at redhat.com>
CommitDate: Tue Dec 8 20:42:39 2020 +0100

    Replace SchedulerMutex with (non-recursive) std::mutex
    
    ...following up on the TODO from 84af20ef3ea72190784e9e7be820684c2558ba8c "Make
    SchedulerMutex non-recursive"
    
    Change-Id: I3be98f2dba7c7486b79ec1f166431333cc69451a
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/107423
    Tested-by: Jenkins
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/include/vcl/task.hxx b/include/vcl/task.hxx
index 35efe4825bbc..d8adae7eff0b 100644
--- a/include/vcl/task.hxx
+++ b/include/vcl/task.hxx
@@ -91,7 +91,7 @@ public:
 
     /**
      * This function must be called for static tasks, so the Task destructor
-     * ignores the SchedulerMutex, as it may not be available anymore.
+     * ignores the scheduler mutex, as it may not be available anymore.
      * The cleanup is still correct, as it has already happened in
      * DeInitScheduler call well before the static destructor calls.
      */
diff --git a/vcl/README.scheduler b/vcl/README.scheduler
index 466485f16645..e08f59e37cdf 100644
--- a/vcl/README.scheduler
+++ b/vcl/README.scheduler
@@ -60,7 +60,7 @@ Start new Tasks.
 
 The Scheduler just processes it's own Tasks in the main thread and needs
 the SolarMutex for it and for DeInit (tested by DBG_TESTSOLARMUTEX). All
-the other interaction just take the SchedulerMutex or don't need locking
+the other interaction just take the scheduler mutex or don't need locking
 at all.
 
 There is a "workaround" for static Task objecs, which would crash LO on
@@ -69,9 +69,9 @@ Scheduler, while the SchedulerLock would be long gone. OTOH this makes
 Task handling less error-prone, than doing "special" manual cleanup.
 There is also a "= TODOs and ideas =" to get rid if static Tasks.
 
-Actually the SchedulerMutex should never be locked when calling into
-non-scheduler code, so it was converted to simulate a non-recursive
-mutex later. As an alternative it could use a std::mutex.
+Actually the scheduler mutex should never be locked when calling into
+non-scheduler code, so it was converted to a non-recursive
+std::mutex.
 
 
 = Lifecycle / thread-safety of Scheduler-based objects =
@@ -407,9 +407,3 @@ will also affect the Gtk+ and KDE backend for the user event handling.
 
 This way it can be easier used to profile Tasks, eventually to improve LO's
 interactivity.
-
-== Convert SchedulerMutex to std::mutex ==
-
-Since the SchedulerMutex is non-recursive, it could use a std::mutex instead
-of simulating one still using the osl::Mutex. Not sure, if a deadlock instead
-of a crash, when called recursively, is the better "user experience"...
diff --git a/vcl/inc/schedulerimpl.hxx b/vcl/inc/schedulerimpl.hxx
index 26a9c47de11f..f6d5dda708bd 100644
--- a/vcl/inc/schedulerimpl.hxx
+++ b/vcl/inc/schedulerimpl.hxx
@@ -21,7 +21,6 @@
 #define INCLUDED_VCL_INC_SCHEDULERIMPL_HXX
 
 #include "salwtype.hxx"
-#include <osl/mutex.hxx>
 #include <vcl/scheduler.hxx>
 
 class Task;
@@ -38,22 +37,6 @@ struct ImplSchedulerData final
     const char *GetDebugName() const;
 };
 
-class SchedulerMutex final
-{
-    // this simulates a non-recursive mutex
-    bool m_bIsLocked;
-    osl::Mutex m_aMutex;
-
-public:
-    SchedulerMutex()
-        : m_bIsLocked(false)
-    {
-    }
-
-    void acquire();
-    void release();
-};
-
 class SchedulerGuard final
 {
 public:
diff --git a/vcl/inc/svdata.hxx b/vcl/inc/svdata.hxx
index e7a423234043..564c28bdc9b0 100644
--- a/vcl/inc/svdata.hxx
+++ b/vcl/inc/svdata.hxx
@@ -38,6 +38,7 @@
 #include "salwtype.hxx"
 #include "displayconnectiondispatch.hxx"
 
+#include <mutex>
 #include <vector>
 #include <unordered_map>
 #include <boost/functional/hash.hpp>
@@ -369,7 +370,8 @@ struct ImplSchedulerContext
     SalTimer*               mpSalTimer = nullptr;           ///< interface to sal event loop / system timer
     sal_uInt64              mnTimerStart = 0;               ///< start time of the timer
     sal_uInt64              mnTimerPeriod = SAL_MAX_UINT64; ///< current timer period
-    SchedulerMutex          maMutex;                        ///< lock counting mutex for scheduler locking
+    std::mutex              maMutex;                        ///< the "scheduler mutex" (see
+                                                            ///< vcl/README.scheduler)
     bool                    mbActive = true;                ///< is the scheduler active?
 };
 
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index 1759178e2e2f..da8511bffa55 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -207,36 +207,18 @@ next_priority:
     rSchedCtx.mnTimerPeriod        = InfiniteTimeoutMs;
 }
 
-void SchedulerMutex::acquire()
-{
-    if (!m_aMutex.acquire())
-        std::abort();
-    if (m_bIsLocked)
-        std::abort();
-    m_bIsLocked = true;
-}
-
-void SchedulerMutex::release()
-{
-    if (!m_bIsLocked)
-        std::abort();
-    m_bIsLocked = false;
-    if (!m_aMutex.release())
-        std::abort();
-}
-
 void Scheduler::Lock()
 {
     ImplSVData* pSVData = ImplGetSVData();
     assert( pSVData != nullptr );
-    pSVData->maSchedCtx.maMutex.acquire();
+    pSVData->maSchedCtx.maMutex.lock();
 }
 
 void Scheduler::Unlock()
 {
     ImplSVData* pSVData = ImplGetSVData();
     assert( pSVData != nullptr );
-    pSVData->maSchedCtx.maMutex.release();
+    pSVData->maSchedCtx.maMutex.unlock();
 }
 
 /**