510856 - Scrolling performance regression after bug 507334

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Arpad Borsos [:Swatinem]]

Before landing of bug 507334, the linked testcase worked smoothly in the order of ~25s.
After landing, the scrolling was very slow in the order of ~90s.
It does not matter if the scrolled frame is partially offscreen or not.

System is a 64bit Ubuntu 9.04 with 180.44 nvidia drivers.

Comment 1

[Ria Klaassen (not reading all bugmail)]

On Windows I can see that it scrolls less pleasant than with e.g. Shiretoko but that seems not related with this bug.

Comment 2

[Alice0775 White]

This problem happens on not only Lunix but also Windows XP.
Google reader ( http://www.google.com/reader/view/ )
Scrolling by mouse wheel is slow.
Scroll continues for a while after having stopped a wheel. This is very bad behavior.

[Works]
http://hg.mozilla.org/mozilla-central/rev/e0cc032cb852
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090813 Minefield/3.7a1pre ID:20090813195901

[Broken]
http://hg.mozilla.org/mozilla-central/rev/e6034ded61fd
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090813 Minefield/3.7a1pre ID:20090813205300

Comment 3

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

Attachment: round out moving content bounds to device pixels

I observed a problem while writing automated tests for scroll analysis: the bounds of the moving content was not being rounded to pixels, so we'd get into a situation where the blit region(s) were not pixel aligned. That would force us to repaint single-pixel strips around the edges of blit regions since we only blit the pixel-aligned interior of the blit region.

This patch fixes the problem by rounding out the bounding box of the moving content to device pixel boundaries. This is safe since we only need a conservative approximation here.

Comment 4

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

Attachment: test framework

This test framework uses the MozAfterPaint infrastructure from the patch in bug 510110. It tests the problem I noted in the previous attachment in this bug.

Comment 5

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

Attachment: fix

This patch fixes this bug, at least as originally reported. The problem is the border on the scrolling element. Because we extend 'rect' to include the source of aUpdateArea, 'rect' overlaps the border of the scrolling element, which isn't moving, so we decide there's non-moving content visible that covers the whole scrolled area.

There is actually no need to extend 'rect' to include the source of aUpdateArea, nor the similar thing for the visible region, so I've removed that.

Comment 6

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

Stuart, you might want to see if the non-test patches in this bug fix your scrolling issues.

Comment 7

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

Attachment: a couple more scrolling tests

Comment 8

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

Attachment: Scrolling debugger script

This uses the new MozAfterPaint features to help debug scrolling problems. Save it somewhere and add it to a page using <script src="..."></script>. When you scroll in that page, it will flash yellow where we repainted an area by blitting, and orange where we repainted an area manually. Areas that don't flash yellow or orange were not repainted at all.

Don't trigger scrolling operations too frequently, since the elements that script adds to highlight areas can actually affect scrolling and produce confusing results. Just wait for the highlights to be removed before triggering another scroll operation.

Comment 9

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

(In reply to comment #3)
> This patch fixes the problem by rounding out the bounding box of the moving
> content to device pixel boundaries. This is safe since we only need a
> conservative approximation here.

Why is rounding out conservative in this case?  And wouldn't rounding to nearest the same way we actually round boundaries (when that's how we're rounding boundaries, i.e., when not transformed) be more accurate?

Comment 10

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

(In reply to comment #9)
> (In reply to comment #3)
> > This patch fixes the problem by rounding out the bounding box of the moving
> > content to device pixel boundaries. This is safe since we only need a
> > conservative approximation here.
> 
> Why is rounding out conservative in this case?

It's OK for our computed "visible area of moving content" to be larger than the true area. After all, currently we assume the visible area of moving content is the entire scrolled area.

> And wouldn't rounding to
> nearest the same way we actually round boundaries (when that's how we're
> rounding boundaries, i.e., when not transformed) be more accurate?

Yes. I could do that if you think it's worth it, although until we have a testcase that really needs that, I'd prefer to not rely on it.

Comment 11

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

Is it ever problematic if the rounding makes it extend outside of the scrolled area?  Or are we intersecting it with the scrolled area afterward anyway?  (Or did we do that before?)

Comment 12

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

We have this:
  visibleRegionOfMovingContent.And(visibleRegionOfMovingContent, aUpdateRect);
which limits the visibleRegionOfMovingContent to the scrolled area.

Comment 13

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

Comment on attachment 397577 [details] [diff] [review]
round out moving content bounds to device pixels

ok, r=dbaron

Comment 14

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

Comment on attachment 397578 [details] [diff] [review]
test framework

r=dbaron, although region_lib.js might benefit from putting the functions on Region.prototype rather than directly on each region object (it avoids complex closures, at least)

Comment 15

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

I'm having trouble understanding how it's ok to exclude items that are within aUpdateRect in the before state but scrolled out of it in the after (current) state from the display list.  (Doesn't the rect you pass to BuildDisplayListForStackingContext do that?)

Do any of the comments in nsLayoutUtils::ComputeRepaintRegionForCopy or nsDisplayItem::OptimizeVisibility need fixing?

Comment 16

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

(In reply to comment #14)
> (From update of attachment 397578 [details] [diff] [review])
> r=dbaron, although region_lib.js might benefit from putting the functions on
> Region.prototype rather than directly on each region object (it avoids complex
> closures, at least)

Good idea. I know very little about writing Javascript libraries...

(In reply to comment #15)
> I'm having trouble understanding how it's ok to exclude items that are within
> aUpdateRect in the before state but scrolled out of it in the after (current)
> state from the display list.  (Doesn't the rect you pass to
> BuildDisplayListForStackingContext do that?)

aRepaintRegion consists of:
   * a) any visible background-attachment:fixed areas in the after-move display
   * list
   * b) any visible areas of the before-move display list corresponding to
   * frames that will not move (translated by aDelta)
   * c) any visible areas of the after-move display list corresponding to
   * frames that did not move
So content that was moved out of view cannot affect aRepaintRegion.

aBlitRegion depends on the visible moving area, which does depend on the content visible before the move as well as afterward. nsDisplayListBuilder::AccumulateVisibleRegionOfMovingContent is applied to all moving content that intersects the update area after the move, and accumulates both the old and new position of that content. For the content that has been moved out of the update area, we have a problem, which I intended to solve with nsDisplayClip::OptimizeVisibility:

    // There may be some clipped moving children that were visible before
    // but are clipped out now. Conservatively assume they were there
    // and add their possible area to the visible region of moving
    // content.
    // Compute the after-move region of moving content that could have been
    // totally clipped out.

... plus the fact that we always scroll a rectangle that matches the rectangle of an nsDisplayClip.

However, I guess ComputeRepaintRegionForCopy shouldn't make that latter assumption. I should modify it to add the region that might have contained content that has moved out of view to the visible moving area.

> Do any of the comments in nsLayoutUtils::ComputeRepaintRegionForCopy or
> nsDisplayItem::OptimizeVisibility need fixing?

No, although I did find one mistake from bug 507334 that I should fix.

Comment 17

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

Checked in attachments 397577 and 397578:
http://hg.mozilla.org/mozilla-central/rev/275cb46bca42
http://hg.mozilla.org/mozilla-central/rev/2b23795830c8

Comment 18

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

Attachment: fix v2

Updated patch as described above.

Comment 19

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

(In reply to comment #16)
> aRepaintRegion consists of:

>    * b) any visible areas of the before-move display list corresponding to
>    * frames that will not move (translated by aDelta)
>    * c) any visible areas of the after-move display list corresponding to
>    * frames that did not move

Do these include display items that are uniform?  If it doesn't, then you'd need to include areas that might have covered the uniform areas in the before-move list but no longer do in the after-move list.

Comment 20

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

Yes, the intersection of the before-move and after-move areas of a uniform display item is not added to aRepaintRegion.

However, in nsDisplayItem::OptimizeVisibility, moving opaque frames are only treated as covering the area that's the intersection of their before-position and after-position:

      // The display list should include items for both the before and after
      // states (see nsLayoutUtils::ComputeRepaintRegionForCopy. So the
      // only area we want to cover is the area that was opaque in the
      // before state and in the after state.
      opaqueArea.IntersectRect(bounds - aBuilder->GetMoveDelta(), bounds);

So anything that intersects aUpdateRect and is visible before *or* after the scroll will be in the post-OptimizeVisibility display list.

I'm not sure if this fully answers your question, though.

Comment 21

[Danial Horton]

current scrolling performane is preferable fwiw.

Comment 22

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

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

r=dbaron

Comment 23

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

First two patches landed on 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d48e4c01d55f
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3fb12be346e2

Comment 24

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

Checked in fix v2:
http://hg.mozilla.org/mozilla-central/rev/62b992597da6
and additional tests:
http://hg.mozilla.org/mozilla-central/rev/5d82c6bfa38a

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eff52bef0336
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3281b1dbcae9