[PATCH] window: Don't needlessly sync parent and geometry

[Pekka Paalanen]

On Sun, 14 Sep 2014 17:09:38 +0200
Ondřej Majerech <majerech.o at gmail.com> wrote:

> On Sun, 14 Sep 2014 13:52:21 +0100
> Daniel Stone <daniel at fooishbar.org> wrote:
> 
> > Hi,
> > 
> > On 13 September 2014 15:35, Ondřej Majerech <majerech.o at gmail.com>
> > wrote:
> > 
> > > When a toytoolkit client redraws, the toolkit syncs the parent and
> > > geometry. If a client redraws often (such as the terminal drawing a
> > > huge amount of output), this can spam the compositor with requests
> > > and may result in the client's eventual being killed.
> > >
> > > We don't need to send requests for changing the geometry or parent
> > > if these haven't changed. So remember the last geometry and parent,
> > > and update them only if needed.
> > >
> > 
> > It looked from weston-terminal output like these happened completely
> > unbounded by the frame callback; should these instead be moved
> > somewhere where the requests are only issued once per frame,
> > regardless of how many times they change in between?
> 
> Yes, throttling seems to be the way to go, judging from the dicussion
> on IRC.
> 
> Only problem is that, so far as I can tell, idle_redraw (which is where
> these two requests are indirectly sent from in this scenario) is a
> place that is supposed to only be called at most once per frame. 
> 
> terminal.c seems to call window_schedule_redraw whenever it receives
> and processes a chunk of data from the tty. So maybe we should make
> window_schedule_redraw actually schedule a redraw for when the current
> frame has been finished?

It should already work like that.

window_schedule_redraw() sets surface->redraw_needed = 1; but does *not*
set window->redraw_needed. It also ensures the idle_redraw() function
will be called.

In idle_redraw(), first resizing is explicitly throttled to the frame
callback (pending frame_cb). If we're not resizing, surface_redraw()
will be a no-op if there is a pending frame_cb for the surface. So
essentially the idle_redraw() is an elaborate no-op, unless something
actually forces a repaint with window->redraw_needed.

When idle_redraw() bails out early because of frame_cb, the frame_cb
handler itself will ensure that idle_redraw() is called again, this
time with frame_cb=NULL.

The only thing setting window->redraw_needed to 1 is idle_resize(), and
resizes are throttled explicitly in idle_redraw() to the main surface
frame_cb particularly.

This elaborate scheme is so that sub-surfaces can run their independent
frame_cb driven repaint loops if wanted.

So I think just sending these events in the repaint callback should do
the right thing, and is also the right design.

Could you check if moving those requests to be part of the repaint cycle
in window.c would work?

Btw. not sending the requests when the current state already is correct
is definitely something we also want to do. Therefore, I pushed your
patch: it seems to fix the immediate issue, and we need this logic
anyway.


Thanks,
pq