[Libreoffice-commits] core.git: vcl/inc vcl/osx vcl/README.scheduler vcl/win
Jan-Marek Glogowski
glogow at fbihome.de
Wed Oct 4 12:53:58 UTC 2017
vcl/README.scheduler | 51 +++++++++++++++++++++++++++++------------------
vcl/inc/osx/saltimer.h | 3 --
vcl/inc/saltimer.hxx | 44 ++++++++++++++++++++++++++++++++++++++++
vcl/inc/win/saltimer.h | 7 +++---
vcl/osx/saltimer.cxx | 22 +++++++++-----------
vcl/win/app/saltimer.cxx | 18 ++++++----------
6 files changed, 98 insertions(+), 47 deletions(-)
New commits:
commit da5cdcdeddf7bc21606b4cb64d8b1fc412146935
Author: Jan-Marek Glogowski <glogow at fbihome.de>
Date: Fri Sep 29 21:02:17 2017 +0200
Convert tick-based timer events to versioned ones
Instead of storing the system ticks in the timer event message
simply store a version.
Moves the version handling code into a VersionedEvent class,
inherited by WinSalTimer and AquaSalTimer.
Change-Id: I5add85031d36b3424a26a9ef798294cbfb00b2e4
Reviewed-on: https://gerrit.libreoffice.org/42959
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Jan-Marek Glogowski <glogow at fbihome.de>
diff --git a/vcl/README.scheduler b/vcl/README.scheduler
index ac4a0dd698d4..8c5e64ba74c5 100644
--- a/vcl/README.scheduler
+++ b/vcl/README.scheduler
@@ -107,7 +107,7 @@ thread redirects using Qt::BlockingQueuedConnection.
== General: processing all current events for DoYield ==
This is easily implemented for all non-priority queue based implementations.
-Windows and MacOS both have a timestamp attached to their events / messages,
+Windows and macOS both have a timestamp attached to their events / messages,
so simply get the current time and just process anything < timestamp.
For the KDE backend this is already the default behaviour - single event
processing isn't even supported. The headless backend accomplishes this by
@@ -130,10 +130,27 @@ may block the main thread until some events happen.
Currently we wait on an extra conditional, which is cleared by the main event
loop.
-== MacOS implementation details ==
+== General: invalidation of elapsed timer event messages ==
+
+Since the system timer to run the scheduler is single-shot, there should never
+be more then one elapsed timer event in system event queue. When stopping or
+restarting the timer, we eventually have to remove the now invalid event from
+the queue.
+
+But for the Windows and macOS backends this may fail as they have delayed
+posting of events, so a consecutive remove after a post will actually yield no
+remove. On Windows we even get unwanted processing of events outside of the
+main event loop, which may call the Scheduler, as timer management is handled
+in critical scheduler code.
+
+To prevent these problems, we don't even try to remove these events, but
+invalidate them by versioning the timer events. Timer events with invalid
+versions are processed but simply don't run the scheduler.
+
+== macOS implementation details ==
Generally the Scheduler is handled as expected, except on resize, which is
-handled with different runloop-modes in MacOS. In case of a resize, the normal
+handled with different runloop-modes in macOS. In case of a resize, the normal
runloop is suspended in sendEvent, so we can't call the scheduler via posted
main loop-events. Instead the scheduler uses the timer again.
@@ -145,7 +162,7 @@ but we can prevent running any other SolarMutex based code. Those wakeup
events must be ignored to prevent busy-locks. For more info read the "General:
main thread deferral" section.
-We can neither rely on MacOS dispatch_sync code block execution nor the
+We can neither rely on macOS dispatch_sync code block execution nor the
message handling, as both can't be prioritized or filtered and the first
does also not allow nested execution and is just processed in sequence.
@@ -153,29 +170,25 @@ There is also a workaround for a problem for pushing tasks to an empty queue,
as [NSApp postEvent: ... atStart: NO] doesn't append the event, if the
message queue is empty.
-Probably that's the reason, why some code comments spoke of lost events and
-there was some distinct additional event processing implemented.
-
== Windows implementation details ==
Posted or sent event messages often trigger processing of WndProc in
PeekMessage, GetMessage or DispatchMessage, independently from the message to
fetch, remove or dispatch ("During this call, the system delivers pending,
nonqueued messages..."). Additionally messages have an inherited priority
-based on the function used to generate them. Even if WM_TIMER should have been
-the lowest prio, a posted WM_TIMER is processed with the prio of a posted
-message.
+based on the function used to generate them. Even if WM_TIMER messages should
+have the lowest priority, a manually posted WM_TIMER is processed with the
+priority of a PostMessage message.
-Therefore the current solution always starts a (threaded) timer even for the
-instant Idles and syncs to this timer message in the main dispatch loop.
-Using SwitchToThread(), this seem to work reasonably well.
+Therefore the current solution always starts a (threaded) timer, even for the
+instant Idles, and syncs to this timer message in the main dispatch loop, to
+process the messages in the correct sequence. Using SwitchToThread(), this
+kind of polling seem to work reasonably well.
An additional workaround is implemented for the delayed queuing of posted
messages, where PeekMessage in WinSalTimer::Stop() won't be able remove the
-just posted timer callback message. We handle this by adding a timestamp to
-the timer callback message, which is checked before starting the Scheduler.
-This way we can end with multiple timer callback message in the queue, which
-we were asserting.
+just posted timer callback message. See "General: invalidation of elapsed
+timer event messages" for the details.
To run the required GUI code in the main thread without unlocking the
SolarMutex, we "disable" it. For more infos read the "General: main thread
@@ -233,7 +246,7 @@ mbStatic workaround from the Task class.
== Run the LO application in its own thread ==
-This would probably get rid of most of the MacOS and Windows implementation
+This would probably get rid of most of the macOS and Windows implementation
details / workarounds, but is quite probably a large amount of work.
Instead of LO running in the main process / thread, we run it in a 2nd thread
@@ -243,7 +256,7 @@ and can process system events.
That's just a theory - it definitely needs more analysis before even attending
an implementation.
-== Re-evaluate the MacOS ImplNSAppPostEvent ==
+== Re-evaluate the macOS ImplNSAppPostEvent ==
Probably a solution comparable to the Windows backends delayed PostMessage
workaround using a validation timestamp is better then the current peek,
diff --git a/vcl/inc/osx/saltimer.h b/vcl/inc/osx/saltimer.h
index f70bd65491b8..65247b930cfa 100644
--- a/vcl/inc/osx/saltimer.h
+++ b/vcl/inc/osx/saltimer.h
@@ -41,10 +41,9 @@ public:
~ReleasePoolHolder() { [mpPool release]; }
};
-class AquaSalTimer : public SalTimer
+class AquaSalTimer final : public SalTimer, protected VersionedEvent
{
NSTimer *m_pRunningTimer;
- sal_Int32 m_nTimerStartTicks; ///< system ticks at timer start % SAL_MAX_INT32
void queueDispatchTimerEvent( bool bAtStart );
void callTimerCallback();
diff --git a/vcl/inc/saltimer.hxx b/vcl/inc/saltimer.hxx
index e0179dd5fd27..983e0771ee9b 100644
--- a/vcl/inc/saltimer.hxx
+++ b/vcl/inc/saltimer.hxx
@@ -34,6 +34,7 @@
class VCL_PLUGIN_PUBLIC SalTimer
{
SALTIMERPROC m_pProc;
+
public:
SalTimer() : m_pProc( nullptr ) {}
virtual ~SalTimer() COVERITY_NOEXCEPT_FALSE;
@@ -55,6 +56,49 @@ public:
}
};
+class VersionedEvent
+{
+ /**
+ * The "additional event data" members on macOS are integers, so we can't
+ * use an unsigned integer and rely on the defined unsigned overflow in
+ * InvalidateEvent().
+ */
+ sal_Int32 m_nEventVersion;
+ bool m_bIsValidVersion;
+
+public:
+ VersionedEvent() : m_nEventVersion( 0 ), m_bIsValidVersion( false ) {}
+
+ sal_Int32 GetNextEventVersion()
+ {
+ InvalidateEvent();
+ m_bIsValidVersion = true;
+ return m_nEventVersion;
+ }
+
+ void InvalidateEvent()
+ {
+ if ( m_bIsValidVersion )
+ {
+ if ( m_nEventVersion == SAL_MAX_INT32 )
+ m_nEventVersion = 0;
+ else
+ ++m_nEventVersion;
+ m_bIsValidVersion = false;
+ }
+ }
+
+ bool ExistsValidEvent() const
+ {
+ return m_bIsValidVersion;
+ }
+
+ bool IsValidEventVersion( const sal_Int32 nEventVersion ) const
+ {
+ return m_bIsValidVersion && nEventVersion == m_nEventVersion;
+ }
+};
+
#endif // INCLUDED_VCL_INC_SALTIMER_HXX
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/inc/win/saltimer.h b/vcl/inc/win/saltimer.h
index f87361e55ed8..06e77b171cb3 100644
--- a/vcl/inc/win/saltimer.h
+++ b/vcl/inc/win/saltimer.h
@@ -22,13 +22,14 @@
#include <saltimer.hxx>
-class WinSalTimer : public SalTimer
+class WinSalTimer final : public SalTimer, protected VersionedEvent
{
// for access to Impl* functions
friend LRESULT CALLBACK SalComWndProc( HWND, UINT nMsg, WPARAM wParam, LPARAM lParam, int& rDef );
+ // for access to m_bPollForMessage
+ friend static void CALLBACK SalTimerProc( PVOID data, BOOLEAN );
HANDLE m_nTimerId; ///< Windows timer id
- sal_uInt32 m_nTimerStartTicks; ///< system ticks at timer start % SAL_MAX_UINT32
bool m_bPollForMessage; ///< Run yield until a message is caught (most likely the 0ms timer)
void ImplStart( sal_uIntPtr nMS );
@@ -49,7 +50,7 @@ public:
inline bool WinSalTimer::IsValidWPARAM( WPARAM aWPARAM ) const
{
- return aWPARAM == m_nTimerStartTicks;
+ return IsValidEventVersion( static_cast<sal_Int32>( aWPARAM ) );
}
inline bool WinSalTimer::PollForMessage() const
diff --git a/vcl/osx/saltimer.cxx b/vcl/osx/saltimer.cxx
index 30ef9ef60055..877fdfae85b5 100644
--- a/vcl/osx/saltimer.cxx
+++ b/vcl/osx/saltimer.cxx
@@ -74,10 +74,8 @@ SAL_WNODEPRECATED_DECLARATIONS_POP
void AquaSalTimer::queueDispatchTimerEvent( bool bAtStart )
{
Stop();
- m_nTimerStartTicks = tools::Time::GetMonotonicTicks() % SAL_MAX_INT32;
- if ( 0 == m_nTimerStartTicks )
- m_nTimerStartTicks++;
- ImplNSAppPostEvent( AquaSalInstance::DispatchTimerEvent, bAtStart, m_nTimerStartTicks );
+ ImplNSAppPostEvent( AquaSalInstance::DispatchTimerEvent,
+ bAtStart, GetNextEventVersion() );
}
void AquaSalTimer::Start( sal_uLong nMS )
@@ -134,7 +132,7 @@ void AquaSalTimer::Stop()
[m_pRunningTimer release];
m_pRunningTimer = nil;
}
- m_nTimerStartTicks = 0;
+ InvalidateEvent();
}
void AquaSalTimer::callTimerCallback()
@@ -147,18 +145,19 @@ void AquaSalTimer::callTimerCallback()
void AquaSalTimer::handleTimerElapsed()
{
- // Stop the timer, as it is just invalidated after the firing function
- Stop();
-
if ( GetSalData()->mpInstance->mbIsLiveResize )
+ {
+ // Stop the timer, as it is just invalidated after the firing function
+ Stop();
callTimerCallback();
+ }
else
queueDispatchTimerEvent( YES );
}
void AquaSalTimer::handleDispatchTimerEvent( NSEvent *pEvent )
{
- if (m_nTimerStartTicks == [pEvent data1])
+ if ( IsValidEventVersion( [pEvent data1] ) )
callTimerCallback();
}
@@ -178,8 +177,8 @@ void AquaSalTimer::handleStartTimerEvent( NSEvent* pEvent )
bool AquaSalTimer::IsTimerElapsed() const
{
- assert( !(m_nTimerStartTicks && m_pRunningTimer) );
- if ( 0 != m_nTimerStartTicks )
+ assert( !(ExistsValidEvent() && m_pRunningTimer) );
+ if ( ExistsValidEvent() )
return true;
if ( !m_pRunningTimer )
return false;
@@ -189,7 +188,6 @@ bool AquaSalTimer::IsTimerElapsed() const
AquaSalTimer::AquaSalTimer( )
: m_pRunningTimer( nil )
- , m_nTimerStartTicks( 0 )
{
}
diff --git a/vcl/win/app/saltimer.cxx b/vcl/win/app/saltimer.cxx
index 62fe620b5522..c75b7534e718 100644
--- a/vcl/win/app/saltimer.cxx
+++ b/vcl/win/app/saltimer.cxx
@@ -19,8 +19,6 @@
#include <sal/config.h>
-#include <tools/time.hxx>
-
#include <svsys.h>
#include <win/saldata.hxx>
#include <win/saltimer.h>
@@ -46,8 +44,9 @@ void WinSalTimer::ImplStop()
return;
m_nTimerId = nullptr;
- m_nTimerStartTicks = 0;
DeleteTimerQueueTimer( nullptr, hTimer, INVALID_HANDLE_VALUE );
+ // Keep both after DeleteTimerQueueTimer, because they are set in SalTimerProc
+ InvalidateEvent();
m_bPollForMessage = false;
// remove as many pending SAL_MSG_TIMER_CALLBACK messages as possible
@@ -71,19 +70,17 @@ void WinSalTimer::ImplStart( sal_uLong nMS )
// keep the yield running, if a 0ms Idle is scheduled
m_bPollForMessage = ( 0 == nMS );
- m_nTimerStartTicks = tools::Time::GetMonotonicTicks() % SAL_MAX_UINT32;
// probably WT_EXECUTEONLYONCE is not needed, but it enforces Period
// to be 0 and should not hurt; also see
// https://www.microsoft.com/msj/0499/pooling/pooling.aspx
CreateTimerQueueTimer(&m_nTimerId, nullptr, SalTimerProc,
reinterpret_cast<void*>(
- sal_uIntPtr(m_nTimerStartTicks)),
+ sal_IntPtr(GetNextEventVersion())),
nMS, 0, WT_EXECUTEINTIMERTHREAD | WT_EXECUTEONLYONCE);
}
WinSalTimer::WinSalTimer()
: m_nTimerId( nullptr )
- , m_nTimerStartTicks( 0 )
, m_bPollForMessage( false )
{
}
@@ -119,11 +116,10 @@ void WinSalTimer::Stop()
ImplStop();
}
-/** This gets invoked from a Timer Queue thread.
-
-Don't acquire the SolarMutex to avoid deadlocks, just wake up the main thread
-at better resolution than 10ms.
-*/
+/**
+ * This gets invoked from a Timer Queue thread.
+ * Don't acquire the SolarMutex to avoid deadlocks.
+ */
static void CALLBACK SalTimerProc(PVOID data, BOOLEAN)
{
__try
More information about the Libreoffice-commits
mailing list