Re: Merging new VCL scheduler feature branch

Hi jmux,

On 10/24/2016 03:20 PM, Jan-Marek Glogowski wrote:
> I've updated and fixed the feature branch and I'm confident enough to
> merge it :-)

        First - thanks for grasping the nettle here. There is nothing terribly
beautiful about our old scheduler code, and we need someone brave to
come along and re-work it. This is certainly brave =)

        Overall - there are some good things here; but there are also some bits
I'm not so thrilled with, and since I'm confident that even the
simplest, and most obvious change to this code results in regressions,
the "drastically changes its behaviour" type of patch is going to be
very useful to be able to bisect them out - I would really suggest
re-basing it and cherry-picking the work in pieces.

> I'll just quote my mail from last month.

        Sorry for taking so long to get to review it.

> > It completely drops the idea of separated idle and timer handling and
> > drops a lot of special handling and workarounds for problems in the
> > original code, but keeps the Idle class for convenience.

        I like this piece FWIW. I'd like a much more glib-like main-loop for
what we have here.

        However - what the timer vs. idle separation achieved was to give a
'true idle' sense - ie. some tasks were only performed when there were
no incoming OS events - such as key-events, re-sizing, re-painting etc.
That is really important to be able to handle producer vs. consumer
mis-matches, and to keep rendering and typing crisp when we're loaded.
It is not entirely obvious how that works in the new world but its one
of the most important pieces.

> > P.S. it adds a VCL python GDB script, which can be used to dump the list
> > of currently scheduled tasks.

        This is great =)

> > P.P.S. naming is sill a mess. Sometimes it's task, sometimes event. The
> > scheduler class is actually the base class for the timers and idles and
> > should be renamed Event or Task.

        Sure. I append some more detailed notes on various pieces.

        On windows, with the branch - when I re-size a writer window in master
- after a few seconds of frantic event production - I get the window
failing to respond - there are no re-renders at all.

        Interestingly - I also see a large number of SfxItemDisruptor_Impl
items in the scheduler debug - which is curious. Possibly some problem
in the (legacy) manual linked-list logic ?

        I'd love to see some of the old bugs in this area re-tested - I started
to add a few of them to the tracker:

https://bugs.documentfoundation.org/showdependencytree.cgi?id=103542

        Bug 91727 eg. was a good one to re-test from an idle handling
perspective; but I think there are more of those I've not added yet.

        Have you tested suspend & resume - which (with the associated clock
jumping) can be something of a challenge ?

        Anyhow some more notes appended, I think dripping these in piece by
piece would be great.

        HTH,

                Michael.        

* This fragment of another patch turns from a synchronous paint on
  re-size to an async/idle one - is that expected ? and/or why ?
        + would love to have that separated in its own bibisectable
          commit.

diff --git a/vcl/source/window/paint.cxx b/vcl/source/window/paint.cxx
index 2d23020..6eb69af 100644
--- a/vcl/source/window/paint.cxx
+++ b/vcl/source/window/paint.cxx
@@ -670,11 +670,8 @@ IMPL_LINK_NOARG(Window, ImplHandleResizeTimerHdl,
Idle *, void)
     if( mpWindowImpl->mbReallyVisible )
     {
         ImplCallResize();
-        if( mpWindowImpl->mpFrameData->maPaintIdle.IsActive() )
-        {
-            mpWindowImpl->mpFrameData->maPaintIdle.Stop();
-            mpWindowImpl->mpFrameData->maPaintIdle.Invoke( nullptr );
-        }
+        if( !mpWindowImpl->mpFrameData->maPaintIdle.IsActive() )
+            mpWindowImpl->mpFrameData->maPaintIdle.Start();
     }
 }

* void DocumentTimerManager::StartIdling()

        + Not really clear about the switch to a timer here:

@@ -97,7 +100,8 @@ IMPL_LINK( DocumentTimerManager, DoIdleJobs, Idle*,
pIdle, void )
-                pIdle->Start();
+                pTimer->SetTimeout( 2000 );
+                pTimer->Start();

        + Where do these random numbers come from ? why two
          second delay for ~all of the core writer functionality
          layout, spell-checking etc. ?
        + Can we have some 'busy' mode instead - which we can switch
          a document into - when it is undergoing heavy manipulation
          that will disable this work ?
                + I wonder how much of your interactivity problem
                  this would solve by itself as the 1st commit ? =)

* I like this guy:

+    static const SAL_CONSTEXPR sal_uInt64 ImmediateTimeoutMs = 0;
...
-    if ( !nMS )
-        nMS = 1;

        A good place to go =) Then again - we need to skip polling, or
do a non-sleeping poll in several backends in this case - and there was
some frightning OS/X backend comment around this.

* The changes here:
        + void Scheduler::ProcessAllPendingEvents()

        Are quite brave it seems to me; but I like brave =)
                + the 1000 event timeout was something of a sanity check.

* Change Idle to be a Timer subclass
        + Like that.

commit 8dc024db4325d804423355aa3360d5da5fb14a7a
Author: Jan-Marek Glogowski <glogow@fbihome.de>
Date:   Tue Sep 13 12:20:45 2016 +0200

    Don't wait in Yield with pending events

    This re-introduces some functionality of commit
      87199d3829257420429057336283c55be6ae7481

commit 06d731428ef6cf93c7333e8228bfb6088853b52f
Author: Luboš Luňák <l.lunak@collabora.com>
Date:   Sat Jan 31 17:40:48 2015 +0100

    make idle timers actually activate only when idle

        * Worth re-testing bug#91727 ...

        * What is this 'idle' concept - and where do OS
          events fit into it ?

-        pSVData->mpSalTimer->CallCallback( idle );

        * How do we only process 'idle' events when there are
          no more OS events to consume ? ie. we are truly idle ?

        + eg. we want to process as many resize events as possible
                + before we actually do the 'idle' re-paint etc.

commit 5abe75596c3df7fdf100533856361a7ed007f561
Author: Jan-Marek Glogowski <glogow@fbihome.de>
Date:   Thu Sep 8 06:55:30 2016 +0200

    Revert all SalYieldResult changes => bool

-SalYieldResult SvpSalInstance::DoYield(bool bWait, bool
bHandleAllCurrentEvents, sal_uLong const nReleased)
+bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents,
sal_uLong const nReleased)

        * Really dislike that - please just make the enum more
          descriptive to express what is going on.
        * The enum having only two values - but being descriptive
          is good.
        * I can't tell what DoYield returns from the above signature, or
          where to look to find that out.
        * I fear the previous mess of '!' operators, and unclear
          booleans and re-use of this result - which made the
          original code really, really hard to follow (for stupid
          people like me).

* There are going to be big problems here:
        + we are going to need to bisect into this:
        + things like this - need re-basing / squashing out of there:

@@ -279,14 +278,8 @@ void Scheduler::Start()
         return;
     }

-<<<<<<< HEAD
     DBG_TESTSOLARMUTEX();

-    // Mark timer active
-    mbActive = true;
-
-=======
->>>>>>> Use mpSchedulerData for delete and active handling


commit 1478c78dfd60c0c22d3823be2ef1d91d4e615164
Author: Jan-Marek Glogowski <glogow@fbihome.de>
Date:   Wed Jul 20 10:54:30 2016 +0200

    tdf#97087 GDB pretty print the Scheduler task list

    In addition to the GDB pretty printer, this annotates a lot more
    Timers and Idles.

        + love that.

-- 
michael.meeks@collabora.com <><, Pseudo Engineer, itinerant idiot