43305 - scrolling code on linux paints twice in some circumstances and leaks elements into linked list

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

Right now the scrolling code on linux paints twice when scrolling down in some
circumstances and further investigations reveals that it's leaking elements into
a linked list that's traversed on every scroll.  This means that in addition to
double painting the longer you scroll the slower it gets.  Bonus.

I've been working on this for the last couple of days.  The pain in this bug
runs deep but I have four sheets of paper outlining a plan to fix it The Right
Way.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 2

[Tomi Leppikangas]

Great work, scrolling looks much faster now. But sometimes widgets
on page get position all wrong (bugzilla query page is good test for this).
I Think this is becouse of scroll optimisation:

  if (ABS(dx) > width || ABS(dy) > height) {
    gdk_superwin_expose_area(superwin, 0, 0, width, height);
    return;
  }

This doesn't move child windows at all, just repaints main window at right
place.

Here is hacked version that moves child windows too:

  if (ABS(dx) > width || ABS(dy) > height) {
    /* get list of widgets */
    GList *iter, *childs = gdk_window_get_children(superwin->bin_window);
    iter=childs;
    while(iter)
      {
        gint x,y;
        gdk_window_get_position(iter->data, &x, &y);
        gdk_window_move(iter->data, x+dx, y+dy);
        iter=iter->next;
      }
    g_list_free(childs);

    gdk_superwin_expose_area(superwin, 0, 0, width, height);
    return;
  }

Comment 3

[Christopher Blizzard (:blizzard)]

Yeah, that will break things now that you mention it.  I'll have a look at this
when I get back into town.  I don't like the idea of scrolling those widgets
around one at a time by hand.  It will give an interesting visual effect on slow
machines.

Comment 4

[Christopher Blizzard (:blizzard)]

Attachment: patch #2

Comment 5

[Christopher Blizzard (:blizzard)]

Ok, the patch that I've uploaded doesn't have the shortcut but it does clip
paints to the width or height of the window.  Should have the best of all worlds
here.  Tax free child window movement and clipped painting.

Comment 6

[Christopher Blizzard (:blizzard)]

So, I'm pretty happy with this code.  I'd like to get a review on it if anyone
is up to it.

Comment 7

[Marko Macek]

When moving the widgets one-by-one the "visual effects" could be removed by
mapping an unpaintable window on top and unmapping it when done. Probably not
worth it?

What happens to this mechanism with large (>32kpx) pages? Widgets probably have
problems...

Comment 8

[Christopher Blizzard (:blizzard)]

I imagine there are problems with > 32 bit scrolls.  I'm not trying to solve
that here.  That's another problem entirely.

Comment 9

[Christopher Blizzard (:blizzard)]

bryner has reviewed this.  I'm still looking for other people to review and test
it.

Comment 10

[Marko Macek]

I tested it and seems to work fine (and indeed is faster).

Still looking at the code... 

Comment 11

[Christopher Blizzard (:blizzard)]

Let me know.  Looking for multiple reviews on this.

Comment 12

[Tomi Leppikangas]

Ok, run this with couple of printfs and paintflash on, found two
things:

1. linked list can grow quite large when scrolling just with up/down
arrows. This is becouse in every expose there is stuff in list and goto jumps
over list cleaning. This is easy to fix by moving label 'end:' just before
list cleaning block.

2. Other thing that i notised that when scrolling down more than one page,
antiexpose events doesnt match with expose. I didnt figure out why, i just
tryed other values and come to this:

  /* if scrolling down ( dy < 0 ) moving window up so expose area at
     the bottom of the window.  Clip the exposed area to the height of
     the window or the offset, whichever is smaller.  */
  if (dy < 0) {
-   gdk_superwin_expose_area(superwin, 0, height - ABS(dy),
+   gdk_superwin_expose_area(superwin, 0, MAX(0, height - ABS(dy)),
                             width, MIN(height, ABS(dy)));
    /* If we've exposed this area add an antiexpose for it.  When the
       window was moved up, the expose will be offset by ABS(dy). */
        gdk_superwin_add_antiexpose(superwin, move_serial,
                                0,
-                               height, /* actually, height-ABS(dy)+ABS(dy). */
+                               MAX(height, ABS(dy)),
                                width, MIN(height, ABS(dy)));
  }

Comment 13

[Christopher Blizzard (:blizzard)]

For 1, we can definitely move end end: label before the cleanup.  I've added
that.

As for 2, good catch.  The area we want to expose doesn't include any areas that
are outside of the visible window where dy is < 0:

MAX(0, height - ABS(dy))

otherwise the y for the start of the expose is < 0 and we paint a lot more than
what we want to.

The antiexpose that we want to generate will be offset by either the height, or
however much we are moving the window

MAX(height, ABS(dy))

since that's what the X expose event will look like.

I've added this for the x < 0 case, too.  Thanks, again!  I'll attach a new
patch here for testing.

Comment 14

[Christopher Blizzard (:blizzard)]

Attachment: patch 3

Comment 15

[Christopher Blizzard (:blizzard)]

So, how does everyone feel about this?  I'd like to get this into the tree to
plug the leak.  I don't know of any outstanding issues with the code.

Comment 16

[Christopher Blizzard (:blizzard)]

I have an r=imot on this.

Comment 17

[Stuart Parmenter]

r=pavlov

Comment 18

[Christopher Blizzard (:blizzard)]

a=brendan on this

Comment 19

[Tomi Leppikangas]

There might be small speedup by emptying mouse motion events from
x queue when scrolling. I tryed this and scrolling looks better,
with long tables scrolling it's not coming like half second behind
mouse.

I added this code to end of gdk_superwin_scroll:

  XSync(GDK_DISPLAY(), FALSE);
  while (XCheckTypedEvent(GDK_DISPLAY(), MotionNotify, &remove_event)
         == TRUE);

(and added   XEvent remove_event; at start)

Comment 20

[Marko Macek]

You should use the last motion event, not the first one.

One thing that you should try: XSync only if there are two MotionNotify events 
in the queue.

Comment 21

[Christopher Blizzard (:blizzard)]

And only on the window in question.  You don't want the potential to start
eating events for windows you don't own.

Also, I'm concerned about doing this from the scroll code and not doing anything
with them.  There's a potential of lossage of information.  We do compress
motion notify events somewhere else.  You should look there for an example.

Comment 22

[Marko Macek]

I agree, it's best to compress elsewhere. I misread the first comment. 

At the very least, the last events needs to be put back in the queue.

Comment 23

[Christopher Blizzard (:blizzard)]

To see how we do it elsewhere have a look at:

http://lxr.mozilla.org/seamonkey/ident?i=HandleGDKEvent

Comment 24

[Christopher Blizzard (:blizzard)]

Checked in.

Comment 25

[Tomi Leppikangas]

Sorry i missed that this compression is already in gdkhanleevent.

But it looks like its not compressing all events, somehow when scrolling hard
it doesnt get in there. But if you add that XSync(GDK_DISPLAY(), FALSE);
at end of gdk_superwin_scroll() it starts to work. This makes scrolling
even more better looking. You can see that more events is compressed by putting
printf at nsWindow::HandleGDKEvent:

    while (XCheckWindowEvent(GDK_DISPLAY(),
                             GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
                             ButtonMotionMask, &xevent)) {
      synthEvent = PR_TRUE;
+     printf("take out one event\n");
    }


Good testpage for this is some long slashdot article, like:
http://slashdot.org/article.pl?sid=00/07/05/167248&mode=thread&threshold=1

Without XSync, no eventcompressing is happening when scrolling page and it
scrolls slighly after mouse, with XSync some events are compressed and
scrolling looks faster.

Comment 26

[Christopher Blizzard (:blizzard)]

It does look a bit better with that in there, yes.  I'll check that in.