User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a1pre) Gecko/20090826 Firefox/3.7a1pre Build Identifier: Scrolling in Google Reader on Linux trunk is currently very slow. I did some hg mozilla-central checkouts and it looks like it was caused by bug 339548 or bug 352093. Before the patches for those bugs were checked in, scrolling was acceptable. I'm not sure if future compositor work would address this issue, but right now it's a noticeable regression. Reproducible: Always

I can confirm this on Trunk and 1.9.2 on Windows 7. I think it might be related to Bug 435056.

Wonder if it's a regression from bug 507334 and fixed by the patches in bug 510856.

I applied the patches in bug 510856 to a trunk checkout, but it didn't fix the regression I'm seeing.

OK, I'll need a reduced testcase I guess

I did some analysis of Google Reader. Looks like the problem is that the scrolling area is not pixel-aligned. When scrolling down, this causes us to repaint a single-pixel-high strip at the top of the scrolling area as well as the scrolled-into-view strip at the bottom of the scrolling area. This isn't so bad in itself, since the repaint region is still small in area, but when we repaint our dirty rect covers the whole scrolling area. Then it seems we spend a *lot* of time painting the -moz-box-shadows for the visible entries; they have a 1-pixel blur, and since our dirty rect is large, we paint the shadows to temporary surfaces which are pretty large and do a Gaussian blur on them... So there are a few things we can do here: 1) Make blurs faster (SSE etc) 2) Compute a per-display-item dirty rect by intersecting the item bounds with the visible region, either for expensive display items or for all display items 3) Fix the regression so we don't paint that extra one-pixel strip for non-pixel-aligned scrolling areas

(In reply to comment #5) > 3) Fix the regression so we don't paint that extra one-pixel strip for > non-pixel-aligned scrolling areas I'm all for this one; I was worried that might be a problem (bug 507334 comment 28).

OK, I fixed that problem with the stray rectangles, and I'll attach the fix, but that's not the problem. We still get a large dirty rect because the scrollbar is being invalidated as well as the bottom strip of the window, so we still end up computing large blurs. This probably didn't happen when we had a widget for the scrollable area, because the dirty region would be separated into the region inside the widget and the region outside the widget. I think I'll need to fix #2 here as well.

Stuart, this patch may help scrolling on some pages.

This plus the previous patch fixes performance for me. Just compute a better dirty rect during OptimizeVisibility. Possibly we should use this for other display items that can benefit from a smaller dirty rect, although I don't know of any that should do that right now.

The second patch fixes a sort-of-regression from bug 352093, and the first patch fixes a regression from bug 507334.

Comment on attachment 398591 [details] [diff] [review] part 2: reduce dirty rects for box-shadows Actually I think we should just do this for all display items and drop the aDirtyRect parameter from Paint.

I wanted to test the patches, but they failed to apply for me.

-- Rename OptimizeVisiblity to ComputeVisibility -- Make it mandatory before calling Paint (and fix the one place I found where we aren't calling it already before Paint) -- ... because for every display item we'll compute its intersection with the visible region and store that in mVisibleRect -- ... and remove aDirtyRect from Paint, items should use mVisibleRect instead -- All the ComputeVisibility work that should be done for every display item is moved to nsDisplayList::ComputeVisibility, to make sure it always happens even if ComputeVisibility is overridden

The patches in this bug apply on top of the patches in bug 510856.

Comment on attachment 398580 [details] [diff] [review] necessary but not sufficient As far as the tests go: it seems worth testing not only that you do blit, but that you don't repaint more than needed. It seems like it's also worth testing both top: 0.2px and top:0.8px, and/or scrolling in both directions. >+ * contained in aBlitRegion. Whatever appunit area was removed from >+ * aBlitRegion in that process is added to aRepaintRegion. An appunits Does this really need to be true for the area removed due to the rounding? Aren't all areas that we can blit inside of actually pixel-snapped? r=dbaron

(In reply to comment #17) > (From update of attachment 398580 [details] [diff] [review]) > As far as the tests go: it seems worth testing not only that you do blit, but > that you don't repaint more than needed. Sure OK. > It seems like it's also worth testing both top: 0.2px and top:0.8px, and/or > scrolling in both directions. OK. > >+ * contained in aBlitRegion. Whatever appunit area was removed from > >+ * aBlitRegion in that process is added to aRepaintRegion. An appunits > > Does this really need to be true for the area removed due to the rounding? > Aren't all areas that we can blit inside of actually pixel-snapped? I'm not sure what you mean. Some content, like text, isn't pixel-snapped when we draw.

Oh, right; we're looking at the display items for the things that are blitted and not the scrollable area. So never mind.

Comment on attachment 398989 [details] [diff] [review] part 2: make all display item Paint methods use a dirty rect obtained via the visible region >+ // This is the rectangle that needs to be painted. Set by >+ // nsDisplayList::ComputeVisibility, used by Paint implementations. >+ // Guaranteed to be contained in GetBounds(). >+ nsRect mVisibleRect; Might be good to say how it's related to the aVisibleRegion passed to ComputeVisibility. >+ * sets mPaintRect on each display item. mVisibleRect? Should nsDisplayOpacity::ComputeVisibility now use mVisibleRect instead of GetBounds()? (Also, it occurs to me we should probably optimize the case of opacity being 0 a little more, unless I'm missing something we're already doing.) Why does nsDisplayTransform::ComputeVisibility compute |visible| using aVisibleRegion's bounds rather than using mVisibleRect? Isn't mVisibleRect always at least a small, and appropriate? Can the calculation at the start of nsDisplaySVGEffects::ComputeVisibility be replaced by using mVisibleRect? Can you add assertions to something (nsDisplayList::Paint? or is there some common nsDisplayItem paint code?) to check that ComputeVisibility was called? (e.g., check that mVisibleArea is nonempty, at least on the first item?) r=dbaron with that, though feel free to postpone the additional cleanup bits to later if you want

(In reply to comment #20) > (From update of attachment 398989 [details] [diff] [review]) > >+ // This is the rectangle that needs to be painted. Set by > >+ // nsDisplayList::ComputeVisibility, used by Paint implementations. > >+ // Guaranteed to be contained in GetBounds(). > >+ nsRect mVisibleRect; > > Might be good to say how it's related to the aVisibleRegion passed to > ComputeVisibility. Sure. > >+ * sets mPaintRect on each display item. > > mVisibleRect? Yep. > Should nsDisplayOpacity::ComputeVisibility now use mVisibleRect instead > of GetBounds()? You mean here? mNeedAlpha = visibleUnderChildren.Intersects(GetBounds(aBuilder)); Yes, I suppose we might as well, since it's simpler. It would only actually help rather weird cases ... when the element subtree is transparent, but the transparent bits are covered up by other opaque content, then we don't need an alpha channel in the temporary surface, which can make some drawing a bit faster. > (Also, it occurs to me we should probably optimize the > case of opacity being 0 a little more, unless I'm missing something > we're already doing.) Yes, we should, but it would be optimal to check that in nsIFrame::BuildDisplayListForStackingContext so we don't even build the display list items (for non-event-handling situations of course). > Why does nsDisplayTransform::ComputeVisibility compute |visible| using > aVisibleRegion's bounds rather than using mVisibleRect? Isn't > mVisibleRect always at least a small, and appropriate? Yes, good catch. > Can the calculation at the start of > nsDisplaySVGEffects::ComputeVisibility be replaced by using > mVisibleRect? Yes. > Can you add assertions to something (nsDisplayList::Paint? or is there > some common nsDisplayItem paint code?) to check that ComputeVisibility > was called? (e.g., check that mVisibleArea is nonempty, at least on the > first item?) Best thing to do is probably to just add a debug-only boolean to nsDisplayList to record whether ComputeVisibility was called, and check it in nsDisplayList::Paint.

I added tests for top:0.8px but they don't pass because we repaint a sliver between y=0.8 and y=1.0 (everything below 1.0 is blitted), and fixing that looks hard :-(. I'm going to check the tests in as todo. To fix this we need to take advantage of the fact that nsDisplayClip's clip rect is snapped to device pixels --- in this case, its top edge snaps to y=1.0, so that sliver of unblitted area isn't actually visible.

OK, that wasn't so bad. In two places we take advantage of our knowledge that the nsDisplayClip used by a blitting scrolling frame must snap to device pixels: nsViewManager::GetRegionsForBlit and AddItemsToRegion.

I've discovered a problem in the mVisibleRect setup which manifests as a subtle reftest failure on Mac. Basically we were relying on the fact that aDirtyRect was (usually) pixel-aligned. Now that we use mVisibleRect which is always intersected with the element's bounds, it's less often pixel-aligned. I think we just need to stop relying on aDirtyRect being pixel-aligned.

It wasn't necessarily pixel-aligned before, since for example nsDisplayClip would intersect it with mClip which wasn't always pixel-aligned.

This fixes the reftest regressions that I found on the try-servers. When we're drawing partial ligatures, we clip the glyph(s) to the bounds of the dirty rect. Currently the overflow area for the text frame doesn't include some antialiased pixels on Mac and Windows, so they're not included in mVisibleRect and get clipped off when we're drawing partial ligatures. The quick fix here is to inflate mVisibleRect a little bit before passing it to the frame. The long-term fix is to land Jonathan Kew's patches to add those antialised pixels to the text ink extents. But that requires splitting overflow areas into two kinds ("ink overflow" vs "scrollable overflow"), so it's longer term.

Checked in parts 1 and 2: http://hg.mozilla.org/mozilla-central/rev/6bfb2b05290f http://hg.mozilla.org/mozilla-central/rev/b540248e3163

Actually I'll mark this 'fixed' since the P1-ness is fixed.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5366d0241663 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b16d576a6602

Comment on attachment 403178 [details] [diff] [review] Part 3: fix todos r=dbaron

part 3 checked in: http://hg.mozilla.org/mozilla-central/rev/e8635ca5bc94