SystemDependentDataBuffer bits ...

classic Classic list List threaded Threaded
5 messages Options
Michael Meeks-5 Michael Meeks-5
Reply | Threaded
Open this post in threaded view
|

SystemDependentDataBuffer bits ...

Hi Armin,

        I guess you missed the appended on IRC.

        It seems odd that just typing in writer should be so rapidly creating
and destroying these cached items. A couple of thoughts:

        * do we need to do this for simple / common polylines etc. ?
                + surely there is a complexity/benefit tradeoff here.

        * should we not disable the SystemDependentDataBuffer ie.
          remove:

                if(maEntries.empty() && maTimer)
                    maTimer->Stop();

          from there we can stop ourselves in the timeout if necessary.

          this can save us doing a chunk of un-necesary scheduler work
          which can be rather expensive.

        Noel any chance of killing those lines & testing ?

        Armin - any thoughts on whether this is truly necessary for
        simple polylines ? (is it to cache the winding / self
        intersection stuff ? )

        Thoughts ?

                Michael.

<mmeeks> caolan, alg: I wonder if you see a lot of:
<mmeeks> info:vcl.schedule:4742:4748:vcl/source/app/scheduler.cxx:588:
1556195258227 0x7f0124085af0  restarted  a: 1 p: 1 vcl
SystemDependentDataBuffer aSystemDependentDataBuffer
<mmeeks> info:vcl.schedule:4742:4748:vcl/source/app/scheduler.cxx:596:
1556195258227 0x7f0124085af0  stopped    a: 1 p: 1 vcl
SystemDependentDataBuffer aSystemDependentDataBuffer
<mmeeks> caolan: if you run with
SAL_LOG="+INFO.vcl.schedule+WARN.vcl.schedule"
<vmiklos> mmeeks: yes, it fies every second, see
vcl/source/app/svdata.cxx:121
<vmiklos> fires
<mmeeks> caolan: looks to me like we're doing heavy-lifting to cache
even the cairo paths for the most banal polypolygons we render - (just
typing randomly in writer) - which hits all sorts of locking there, as
well as hitting the scheduler core too.
<mmeeks> vmiklos: sure - but I get ~hundred of those rendering a tile ;-)
<mmeeks> vmiklos: we seem to add and remove it a -lot- which seems
rather unreasonable - but perhaps it's just noisy debuug
<vmiklos> ah
<noelgrandin> oddd I thought that that SystemDependantDataBuffer stuff
was only for caching drawing operations for drawinglayer/etc, would not
have expected it to fire in writer
<mmeeks> vmiklos: ~150k of those while typing ~3 lines of random text in
writer =)
<mmeeks> noelgrandin: might be worth a chase ? =) I suspect we're doing
it for banal polypolygons =)
<mmeeks> and we shouldn't - start/stop/copy/allocate is expensive
<noelgrandin> oh great, debugging SystemDependentDataBuffer::startUsage
crashes gdb
<caolan> mmeeks, I don't know anything about that relatively new caching
stuff, except that it seems to be the reason tdf#124863 doesn't work anymore

--
[hidden email] <><, GM Collabora Productivity
Hangout: [hidden email], Skype: mmeeks
(M) +44 7795 666 147 - timezone usually UK / Europe
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Thorsten Behrens-6 Thorsten Behrens-6
Reply | Threaded
Open this post in threaded view
|

Re: SystemDependentDataBuffer bits ...

Hi Michael,

let me jump in for the moment -

Michael Meeks wrote:
> It seems odd that just typing in writer should be so rapidly creating
> and destroying these cached items.
>
Right - IIRC that was very trivial hairlines there, not much value in
buffering that -

> * should we not disable the SystemDependentDataBuffer ie.
>  remove:
>
>                 if(maEntries.empty() && maTimer)
>                     maTimer->Stop();
>
>  from there we can stop ourselves in the timeout if necessary.
>
Not sure I get the question - surely disabling the timer helps to get
scheduler load down? As this code is already inside implTimeoutHdl().

> Armin - any thoughts on whether this is truly necessary for
> simple polylines ? (is it to cache the winding / self
> intersection stuff ? )
>
Yeah, let's cut off trivial cases from being buffered - but that
should be easy to do in the vcl sal layer. There's already some code
to estimate byte size of the buffered item, can perhaps be extended
with other metrics, and then have some limit below that we simply
don't care.

Just a bit concerned we'd flip-flop between optimizing for different
cases here - some perf test rig would be ideal I guess..

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (1K) Download Attachment
Jan-Marek Glogowski Jan-Marek Glogowski
Reply | Threaded
Open this post in threaded view
|

Re: SystemDependentDataBuffer bits ...

In reply to this post by Michael Meeks-5
Hi everyone,

the following is just some guess work, as my build will take much longer to verify.

I had a look at the code and the timer fires every second. That would mean one
call to Start() for every second, which could trigger the message via timeout,
which is slightly less then:

> <mmeeks> vmiklos: ~150k of those while typing ~3 lines of random text in
> writer =)

The only thing left is thousands (missed the "k" in 150 at first glance) of
calls to startUsage and endUsage, always resulting in an empty list, which
starts and stops the timer all over again.

> * should we not disable the SystemDependentDataBuffer ie.
>  remove:
>
>                 if(maEntries.empty() && maTimer)
>                     maTimer->Stop();

That wouldn't help, as the main problem is all the Start() calls in startUsage.
These are "expensive" (and Start() actually should be named Restart()), which
means the Scheduler eventually has to calculate the next timeout. Probably not
the best API.

OTOH Stop() just sets a bool and the Scheduler eventually will do some cleanup
of the stopped task next time it is run. We just "risk" to wake up too early; once.

But I suspect the cache is currently broken in some way. Nothing is really
cached, as it looks like every entry is immediately evicted again.

I've pushed an untested patch to Gerrit as https://gerrit.libreoffice.org/71376.
I'm quite sure, this doesn't change anything, unless some code directly invokes
the task not even waiting for the timeout.

HTH

Jan-Marek
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-5 Michael Meeks-5
Reply | Threaded
Open this post in threaded view
|

Re: SystemDependentDataBuffer bits ...

In reply to this post by Thorsten Behrens-6

On 26/04/2019 13:43, Thorsten Behrens wrote:
> Right - IIRC that was very trivial hairlines there, not much value in
> buffering that -

        =)

>> * should we not disable the SystemDependentDataBuffer ie.
>>  remove:
>>
>>                 if(maEntries.empty() && maTimer)
>>                     maTimer->Stop();
>>
>>  from there we can stop ourselves in the timeout if necessary.
>
> Not sure I get the question - surely disabling the timer helps to get
> scheduler load down? As this code is already inside implTimeoutHdl().

        I rather suspect that adding and removing a timer a very large number
of times has a potentially significant cost. Whereas adding a timer once
- and (when it happens a second later) - noticing there is nothing to do
and removing it has a very small cost once per second. Also makes
logging & debugging scheduler related issues much nicer.

> Just a bit concerned we'd flip-flop between optimizing for different
> cases here - some perf test rig would be ideal I guess..

        If we get the low hanging fruit I guess we win and leave the detail to
the future =)

        ATB,

                Michael.

--
[hidden email] <><, GM Collabora Productivity
Hangout: [hidden email], Skype: mmeeks
(M) +44 7795 666 147 - timezone usually UK / Europe
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Jan-Marek Glogowski Jan-Marek Glogowski
Reply | Threaded
Open this post in threaded view
|

Re: SystemDependentDataBuffer bits ...

In reply to this post by Jan-Marek Glogowski
So replying to myself.

Am 26.04.19 um 16:46 schrieb Jan-Marek Glogowski:

> Hi everyone,
>
> the following is just some guess work, as my build will take much longer to verify.
>
> I had a look at the code and the timer fires every second. That would mean one
> call to Start() for every second, which could trigger the message via timeout,
> which is slightly less then:
>
>> <mmeeks> vmiklos: ~150k of those while typing ~3 lines of random text in
>> writer =)
>
> The only thing left is thousands (missed the "k" in 150 at first glance) of
> calls to startUsage and endUsage, always resulting in an empty list, which
> starts and stops the timer all over again.

After the build finished (I just have an N5000 with 4GB RAM - takes some time),
I augmented the code a bit and got this:

info:vcl.schedule:15575:15575:vcl/source/app/scheduler.cxx:588: 1556320265354
0x55bbba60d8a0  restarted  a: 1 p: 1 vcl SystemDependentDataBuffer
aSystemDependentDataBuffer
debug:15575:15575: startUsage 0x55bbbad0a1a0
info:vcl.schedule:15575:15575:vcl/source/app/scheduler.cxx:596: 1556320265354
0x55bbba60d8a0  stopped    a: 1 p: 1 vcl SystemDependentDataBuffer
aSystemDependentDataBuffer
debug:15575:15575: endUsage   0x55bbbad0a1a0
info:vcl.schedule:15575:15575:vcl/source/app/scheduler.cxx:588: 1556320265354
0x55bbba60d8a0  restarted  a: 1 p: 1 vcl SystemDependentDataBuffer
aSystemDependentDataBuffer
debug:15575:15575: startUsage 0x55bbbad05f10
info:vcl.schedule:15575:15575:vcl/source/app/scheduler.cxx:596: 1556320265354
0x55bbba60d8a0  stopped    a: 1 p: 1 vcl SystemDependentDataBuffer
aSystemDependentDataBuffer
debug:15575:15575: endUsage   0x55bbbad05f10
...

few hundred times, which proves my guess. So the Timer actually never runs.
There is no "invoke" line. Nothing is really cached and evicted by the timer!

As I already wrote Stop() is cheap.

And I also already wrote Start() is "expensive" (quotation marks!); I don't
think it really is - not in this or an other case. Worst case would be always
changing the priority, as the lazy cleanup results in high memory usage. The
"restarted" path consists of:

1. Get Scheduler lock. We're the only user, so no fight (look at the logs!).
2. See the task is already scheduled, so no alloc or "fancy other stuff.
3. Get the current timestamp (this is expected to happen often, so cheap).
4. If the scheduled system timer timeout is > the expected Timer timeout,
restart system timer with shorter interval. Happens once. (this is in
Timer::Start(), as it's virtual).

That's it. But since we're already typing, a lot of other stuff will also be
scheduled. For Writer it's "sw::DocumentTimerManager m_aDocIdle", which triggers
background layouting, autotext, grammar and spell checking.

So what is left: as I already guessed, probably a broken cache. startUsage is
always followed by an endUsage, which hold the same pointer (that's rData.get()
in the output). Maybe that is expected most of the time from the design. Yup,
this pollutes the scheduler log output. I updated the patch to "fix" that,
partly by using Michael Meeks initial suggestion.

See the updated https://gerrit.libreoffice.org/#/c/71376/

Jan-Marek
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice