47549 - Floaters cause entire page to redraw as content is appended (block paint perf)

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Simon Fraser [no longer active]]

In a build with double-buffering off, load the above URL. Note how, while 
loading, the entire page redraws many, many times, and then, when the page is 
loaded, how it flickers as the images animate.

This suggests that there are some invalidation/redraw bugs that are wrongly 
dirtying the entire window.

Comment 1

[Kevin McCluskey (gone)]

Looks like full page invalidates may be caused by floaters.

I set a breakpoint in Invalidate for large rectangles (300px X 300px) This is 
the stack trace the majority of the time this is triggered:

nsWindow::Invalidate(nsWindow * const 0x0382e4b4, const nsRect & {...}, int 0) 
line 1798
nsViewManager2::UpdateView(nsViewManager2 * const 0x037b6d30, nsIView * 
0x0382be70, const nsRect & {...}, unsigned int 4) line 1186
nsFrame::Invalidate(nsIPresContext * 0x03735ba0, const nsRect & {...}, int 0) 
line 1989
nsBlockFrame::ReflowFloater(nsBlockReflowState & {...}, nsPlaceholderFrame * 
0x00d32750, nsRect & {...}, nsMargin & {...}, nsMargin & {...}) line 5629
nsBlockReflowState::AddFloater(nsLineLayout & {...}, nsPlaceholderFrame * 
0x00d32750, int 0) line 5727
nsLineLayout::AddFloater(nsPlaceholderFrame * 0x00d32750) line 597
nsLineLayout::ReflowFrame(nsIFrame * 0x00d32750, nsIFrame * * 0x0012d7c8, 
unsigned int & 0, nsHTMLReflowMetrics * 0x00000000, int & 0) line 960
nsInlineFrame::ReflowInlineFrame(nsIPresContext * 0x03735ba0, const 
nsHTMLReflowState & {...}, nsInlineFrame::InlineReflowState & {...}, nsIFrame * 
0x00d32750, unsigned int & 0) line 563 + 26 bytes
nsInlineFrame::ReflowFrames(nsIPresContext * 0x03735ba0, const nsHTMLReflowState 
& {...}, nsInlineFrame::InlineReflowState & {...}, nsHTMLReflowMetrics & {...}, 
unsigned int & 0) line 412 + 28 bytes
nsInlineFrame::Reflow(nsInlineFrame * const 0x00d32678, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 323 + 28 bytes
nsLineLayout::ReflowFrame(nsIFrame * 0x00d32678, nsIFrame * * 0x0012e470, 
unsigned int & 0, nsHTMLReflowMetrics * 0x00000000, int & 0) line 937
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x0312b2b8, nsIFrame * 0x00d32678, unsigned char * 
0x0012d9e8) line 4336 + 29 bytes
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x0312b2b8, int * 0x0012e064, unsigned char * 0x0012deac, int 
0, int 1) line 4220 + 28 bytes
nsBlockFrame::DoReflowInlineFramesAuto(nsBlockReflowState & {...}, nsLineBox * 
0x0312b2b8, int * 0x0012e064, unsigned char * 0x0012deac, int 0, int 1) line 
4156 + 42 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineBox * 
0x0312b2b8, int * 0x0012e064, int 1, int 0) line 4101 + 32 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x0312b2b8, int 
* 0x0012e064, int 1) line 3236 + 29 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2925 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x00d325a0, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1729 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Incremental, nsIFrame * 0x00d325a0, const nsRect & {...}, int 1, 
int 0, int 1, nsMargin & {...}, unsigned int & 0) line 506 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x00d325a0, const nsRect & {...}, 
int 1, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 331 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x0312b330, int * 0x0012ebcc) line 3854 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x0312b330, int 
* 0x0012ebcc, int 1) line 3118 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2925 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x00d32518, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1729 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x00d32518, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x00d31844, nsIPresContext * 0x03735ba0, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 306
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0, int 0, int 0, int 6270, int 8460, int 1) line 813
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x00d324b0, 
nsBoxLayoutState & {...}) line 484 + 52 bytes
nsBox::Layout(nsBox * const 0x00d324b0, nsBoxLayoutState & {...}) line 1002
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x00d3195c, nsBoxLayoutState 
& {...}) line 377
nsBox::Layout(nsBox * const 0x00d3195c, nsBoxLayoutState & {...}) line 1002
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x00d3195c, 
const nsRect & {...}) line 593 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x00d3195c, 
const nsRect & {...}) line 1009 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1092
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x00d318b8, nsBoxLayoutState 
& {...}) line 1017 + 15 bytes
nsBox::Layout(nsBox * const 0x00d318b8, nsBoxLayoutState & {...}) line 1002
nsBoxFrame::Reflow(nsBoxFrame * const 0x00d31880, nsIPresContext * 0x03735ba0, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 728
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x00d31880, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 725 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x00d31880, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x00d31808, nsIPresContext * 
0x03735ba0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 546
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x03e3d610, 
nsIPresContext * 0x03735ba0, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 145
PresShell::ProcessReflowCommands(int 1) line 4230
ReflowEvent::HandleEvent() line 4119
HandlePLEvent(ReflowEvent * 0x03e3d5c0) line 4129
PL_HandleEvent(PLEvent * 0x03e3d5c0) line 587 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x0175c380) line 528 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x006f04da, unsigned int 49434, unsigned int 0, 
long 24494976) line 1043 + 9 bytes
USER32! 77e71820()
0175c380()

CC'ing buster, attinasi
Added perf keyword

Comment 2

[Kevin McCluskey (gone)]

Changing platform to ALL. It has the same bad behavior on WINNT

Comment 3

[Kevin McCluskey (gone)]

This bug has been marked "future" because the original netscape engineer working
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way
-- please attach your concern to the bug for reconsideration.

Comment 4

[Kevin McCluskey (gone)]

Set milestone to mozilla0.8

Comment 5

[Kevin McCluskey (gone)]

the majority of the full-page invalidates are result of code in 
nsBlockReflowContext::ReflowBlock

At the point where it's setting the reflow reason, the state of the frame is 
dirty and it ends up setting the reflow reason to resize. Setting it to resize 
causes the entire block to be invalidated because the block assumes there may be 
text frames that it's holding that do not how to invalidate themselves.
 
      if (eReflowReason_Resize == reason) {
        // we're doing a resize reflow, even though our outer reflow state is 
incremental
        // text (and possibly other objects) don't do incremental painting for 
resize reflows
        // so, we have to handle the invalidation for repainting ourselves
        mBlockShouldInvalidateItself = PR_TRUE;
      }

We need to add code to frame's that doesn't know how to invalidate itself and 
stop setting the mBlockShouldInvalidateItself as a catch all.

The net effect of the current code is that every time a frame is appended to a 
block we end up repainting all of the visible frames.

I looked at the nsTextFrame's reflow code and it does not invalidate the frame, 
so the BlockFrame is correct in it's assumption that textframes do not 
invalidate.
 

Comment 6

[Kevin McCluskey (gone)]

Changed summary from:
Entire page redraws as images animate
to:
Floaters cause entire page to redraw as content is appended

Comment 7

[Kevin McCluskey (gone)]

I did some timing tests on cnn and invidual.com on Linux loading documents which
contain floaters. Floaters typically cause the entire cnn.com page to be redrawn
20-26 times during the page load. My goal was to determine how the overall page
load performance is impacted by the extra redraws.

I loaded the pages using a release build and a T1 connection. I loaded CNN.com 5
times and recorded the average time. I commented out the invalidates performed
by nsBlockFrame.cpp then I loaded it 5 times and recorded the average. I
repeated the process again, loading CNN.com 5 times with the invalidate code
followed by 5 times without the invalidates.

The overall page load is 8-10% faster using the nsBlockFrame.cpp which has the
invalidates commented out.

I did the same test with invidual.com and the overall page load was approx 5%
faster using the nsBockFrame.cpp code with the invalidates commented out.

The extra invalidates are the result of the reflow logic in nsBlockFrame.cpp for
floaters. It assumes that it must invalidate the entire block each time content
is appended.

The impact of extra invalidates in the BlockFrame code should be highly
dependent on the display depth, display resolution, and video card, as well as
the network connection, so the impact of the extra invalidates may not always be
noticeable. I was running with 32 bit, 1024X768 resolution and I had the browser
window maximized.

There is a bugzilla bug filed on this: 47549 - Floaters cause entire page to
redraw as content is appended.

Some other sites which may be impacted by this include:
www.hotwired.com
www.autoweb.com
www.usatoday.com
www.hotbot.com

They also paint the entire document many times while the page is loading.

Comment 8

[Kevin McCluskey (gone)]

Reassigning to buster.

Comment 9

[buster]

accepting

Comment 10

[buster]

kevin:  I'm having a hard time measuring the impact of my changes.  I must be
doing something goofy.  I'll attach them here, so you can test them if you have
any free time.  This code is *not* a fix, it's an experiment to try to
understand the relationship between text frame invalidation and containing block
invalidation.  Anyway, I'd be interested to hear how this changes your
measurements.  I'll be in the office all day tomorrow if you want to do this
together.

Comment 11

[buster]

Attachment: simple test code to turn off forced invalidation of the block, and turn on invalidation for resize reflows in the text frame

Comment 12

[Kevin McCluskey (gone)]

I ran timings with buster's patch on Linux.

the URL in the test case loaded 7% faster with the patch.

Comment 13

[Kevin McCluskey (gone)]

Steve: I haven't seen any problems where the window has not been invalidated
completely, but the patch causes paint's to leak outside of textfields - so it
slows down text input for text fields.

Comment 14

[buster]

that may actually be an independent problem.  are you saying that you're seeing
excessive painting from text controls with the patch, but not without?  or you
just noticed them with the patch, and haven't yet confirmed one way or the other
what the behavior was before?

Comment 15

[Kevin McCluskey (gone)]

"are you saying that you're seeing
excessive painting from text controls with the patch, but not without?"

Yes. With the patch installed I see extra painting when typing into text fields
that I don't see without the patch.

Comment 16

[Kevin McCluskey (gone)]

Here's another place which is causing full-page paints is in nsBlockFrame.cpp

/**
 * Reflow a line. The line will either contain a single block frame
 * or contain 1 or more inline frames. aKeepReflowGoing indicates
 * whether or not the caller should continue to reflow more lines.
 */
nsresult
nsBlockFrame::ReflowLine(nsBlockReflowState& aState,
                         nsLineBox* aLine,
                         PRBool* aKeepReflowGoing,
                         PRBool aDamageDirtyArea)
...

     if (oldCombinedArea.height != lineCombinedArea.height) {
          nsRect  dirtyRect;

          // Just damage the horizontal strip that was either added or went
          // away
          dirtyRect.x = lineCombinedArea.x;
          dirtyRect.y = PR_MIN(oldCombinedArea.YMost(),
                               lineCombinedArea.YMost());
          dirtyRect.width = PR_MAX(oldCombinedArea.width,
                                   lineCombinedArea.width);
          dirtyRect.height = PR_MAX(oldCombinedArea.YMost(),
                                    lineCombinedArea.YMost()) -
                             dirtyRect.y;
#ifdef NOISY_BLOCK_INVALIDATE
          printf("%p invalidate 8 (%d, %d, %d, %d)\n",
                 this, dirtyRect.x, dirtyRect.y, dirtyRect.width, dirtyRect.height);
#endif
        ====> Invalidate(aState.mPresContext, dirtyRect);
        }

If the invalidate is commented out when just the vertical height changes, It
eliminates the bulk of the remaining full page paints on this page.

Comment 17

[Kevin McCluskey (gone)]

A few remaining full page repaints are generated by the following code in
nsTableFrame.cpp:

/* Layout the entire inner table. */
NS_METHOD nsTableFrame::Reflow(nsIPresContext* aPresContext,
                               nsHTMLReflowMetrics& aDesiredSize,
                               const nsHTMLReflowState& aReflowState,
                               nsReflowStatus& aStatus)

...
 // If this is an incremental reflow and we're here that means we had to
      // reflow all the rows, e.g., the column widths changed. We need to make
      // sure that any damaged areas are repainted
      if (eReflowReason_Incremental == aReflowState.reason) {
        nsRect  damageRect;

        damageRect.x = 0;
        damageRect.y = 0;
        damageRect.width = mRect.width;
        damageRect.height = mRect.height;
       ==> Invalidate(aPresContext, damageRect);
      }

Comment 18

[buster]

fixed.

Comment 19

[Chris Petersen]

Marking fixed in the March 5 build.