Closed
Bug 75121
Opened 24 years ago
Closed 22 years ago
[DHTML] inefficient incremental reflow targeted at absolutely positioned block frames
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
VERIFIED
FIXED
Future
People
(Reporter: waterson, Assigned: roc)
References
Details
(Keywords: testcase, topembed+, topperf, Whiteboard: [adt2] [patch])
Attachments
(1 file, 5 obsolete files)
15.00 KB,
patch
|
roc
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
Incremental reflows targeted at an absolutely positioned block frame does an extra resize reflow for the sole purpose of computing the overflow area. I couldn't find any other bugs filed on this, so I figured I'd note it somewhere as something that ought be fixed.
Reporter | ||
Updated•24 years ago
|
Comment 1•23 years ago
|
||
dbaron, would fixing this significantly help our badly hurting DHTML performace?
Comment 2•23 years ago
|
||
I think this is the main reason for the bad performance in the DHTLM analyzed in bug 114584. Assuming that's typical DHTML, fixing this bug would help DHTML performace quite a bit.
Blocks: 97287
Updated•23 years ago
|
Keywords: mozilla0.9.9,
topperf
Summary: inefficient incremental reflow targeted at absolutely positioned block frames → [DHTML] inefficient incremental reflow targeted at absolutely positioned block frames
FWIW, the code mentioned is in nsBlockFrame::Reflow.
The code in question was moved from nsAreaFrame to nsBlockFrame and dates back to: mozilla/layout/html/base/src/nsAreaFrame.cpp revision 1.69 date: 1999/12/06 15:49:40; author: troy%netscape.com; state: Exp; lines: +52 -62 Change to how overflow is handled for absolutely positioned elements. We no longer use nsIAraeFrame and now it's folded into the overflow area in the reflow metrics
The bug troy seems to have been fixing was bug 20687.
... which was probably fixed in another / a better way by later patches.
Comment 8•23 years ago
|
||
Does that mean that the code in question can safely be removed? I'm not sure exactly what code you're refering to so I don't know what code to ifdef out for testing. How about nsAbsoluteContainingBlock::Reflow, is that involved in any way? (Becasue it's there DHTML spends all the time) It's called by nsBlockFrame::Reflow and it calls nsBlockFrame::Reflow through nsAbsoluteContainingBlock::ReflowAbsoluteFrame.
Assignee | ||
Comment 9•23 years ago
|
||
Unfortunately we DO need the overflow area to figure out the bounds of the views on all the containers containing the reflowing frame in SyncFrameViewAfterReflow; even if we just moved the frame, we still need to know its overflow area in case we have to resize its containers' views because of the move. But we only need it if NS_FRAME_OUTSIDE_CHILDREN is set. When NS_FRAME_OUTSIDE_CHILDREN is not set and nothing's changed in the children, it can remain unset and we know there is no overflow.
Comment 10•23 years ago
|
||
How far away from a patch are we here? Should we pull this into mozilla0.9.9, or mozilla1.0?
Comment 11•23 years ago
|
||
> But we only need it if NS_FRAME_OUTSIDE_CHILDREN is set. When
> NS_FRAME_OUTSIDE_CHILDREN is not set and nothing's changed in the children, it
> can remain unset and we know there is no overflow.
Do you mean that there are something we should change here?
Assignee | ||
Comment 12•23 years ago
|
||
dbaron, you're talking about this line right? http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#673 I am not super-savvy with block layout so dbaron and others would have to confirm, BUT I believe that we can bypass the resize reflow if NS_FRAME_OUTSIDE_CHILDREN is not set --- because if the flowed content overflowed, then NS_FRAME_OUTSIDE_CHILDREN would have been set -- and just set aMetrics.mOverflowArea to (0, 0, aMetrics.width, aMetrics.height).
Beyond that, in the case where there is overflow, we could just descend into the frames (only descending into the children of those that have NS_FRAME_OUTSIDE_CHILDREN). And we could even do that in nsAbsoluteContainingBlock::IncrementalReflow -- i.e., by fixing the code that's supposed to be doing a working incremental reflow.
Updated•23 years ago
|
Keywords: mozilla1.0+
Comment 17•23 years ago
|
||
Providing a testcase for this (complete test-series at http://www.umsu.de/jsperf ) test-workstation has 1.1ghz, 512ram, 32mb geforce2 go trunk 2002052008 | msie6 | opera6.0 | ns4.78 ----------------------------------------------- 4957 | 1502 | 1502 | 3715
Keywords: testcase
Comment 18•23 years ago
|
||
I have these results with matic's testcase: (all Linux) Moz as of May 17: 5090 Opera 6.0 Beta 1 for Linux: 2089 Netscape 4.79: 1676
Comment 19•23 years ago
|
||
more results: Mozilla 1.0RC2/debian, Celeron 422, Linux, old ATI driver: Opera 6.0 -> 2462 Mozilla -> 3646 konqueror 2.2.2 -> 3969 Mozilla 2002051907, Athlon 1000, Linux, nVidia 3D driver: Opera 6.0 -> 1823 konqueror 2.2.2 -> 2188 Netscape 4.77 -> 2330 Mozilla -> 3080 (getting faster with each test) btw. at 3000 it starts to look fast, every slower value lookes bad to me
This bug is about a specific code-level problem, so detailed discussion of performance testcases may not be relevant.
Comment 21•23 years ago
|
||
*worksforme* AMD 1.2Ghz 512Mb RAM Kyro II 64Mb W2k Server my test (avg of 5 launch) trunk 20020619 | msie6 ----------------------- 1578 | 1502 only 5% slower than IE
Comment 22•23 years ago
|
||
my test (avg of 3 launch) trunk 20020620 | msie6 ----------------------- 3174 | 1605 PIII, 500 Mhz, Windows 2000...
Comment 23•23 years ago
|
||
Same numbers as José here (trunk 2002061908 on win-xp,1.1ghz,512ram)
Keywords: perf
Comment 24•22 years ago
|
||
This one blocks a large number of bugs. Can a fix be achieved till 1.2 ?
Keywords: mozilla1.2
Comment 27•22 years ago
|
||
Waterson is otherwise engaged. Anyone interested in attacking this one?
Keywords: mozilla0.9.9,
mozilla1.0+
This is some work-in-progress for bug 154230 that I did before I found the code in nsAbsoluteContainingBlock.cpp that I used to fix bug 154230. This might be relevant to this bug, although I don't really like the design and I haven't even considered it in the context of the existing code that I used to fix bug 154230.
This should be a complete patch, although I haven't tested it beyond compiling it. This is a much simpler approach, although I liked the refactoring of |UpdateOutsideChildrenBit| in attachment 93801 [details] [diff] [review].
This patch doesn't help http://www.world-direct.com/mozilla/dhtml/75121/anim-test.htm , but it does (I think) fix the problem described in comment 0, so I'm removing the URL. Should another bug be filed on that URL? I suspect there are other testcases that this would help a good bit more.
Comment 3 did point out a correct testcase, though, http://www.world-direct.com/hm/nav.html . On that testcase, I'm seeing: without patch: 3303, 3250, 3265, 3273 with patch: 2291, 2226, 2229, 2227
Taking.
Assignee: waterson → dbaron
Status: ASSIGNED → NEW
Priority: P3 → P2
Target Milestone: Future → mozilla1.2alpha
Status: NEW → ASSIGNED
Whiteboard: [adt2] → [adt2][patch]
Comment 33•22 years ago
|
||
Weird, getting these results with Pentium III, 800 Mhz, Windows 98 SE. See comment 22 to compare with older build and Windows 2000. my test (avg of 3 launch) trunk 20020730 | msie6 ----------------------- 1870 | 2980
Comment 34•22 years ago
|
||
Forgot to mention the URL: http://www.world-direct.com/mozilla/dhtml/75121/anim-test.htm and forget my last comment. Now I retested and I got same nubmers as IE... One thing worth mentioning though, ALT + TAB to IE, ran the test. ALT + TAB back to Mozilla and I again get good numbers as in comment 33 ????!
Comment on attachment 93811 [details] [diff] [review] completely different approach This also needs the change: @@ -5627,7 +5601,7 @@ if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) { PRInt32 depth = GetDepth(); nsRect ca; - ComputeCombinedArea(mLines, mRect.width, mRect.height, ca); + ::ComputeCombinedArea(mLines, mRect.width, mRect.height, ca); nsFrame::IndentBy(stdout, depth); ListTag(stdout); printf(": bounds=%d,%d,%d,%d dirty=%d,%d,%d,%d ca=%d,%d,%d,%d\n", since there's also a static function in global scope with the same name (for DEBUG only).
Updated•22 years ago
|
Attachment #93811 -
Attachment is obsolete: true
Actually, I just did a read through the code that is now going to be used but that hasn't gotten testing in the past, and I did notice one problem, which I'll attach a new patch to fix: the |CalculateChildBounds| was being done every time through the loop in nsAbsoluteContainingBlock::IncrementalReflow, so if multiple absolutely positioned children had incremental reflows within them, and one of the later ones had its overflow area shrink, the old overflow area would still be counted. To fix this, I decided to move the CalculateChildBounds completely out of the IncrementalReflow function, so that it's always called by the caller (since right now, nsPositionedInlineFrame doesn't use it, so those changes are |#if 0|).
I also filed bug 162191 for fixing this bug for nsPositionedInlineFrame.
This fixes the problem mentioned in my previous comment that the overflow area should not be computed until after all reflows have been done. (Hmm, I'm wondering... what happens if we do an incremental reflow of something with 'top: auto' or 'left: auto', and the inline moves, but the containing block doesn't change size? Do we have a bug on that?)
Attachment #94845 -
Attachment is obsolete: true
Assignee | ||
Comment 41•22 years ago
|
||
Comment on attachment 94846 [details] [diff] [review] above, but don't compute overflow until done r/sr=roc+moz I'm sold
Attachment #94846 -
Flags: review+
roc suggested by email that I test http://www.dhtmlcentral.com/, and, sure enough, this patch breaks the front page (some of the sections don't display until I resize the page). So I'm going to investigate what the problem is...
Comment 43•22 years ago
|
||
Having had a good look around the current list of DHTML perf bugs it would appear that a significant amount of perf bugs are related to cases where a div is moved but no actual resize or change of content takes place. While I can't give any technical insight I would assume that this (type of) fix would make a substantial dent in rendering speed of these types of bugs in that skipping Reflow operations when none are required sounds like a very good idea. Keep up the good work!
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → Future
Comment 45•22 years ago
|
||
dbaron - your last comment mentioned that dhtmlcentral.com needed to be resized in order to display correctly - does this suggest that this page required an incremental reflow for these divs (in order to invalidate the div's area on-screen and cause a paint), and if so is the NS_FRAME_OUTSIDE_CHILDREN flag required to be set for the initial paint of these div's to take place? Is there a test build anywhere which could be used to test other sites or gather other cases to try and pinpoint the cause of the problem? I've noticed a few people citing test cases which use setTimeout() to provide animation across divs, I would guess that these are fairly pointless as a benchmark, since we're actually *asking* the browser to wait, thus a fast enough machine will return better results (in comparison to IE on the same machine) depending on whether it can keep up with the timer thread ticks. An interesting side-note is that moz appears to be faster at performing: var myvar=document.getElementByID("").style.top than IE is (about 25% faster, 100 iterations) but changes to style.top (style.top=1, 100 iterations, no other changes to div) is about 50% slower than IE. style.top=[random value from array] is about 3 times slower than IE (Moz's ~6100ms to IE's ~1850ms), with div style set to position:relative instead of absolute the gap closes again (~1300ms on IE, ~2000ms on Moz). Just some random observations to try and help this along..
Assignee | ||
Comment 46•22 years ago
|
||
dbaron gave me permission to take over this bug. I've been investigating dhtmlcentral.com. The problem there is that script sets width/height styles on the positioned DIVs, but some of these settings never seem to take effect. It appears that some StyleChange reflows are being lost. I'll keep investigating...
Assignee: dbaron → roc+moz
Status: ASSIGNED → NEW
Assignee | ||
Comment 47•22 years ago
|
||
OK, I found the problem with the previous patch. If there was a reflow command targeted at the block (e.g., style change reflow) AND the block had absolutely positioned children eligible for incremental reflow, then we erroneously signalled in nsAbsoluteContainingBlock::IncrementalReflow that we had handled the reflow by updating the children. In fact in that case we need to signal that we CANNOT handle the reflow, and in fact we don't want to incrementally reflow the children there because they'll be reflowed again when nsBlockFrame performs the style change reflow. dhtmlcentral.com is fixed with this patch.
Attachment #93801 -
Attachment is obsolete: true
Attachment #94846 -
Attachment is obsolete: true
Comment on attachment 100173 [details] [diff] [review] Fixed patch r=dbaron on roc's changes (I did a diff of the diffs), and carring over roc's review of mine to make it "has review". While poking around on the problem that you just fixed, I did notice one other potential bug that you might also want to include: Index: nsPresShell.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v retrieving revision 3.560 diff -u -d -r3.560 nsPresShell.cpp --- nsPresShell.cpp 7 Aug 2002 04:06:57 -0000 3.560 +++ nsPresShell.cpp 13 Aug 2002 00:02:11 -0000 @@ -3667,8 +3667,12 @@ nsReflowType queuedRCType; aReflowCommand->GetType(RCType); rc->GetType(queuedRCType); + nsCOMPtr<nsIAtom> CLName, queuedCLName; + aReflowCommand->GetChildListName(*getter_AddRefs(CLName)); + rc->GetChildListName(*getter_AddRefs(queuedCLName)); if (targetFrame == targetOfQueuedRC && - RCType == queuedRCType) { + RCType == queuedRCType && + CLName == queuedCLName) { #ifdef DEBUG if (VERIFY_REFLOW_NOISY_RC & gVerifyReflowFlags) { printf("*** PresShell::AlreadyInQueue(): Discarding reflow command: this=%p\n", (void*)this);
Attachment #100173 -
Flags: review+
Assignee | ||
Comment 49•22 years ago
|
||
I tweaked dbaron's fix a bit for speed.
Attachment #100173 -
Attachment is obsolete: true
Assignee | ||
Comment 50•22 years ago
|
||
Comment on attachment 100190 [details] [diff] [review] patch incorporating dbaron's additional fix r=roc on dbaron's nsPresShell change, bringing forward r on the rest
Attachment #100190 -
Flags: review+
Comment 51•22 years ago
|
||
This one looks like a good one to fix because it will help Mozilla DHTML perf. Can we retarget it for Mozilla1.2b?
Comment 52•22 years ago
|
||
Comment on attachment 100190 [details] [diff] [review] patch incorporating dbaron's additional fix sr=kin@netscape.com
Attachment #100190 -
Flags: superreview+
Assignee | ||
Comment 54•22 years ago
|
||
marking fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 55•22 years ago
|
||
thanks very much for working on this; it really reduces the CPU usage for marquee!
Hmmm. This shouldn't affect marquee, where the only things that should be reflowing are the (hidden) scrollbars. (And yes, I have a bug on the fact that we do even that.)
Comment 58•22 years ago
|
||
This had made a big difference to the raw performance shifting a large number of div's around, however it's let down by a (perceived, by me anyway) lack of regular paints - I've used a script which takes roughly as long as IE (maybe only slightly slower) to shift 100 div's randomly around the screen, it's just we paint less often (or maybe we take longer to do the paints that we actually do). Painting less often has the benefit that animating one div at 10ms intervals (and repeating 100 times) works well in Mozilla, whereas in IE, it tries to render each change to the div and can take 5 seconds rather than 1 - Just an aside, either way the performance of manipulating many absolutely positioned div's has shot up, even if the actual rendering still appears slow. Now we just need more actual paints :)
Comment 59•22 years ago
|
||
"Painting less often has the benefit that animating one div at 10ms intervals (and repeating 100 times) works well in Mozilla, whereas in IE, it tries to render each change to the div and can take 5 seconds rather than 1 " Now that the fix for bug 164931 has landed Mozilla should behave the same as IE in regards to paint behavior. Mozilla will not fire the timer to animate the next frame until all painting for the previous frame has completed.
Comment 60•22 years ago
|
||
dbaron: you are right, the improvement for marquee occurred sometime be 9-14 and 9-23 so it is not related to this.
Comment 61•22 years ago
|
||
CPU usage for marquee should have been improved by the fix for bug 164931. I have seen pages with Marquee drop from 20% cpu down 2-5% in some instances as a result of the patch for bug 164931. This is because we now use timers which fire with maximum update rate of 100fps. Previously, when we used posted WM_APP events our frame rate was unbounded, so the faster your CPU the faster our potential frame rate. This kept the CPU usage high on fast machines. Now that we use timers the CPU usage will drop on faster machines. The posted WM_APP's also starved paints, So although the frame rate was higher using posted WM_APP events the additional frames were not always painted so it was the worst of both worlds. High CPU usage and no improvement in animation smoothness.
Comment 62•22 years ago
|
||
kevin, "Now that the fix for bug 164931 has landed Mozilla should behave the same as IE in regards to paint behavior. Mozilla will not fire the timer to animate the next frame until all painting for the previous frame has completed. " Cool, but either way IE takes longer (on my machine) to render the example attached to bug 17702 so I don't know whats going on there, adjusting the timer to 10ms in that example certainly makes it look as if we're painting when we get time rather than rendering every paint (possibly more eveident or slower machines?). Obviously I'm just going on what I can see, ultimately you guys know how it all works so there's no point me arguing about it :)
Comment 64•22 years ago
|
||
I verified that the marquee CPU load improvement (less) did happend on the day bug 164931 got checked in.
Comment 65•22 years ago
|
||
this bug is causing bug 170688
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Worth noting here, but that doesn't make it any less fixed.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
This also seems to have caused bug 172031.
Comment 68•22 years ago
|
||
Comment on attachment 100190 [details] [diff] [review] patch incorporating dbaron's additional fix a=asa for checkin to 1.2 (on behalf of drivers)
Attachment #100190 -
Flags: approval+
Comment 69•22 years ago
|
||
Comment on attachment 100190 [details] [diff] [review] patch incorporating dbaron's additional fix oops, ignore me. wrong bug.
Attachment #100190 -
Flags: approval+
Comment 70•22 years ago
|
||
So can something be done to fix bug 172031 or should this one be reopened?
I suspect this caused bug 183756.
You need to log in
before you can comment on or make changes to this bug.
Description
•