530686 - Scrolling Google Reader has gotten much worse.

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

I've put a test case up at:

http://people.mozilla.org/~jmuizelaar/reader/reader.htm

It is worse in both 3.6 and trunk than 3.5

Comment 1

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

What happens here is that the fixed-pos text "View settings..." overflows microscopically into the scrolling text below:

    Text 0x9dc2830(Text(0)) (180,192,5211,900)
    Clip 0x0() (180,1105,79920,37731)

The first display item is the text, the second one is the clip region. The clip region gets snapped to pixels so its actual y-coordinate is at 1080 appunits.

Then, when we're scrolling down by lines, we're repainting a single row of pixels at the top and a line's worth of pixels at the bottom. Thus the bounding rectangle for the intersection of the visible region and the box-shadow is really large, and we draw a really large box-shadow, which is slow.

I'm not quite sure what to do here. It's unfortunate that pixel-snapping the scroll area makes it intersect with the text, but if the text used a font with unusually large descenders, it would intersect anyway.

Perhaps we should just speed up box-shadow. One option would be to store a visible region for nsDisplayBoxShadow instead of just a rect, and for disconnected regions like this just paint each part independently. Or we could speed up the blurring, see bug 509052.

Comment 2

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

Attachment: fix

This makes it pretty good.

Comment 3

[David Baron :dbaron:]

Comment on attachment 414439 [details] [diff] [review]
fix

>+  nsRect borderRect = nsRect(offset, mFrame->GetSize());

Probably better to write
nsRect borderRect(offset, mFrame->GetSize());

r=dbaron

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Why did this get so much worse in the first place and why do 3.6 and trunk have such different behavior? I should also note, that the problems were also really only noticeable on win32 (win7)

Comment 5

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

This would have been a regression from bug 513082, which snapped the clip area to pixels during display list analysis for scrolling, thus making it appear to overlap the text.

Bug 513082 was itself a fix for problems introduced by bug 507334. And bug 507334 reworked some of our scrolling analysis to enable optimizations needed for the Fennec UI.

Comment 6

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

Attachment: fix v2

While testing bug 531461 I noticed that ComputeDisjointRectangles was embarrassingly broken. In particular we failed to reset 'accumulated' to empty after using it, and if a subsequent region rect failed to be accumulated into the existing 'accumulated', we'd just ignore it.

Comment 7

[David Baron :dbaron:]

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

This seems fine, although adding only the single accumulated.Empty() line to the previous patch also does (and perhaps faster).  Or is there a difference I'm missing?

Comment 8

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

Just adding accumulated.Empty() is not enough. If we do that, then in any iteration where r is non-null and we flush 'accumulated', *r is ignored.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1a0f2d658a49

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/035882e47ab3

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8741cf0d0621 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2cd073bb2314 to fix the reftest orange due to the test ref name being typoed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

And the reftest reference itself had a spurious reftests-wait too.  Pushed http://hg.mozilla.org/mozilla-central/rev/54e9fe755ea4 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/86a1fb0acac3 to fix.

Comment 13

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

Sorry :-( thanks for fixing that up.

Comment 14

[Bryan]

Is this the same issue that now appears on http://www.boygeniusreport.com/ ???  Scrolling is now really choppy and laggy and I don't believe it was this was some time ago.

BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091201 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091201022333

~B

Comment 15

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

Can you file a new bug for that? (It seems fine to me on my latest Mac and Windows builds.)

Comment 16

[Jeff Muizelaar [:jrmuizel]]

This is still much worse than 3.5:

On this page (http://people.mozilla.org/~jmuizelaar/reader/reader-auto.htm) I get 

took 87371ms
5218avg:83.720774243005
max:115,465

with this build:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091202 Minefield/3.7a1pre (.NET CLR 3.5.30729)

vs.

3.5 which gives:

took 87371ms
5218avg:83.720774243005
max:115,465

Comment 17

[Jeff Muizelaar [:jrmuizel]]

More recent nightlies have improved though. A build from 11/11 gives:

took 102318ms
5218avg:98.04331161364509
max:120,4855

Comment 18

[David Baron :dbaron:]

The two sets of numbers in comment 16 are the same.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Indeed, the actual numbers for 3.5 are:

took 16788ms
5218avg:16.08662322729015
max:108,0

Comment 20

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

I don't know why I didn't see this before, but the scrolled content is inside a table, for which we create an nsTableBorderBackground. This doesn't move when we scroll, so we think we need to repaint most of the window to re-composite the scrolled content over the background.

Comment 21

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

Attachment: ignore tables

OK, this takes the patch in bug 531461 a step further, by creating nsDisplayTableBorderBackground only if a table part actually has a border or background.

Comment 22

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

Attachment: ignore tables v2

Oops, we need to null-check currentItem in nsTableCellFrame now too.

Comment 23

[David Baron :dbaron:]

Comment on attachment 415797 [details] [diff] [review]
ignore tables v2

>+       aFrame->GetStyleDisplay()->mAppearance ||

This may as well be aFrame->IsThemed()

r=dbaron

Comment 24

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

I saw a crash in AnyTablePartHasBorderOrBackground. It may have been related to my build being incorrectly partially rebuilt, but I'm not sure, so I've pushed the build to the try-server and not checking in for now.

Comment 25

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

OK, all tests passed on all three platforms, so I guess we should push this.

Comment 26

[David Baron :dbaron:]

http://hg.mozilla.org/mozilla-central/rev/18afa623a64f

Comment 27

[David Baron :dbaron:]

Hmmm. Casuing bustage because this:

../../../dist/include/nsStyleStruct.h:831: warning: inline function 'PRBool nsStyleBorder::IsBorderImageLoaded() const' used but never defined

leads to linkage errors.  I'll try moving the function to be non-inline.

Comment 28

[David Baron :dbaron:]

Bustage fix (moved back to non-inline):
http://hg.mozilla.org/mozilla-central/rev/ecb85a29af12

Comment 29

[David Baron :dbaron:]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dcdb623dd8d9

Comment 30

[Jeff Muizelaar [:jrmuizel]]

A more recent nightly gives:

took 13809ms
5218avg:13.232081257186662
max:1022,2905

which is even better than we had before (though the max number is unusually high).