424915 - Scrolling on FF3/gmail/linux much slower than on FF2

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Oleg Romashin (:romaxa)]

Scrolling performance very slow on URL with latest trunk.

Don't know exact regression but with FF2.0 it works fast.

Steps:
1) open URL
2) Collapse all "Labels", "Chat" elements on left side.
3) No chats open, in settings "Show 100 conversations per page"
4) scroll UP/down with mouse wheel.

Result: page scrolling works much slower that mouse wheel scrolling.

CPU 1.8GHz, 1Gb Ram, ATI fglrx, Linux. 1400x1050

PS: Also with old UI "http://mail.google.com/mail/?ui=1" scrolling works much faster on FF3.
On FF2 there are no difference between UI-1 and UI-2.

Comment 1

[Oleg Romashin (:romaxa)]

Need profile data, also regression.

Probably this bug is duplicate or dependent on Bug 422330

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Don't know exact regression

Need that or a reduced testcase to make much progress here.  I recommend finding the range: reducing gmail is a pain.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Gmail combined testcase

Comment 4

[Oleg Romashin (:romaxa)]

I found some interesting behavior...

Save gmail page locally and open it... scrolling still slow.
https://bugzilla.mozilla.org/attachment.cgi?id=311806 !/gmail/gmail.full.html

Right click on gmail page "This Frame" -> "Open Frame in new tab":
https://bugzilla.mozilla.org/attachment.cgi?id=311806 !/gmail/gmail.full_files/a_003.html
Scrolling works fast....

Seems there very big difference between scrolling in frame and out of the frame.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're repainting the entire window. Not sure why, we should be able to avoid it. This reminds me of older bugs on gmail scrolling...

Comment 6

[Oleg Romashin (:romaxa)]

First regression has been found between:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/05/2007-05-27-04-trunk/firefox-3.0a5pre.en-US.linux-i686.tar.bz2
and
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/05/2007-05-28-04-trunk/firefox-3.0a5pre.en-US.linux-i686.tar.bz2

Comment 7

[Oleg Romashin (:romaxa)]

Is it related to bug 343430?

Comment 8

[José Jeria]

Isn't this bug 382392?

See bug 382392, comment 0 about believing that bug 343430 caused this.
 

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Analysis and possible fix posted to bug 382392.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 382392 comment #16. GTK2 lacks a reasonable scrolling API. So we either scroll the whole window and accept jittery movement of the non-scrolled content, i.e. reopen bug 343430, or we repaint the whole window and accept this bug. I lean towards the latter, but if you want to change the behaviour in a custom build, you can mess with the #ifdefs in nsViewManager::CanScrollWithBitBlt.

Ironically I think we can fix this bug for the other platforms ... see bug 382392 for details. And post-1.9 the compositor project will fix this properly for all platforms.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Aha, but it turns out that the gmail-specific problem here should be fixable. There are some hidden IFRAMEs that have been hidden just by positioning them with z-index:-1 behind another IFRAME containing most of the content. nsLayoutUtils::ComputeRepaintRegionForCopy thinks these need to be repainted every time we scroll, but that's not true. The IFRAME with the content contains a scrolled background that's solid white; because the background moves as part of the scroll operation, ComputeRepaintRegionForCopy does not treat it as opaque, because in general optimizing away display items that are covered by opaque items that are moving is incorrect.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I can fix this by allowing an opaque display item that's moving to cover items underneath it *if* the opaque item contains the entire before *and* after area that's moving. That guarantees that anything covered would have been covered both before and after so it's OK to eliminate such items effectively from both before and after display lists.

I need to figure out how to adjust comments and explain this code so that people can actually understand it.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here's the fix. The patch does a few things:

-- The setting of mMovingFrame is moved out of the nsDisplayListBuilder constructor to a new method SetMovingFrame. That new method takes an additional parameter which is the offset by which the moving frame moved.

-- nsDisplayItem::OptimizeVisibility contains the real fix. Instead of just ignoring opaque frames that moved, we allow them to cover other frames with the intersection of their old and new bounds. This ensures that display items that were visible before *or* after the move will end up in the final display list. We need this because ComputeRepaintRegionForCopy needs to use the after-list as a proxy for both the before and after lists.

-- nsDisplayItem::OptimizeVisibility had an optimization that discarded moving, but not position-varying, items from the display list since they weren't relevant to ComputeRepaintRegionForCopy. I've removed that "optimization" since it's actually a bit expensive to call IsVaryingRelativeToFrame and there's no known gain from removing the display item early. The optimization also wasn't clearly marked as such so it was confusing things.

-- nsDisplayBackground::IsVaryingRelativeToFrame is renamed to IsVaryingRelativeToMovingFrame and the aMovingFrame parameter is dropped since it's always nsDisplayListBuilder::GetRootMovingFrame. Since ComputeRepaintRegionForCopy only calls it on frames in the moving subtree, we make that a precondition and stop checking it inside the method. (This is the real motivation for removing the optimization in nsDisplayItem::OptimizeVisibility that made us call it at other times.)

-- gut nsDisplayWrapList::IsVaryingRelativeToMovingFrame since we never call it

-- PrintDisplayListTo needs to print more information to help debugging of ComputeRepaintRegionForCopy

-- AddItemsToRegion has to change to account for the above changes

-- Updated comments in a few places to hopefully improve the understandability of this code

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should get this fixed if we can. This is a very common workload. Bug 382392 will mostly fix this bug for Windows, but something like the patch here is needed to fix the problem on Mac and Linux.

Comment 15

[Oleg Romashin (:romaxa)]

Yep, with this patch it works much faster.

Speed almost the same as with FF2 (maybe 5% slower, but I think this is already another problem).

Comment 16

[Oleg Romashin (:romaxa)]

Also scrolling still much slower comparing with FF2, if 3 or more chats windows opened....

Should I create new bug about it?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With lots of chat windows open, we have to paint a lot more.

You can open a new bug about that if you like, but I won't do anything about it until compositor is done.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

We don't have infrastructure to automatically test this, but you can load this testcase on Mac, start "Quartz Debug", select "Flash screen updates" and then click the button ... with the patch you should NOT see the cyan area covered in yellow while the test runs. On trunk you do, indicating that we're repainting the entire cyan area when we don't have to.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

The debugging changes cause crashes, so fix them.

Comment 20

[Damon Sicore (:damons)]

+'ing this.  We should take the patch to get the perf back.

Comment 21

[Damon Sicore (:damons)]

Per discussion with dbaron and roc, wouldn't hold the release for this if it were the last bug on the list (we're at that point now).  Moving to wanted1.9.0.x+.

Will take a patch if reviews are passed.  Please request approval once reviews are completed.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 312669 [details] [diff] [review]
fix v2

>    /**
>     * @return the frame that is being hypothetically moved
>     */
>    nsIFrame* GetRootMovingFrame() { return mMovingFrame; }

Isn't it a little more than hypothetical now?

> +  const nsPoint& GetMoveDelta() { return mMoveDelta; }

Seems like you should have the same comment here you have on the
definition of mMoveDelta, at least.


> -PRBool nsDisplayWrapList::IsVaryingRelativeToFrame(nsDisplayListBuilder* aBuilder,
> -                                                   nsIFrame* aFrame) {
> -  for (nsDisplayItem* i = mList.GetBottom(); i != nsnull; i = i->GetAbove()) {
> -    if (i->IsVaryingRelativeToFrame(aBuilder, aFrame))
> -      return PR_TRUE;
> -  }
> -  return PR_FALSE;
> +PRBool nsDisplayWrapList::IsVaryingRelativeToMovingFrame(nsDisplayListBuilder* aBuilder) {
> +  // We could try to do something but let's conservatively just return PR_TRUE.
> +  return PR_TRUE;
>  }

How about an assertion that it's actually never called, plus a comment
saying what it would need to do if you called it (or why you'd never
want to)?

+      // The display list should include items for both the before and after
+      // states.

What ensures this?  Is it AddItemsToRegion?  (Well,
nsDisplayItem::OptimizeVisibility ensures we don't consider an item
covered unless it was covered in both states?  But what about items that
were clipped in the after state but not clipped in the before state?  It
does need to be true, so hopefully it is -- but you might want to point
to the code that does so.)

-  virtual PRBool IsVaryingRelativeToFrame(nsDisplayListBuilder* aBuilder,
-       nsIFrame* aFrame) { return PR_FALSE; }
+  virtual PRBool IsVaryingRelativeToMovingFrame(nsDisplayListBuilder* aBuilder) { return PR_FALSE; }

Could you wrap at less than 80 characters?


Regarding your comments in the bug:

> Since
> ComputeRepaintRegionForCopy only calls it on frames in the moving subtree, we
> make that a precondition and stop checking it inside the method. (This is the
> real motivation for removing the optimization in
> nsDisplayItem::OptimizeVisibility that made us call it at other times.)

It looks like the optimization satisfied the precondition too, I think.
But it still makes sense to remove it if it doesn't help.

r+sr=dbaron

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #22)
> (From update of attachment 312669 [details] [diff] [review])
> >    /**
> >     * @return the frame that is being hypothetically moved
> >     */
> >    nsIFrame* GetRootMovingFrame() { return mMovingFrame; }
> 
> Isn't it a little more than hypothetical now?

I guess so...

> > +  const nsPoint& GetMoveDelta() { return mMoveDelta; }
> 
> Seems like you should have the same comment here you have on the
> definition of mMoveDelta, at least.

OK

> > -PRBool nsDisplayWrapList::IsVaryingRelativeToFrame(nsDisplayListBuilder* aBuilder,
> > -                                                   nsIFrame* aFrame) {
> > -  for (nsDisplayItem* i = mList.GetBottom(); i != nsnull; i = i->GetAbove()) {
> > -    if (i->IsVaryingRelativeToFrame(aBuilder, aFrame))
> > -      return PR_TRUE;
> > -  }
> > -  return PR_FALSE;
> > +PRBool nsDisplayWrapList::IsVaryingRelativeToMovingFrame(nsDisplayListBuilder* aBuilder) {
> > +  // We could try to do something but let's conservatively just return PR_TRUE.
> > +  return PR_TRUE;
> >  }
> 
> How about an assertion that it's actually never called, plus a comment
> saying what it would need to do if you called it (or why you'd never
> want to)?

How about a warning? An assertion suggests calling it would be wrong, which isn't true. I'll add a comment explaining why this is not called at the moment.

> +      // The display list should include items for both the before and after
> +      // states.
> 
> What ensures this?  Is it AddItemsToRegion?  (Well,
> nsDisplayItem::OptimizeVisibility ensures we don't consider an item
> covered unless it was covered in both states?  But what about items that
> were clipped in the after state but not clipped in the before state?  It
> does need to be true, so hopefully it is -- but you might want to point
> to the code that does so.)

It's not AddItemsToRegion.

For a frame to be clipped in the after state but not the before state, either the clipper or clippee has to move while the other does not move. Having the clipper move while the clipee does not is very rare, AFAIK it only happens when an abs-pos frame is clipping a fixed-pos frame, and that situation is handled by ApplyAbsPosClipping in nsFrame.cpp, which prevents the cipping in that case.

Now, the case where clippee moves but the clipper does not is probably a lot more common, and we actually do have a bug there! I'll attach a testcase. I'm not aware of it being reported elsewhere.

> -  virtual PRBool IsVaryingRelativeToFrame(nsDisplayListBuilder* aBuilder,
> -       nsIFrame* aFrame) { return PR_FALSE; }
> +  virtual PRBool IsVaryingRelativeToMovingFrame(nsDisplayListBuilder*
> aBuilder) { return PR_FALSE; }
> 
> Could you wrap at less than 80 characters?

Sure.

> > Since
> > ComputeRepaintRegionForCopy only calls it on frames in the moving subtree,
> > we make that a precondition and stop checking it inside the method. (This
> > is the real motivation for removing the optimization in
> > nsDisplayItem::OptimizeVisibility that made us call it at other times.)
> 
> It looks like the optimization satisfied the precondition too, I think.
> But it still makes sense to remove it if it doesn't help.

You're right.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 312669 [details] [diff] [review]
fix v2

Significant speedup to scrolling in Gmail on Mac and Linux. The code is a bit tricky but it is well understood. Downside, this is invalidation-related so no automated test coverage.

Comment 25

[Laurent Jouanneau]

On my computer, I confirm that the scrolling of some page  are *very* slow with Firefox 3.0b5  (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5)

here an example of such page : http://linuxfr.org/2008/04/09/23950.html

However, I don't know if the behavior on this page is the same problem as on gmail, and if your patch fix it (when I apply the patch on the current trunk, some hunks failed). I will create a new bug if you tell me it is an other problem.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 312669 [details] [diff] [review]
fix v2

a=beltzner, but if this is found to regress, I think we can back it out and hold off until we have more time to get a better test in place.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Laurent Jouanneau, what you're seeing is almost certainly a different problem.  I would suspect border drawing (and the suck that is RENDER) to be the problem in your case.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I filed bug 428156 about a case where we incorrectly handle a moving element clipped by a non-moving element.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 30

[Laurent Jouanneau]

Boris: Ok, I confirm this is an other issue. I tested with the latest trunk after the checkin of the patch, and the problem is still there. Is my issue related to your bug 405740 ?

Comment 31

[Boris Zbarsky [:bzbarsky]]

I have no idea.  The only way to know is to start reducing the page until you find the exact style rule (or small set of style rules) that lead to the slowness...

The other thing to do is to narrow down the time range in which the slowness appeared.  That will at least tell you which rules to worry about.

My guess is that the issue is the moz-border-radius, and I was sure we had a bug on that... but we might not.  If that's in fact the issue, and the slowness started with bug 368247, please make sure the bug you file blocks that one?

Comment 32

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task