Closed Bug 90503 Opened 23 years ago Closed 23 years ago

Table repainted every reflow

Categories

(Core :: Layout, defect)

x86
Windows 2000
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: bratell, Assigned: attinasi)

References

()

Details

(Keywords: perf, regression)

Attachments

(1 file)

I was playing with the table stress case in Quantify and noticed that we 
repaints all table texts (probably the whole table) repeatadly. During the load 
quantify counts 24500 PaintAsciiText:s but I only see a couple of hundred table 
cells on the screen. They don't move more than maybe once or twice so there are 
no need to paint them all tens or hundreds of times. 

I don't know how to debug problems like these so I thought I should file a bug 
and maybe get someone's attention. Someone who could tell me:
 a) Why it has to be that way
 b) That it is a known (hard to fix) bug
 c) That it is already fixed
 d) How to debug (and fix?) it

The painting (the time under nsView::Paint) is 40% of the load time so I guess 
there is room for improvement.

This was with a build from 8-9 July or there about.
Blocks: 54542
Keywords: perf
I'm guessing (wildly) that the whole table is marked dirty even though nothing 
(visible) is changed and it's therefore repainted. 


CC:ing waterson who has helped me with layout issues in the past. You'll regret 
that. :-)
The only thing I can find out is that the whole page area is marked as wanting a 
paint in nsWindow::OnPaint() and there are no more functions on the stack to say 
who set the whole area as dirty. What should I look for to find code that 
invalidates the area? Help? Someone?
It seems to be related to the scrollbar. Every time the scrollbar is updated, 
the whole page is repainted. My guess: The invalidate for the scrollbar area 
somehow causes an invalidate of the whole area even though the only thing 
changing is the scroll bar. What do you think?

The top of stack for a typical Invalidate is:

nsWindow::Invalidate(nsWindow * const 0x04beaa5c, int 0) line 2024
nsWindow::Resize(nsWindow * const 0x04beaa5c, int 821, int 454, int 1) line 1614
nsView::SetDimensions(nsView * const 0x03fcd9f8, int 12315, int 6810, int 1) 
line 607
nsScrollPortView::SetDimensions(nsScrollPortView * const 0x03fcd9f8, int 12315, 
int 6810, int 1) line 114
nsViewManager::ResizeView(nsViewManager * const 0x055c57f0, nsIView * 
0x03fcd9f8, int 12315, int 6810, int 0) line 2333
nsContainerFrame::SyncFrameViewAfterReflow(nsIPresContext * 0x04da7cb0, nsIFrame 
* 0x0610b4dc, nsIView * 0x03fcd9f8, const nsRect * 0x00000000, unsigned int 0) 
line 505
nsBox::SyncLayout(nsBoxLayoutState & {...}) line 1062 + 23 bytes
nsBoxFrame::SyncLayout(nsBoxLayoutState & {...}) line 1253 + 12 bytes
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x0610b514, nsBoxLayoutState 
& {...}) line 408
nsBox::Layout(nsBox * const 0x0610b514, nsBoxLayoutState & {...}) line 985
...
Summary: To many repaints on table → Too many repaints of table
So that was probably not right either. 

Now I'm looking at the nsTextFrame reflows. That is kind of hard code for 
someone not used to gecko but I'll try and see what I find.

There about 20000 nsTextFrame::Reflows which I guess is a reasonable number 
since there are 10000 table cells. At worst that is 2 reflows per frame.

All these do a nsFrame::Invalidate. Could that invalidation affect anything 
beside the frame itself?

It's like the full paint occurs every time there is a reflow of any text 
frames...
Adding dbaron and attinasi, who seems to be the experts in this area, to the CC 
list. 
Summary: Too many repaints of table → Table repainted every reflow
New theory: 
nsContainerFrame::SyncFrameViewAfterReflow() resizes the table box which causes 
a resize of the View which causes an invalidation of the whole window despite 
the fact that nothing has changed on screen. Could this be right?

There must be a way to see that all that has changed is stuff outside the 
visible area? I see a big win here. Remember that painting was a stunning 40% of 
the total time for this page (during a quantify run which causes more 
timer triggered reflows which caused more repaints so the gain is less than 
40% in reality).

The stack for this view resizing is:

nsViewManager::UpdateView(nsViewManager * const 0x0382dff8, nsIView * 
0x03881a30, const nsRect & {...}, unsigned int 4) line 1814
nsViewManager::ResizeView(nsViewManager * const 0x0382dff8, nsIView * 
0x0376a4b8, int 76980, int 6300, int 0) line 2347
nsContainerFrame::SyncFrameViewAfterReflow(nsIPresContext * 0x037d4108, nsIFrame 
* 0x038b5124, nsIView * 0x0376a4b8, const nsRect * 0x0012f368, unsigned int 3) 
line 505
nsContainerFrame::FinishReflowChild(nsIFrame * 0x038b5124, nsIPresContext * 
0x037d4108, nsHTMLReflowMetrics & {...}, int 0, int 0, unsigned int 3) line 850 
+ 28 bytes
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 
0x037d4108, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0, int 0, int 0, int 12105, int 3285, int 1) line 961 + 33 bytes
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x038bb4dc, 
nsBoxLayoutState & {...}) line 523 + 52 bytes
nsBox::Layout(nsBox * const 0x038bb4dc, nsBoxLayoutState & {...}) line 985
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x038b54e4, nsBoxLayoutState 
& {...}) line 379
nsBox::Layout(nsBox * const 0x038b54e4, nsBoxLayoutState & {...}) line 985
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x038b54e4, 
const nsRect & {...}) line 591 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x038b54e4, 
const nsRect & {...}) line 1038 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1145
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x038b5274, nsBoxLayoutState 
& {...}) line 1046 + 15 bytes
nsBox::Layout(nsBox * const 0x038b5274, nsBoxLayoutState & {...}) line 985
nsBoxFrame::Reflow(nsBoxFrame * const 0x038b523c, nsIPresContext * 0x037d4108, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 781
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x038b523c, nsIPresContext * 
0x037d4108, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 735 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x038b523c, nsIPresContext * 
0x037d4108, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 726 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x038b50e8, nsIPresContext * 
0x037d4108, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 538
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x04c5fc30, 
nsIPresContext * 0x037d4108, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 145
PresShell::ProcessReflowCommand(nsVoidArray & {...}, int 1, nsHTMLReflowMetrics 
& {...}, nsSize & {...}, nsIRenderingContext & {...}) line 5830
PresShell::ProcessReflowCommands(int 1) line 5885
ReflowEvent::HandleEvent() line 5743
HandlePLEvent(ReflowEvent * 0x04c5fd00) line 5757
PL_HandleEvent(PLEvent * 0x04c5fd00) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00c62500) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x001319d6, unsigned int 49387, unsigned int 0, 
long 12985600) line 1071 + 9 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x00d54790) line 422
main1(int 1, char * * 0x003a8b80, nsISupports * 0x00000000) line 1174 + 32 bytes
main(int 1, char * * 0x003a8b80) line 1478 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e97d08()
Bug 73285 might be related
This looks like a dupe of the thought to be fixed bug 19256.
Depends on: 73285
I did the change mentioned in bug 73285 and now I don't get the extreme painting 
I got before. The load time dropped from ~5 to ~4 seconds (20%) and Quantify 
says painting is about 5% instead of 40% of the page load time. 

So what has to be done to enable this code "for real"? Is it something with the 
table code not handling a resize correctly?

Are you sure that's bug 73285 and not bug 73825?
You are right. Bug 73825 it is.
Depends on: 73825
No longer depends on: 73285
I wonder if this is due to my change to nsContainerFrame.cpp on July 5th - I had
to change the logic in the view resize so the entire view would be invalidated
if the height of the frame changed too (fixed bug 868532). As I noted in that
bug, I can optimize the condition a little bit by only checking if the height
decreased, however I could not detect any difference in performance so I didn't
do it. It might help this case though.

Can somebody check if this problem is fixed by backing out my change
(nsContainerFrame.cpp r1.111 was my checkin)?
Yes, that is it. 

During the page load (when the table grows I guess) the frameSize.height will 
differ from height, everything else being the same. I noticed that when running 
in the debugger but I assumed that the code had always looked like that. I 
didn't see that previously only the width was checked.

(The bug fixed was bug 86852)
It sounds then like I need to use the version of the change that only does the
full invalidate when the height of the frame decreases instead of just changes.
It makes the code look funkier, but I don't want to slow down table building now
do I? I'll give you a patch in a second...
Daniel, any chance you can test the patch I just attached? It still fixes bug
86852 (which was about the frame height shrinking and not invalidating the top
part) but should also fix this performance regression.
Do you want to make that >=  ?

With your path we repaint when the size is the same which I guess is not what we
want. :-) 

(I could do with a comment and two too)
Marc, I tested with frameSize.width == width && frameSize.height >= height (note 
the >=) and there is no excessive painting so that fixes this bug.
Assignee: karnaze → attinasi
Thanks Daniel, expecially for the >= catch ;)

I'll update the comment and get this in. I really appreciate your testing it for me.
Status: NEW → ASSIGNED
Marking as depends-on bug 86852, where the cause and rememdy can be found.
Depends on: 86852
No longer depends on: 73825
Keywords: regression
The fix for this was applied to the trunk. If the patch goes to the branch, it
will be the correct one (previous patch was never on the trunk). Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Marking verified per last comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: