Closed
Bug 90503
Opened 23 years ago
Closed 23 years ago
Table repainted every reflow
Categories
(Core :: Layout, defect)
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.
Reporter | ||
Comment 1•23 years ago
|
||
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. :-)
Reporter | ||
Comment 2•23 years ago
|
||
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?
Reporter | ||
Comment 3•23 years ago
|
||
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
Reporter | ||
Comment 4•23 years ago
|
||
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...
Reporter | ||
Comment 5•23 years ago
|
||
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
Reporter | ||
Comment 6•23 years ago
|
||
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()
Reporter | ||
Comment 8•23 years ago
|
||
This looks like a dupe of the thought to be fixed bug 19256.
Depends on: 73285
Reporter | ||
Comment 9•23 years ago
|
||
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?
Comment 10•23 years ago
|
||
Are you sure that's bug 73285 and not bug 73825?
Reporter | ||
Comment 11•23 years ago
|
||
You are right. Bug 73825 it is.
Assignee | ||
Comment 12•23 years ago
|
||
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)?
Reporter | ||
Comment 13•23 years ago
|
||
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)
Assignee | ||
Comment 14•23 years ago
|
||
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...
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Comment 16•23 years ago
|
||
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.
Reporter | ||
Comment 17•23 years ago
|
||
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)
Reporter | ||
Comment 18•23 years ago
|
||
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
Assignee | ||
Comment 19•23 years ago
|
||
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
Assignee | ||
Comment 20•23 years ago
|
||
Marking as depends-on bug 86852, where the cause and rememdy can be found.
Keywords: regression
Assignee | ||
Comment 21•23 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•