Closed Bug 75121 Opened 23 years ago Closed 22 years ago

[DHTML] inefficient incremental reflow targeted at absolutely positioned block frames

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: waterson, Assigned: roc)

References

Details

(Keywords: testcase, topembed+, topperf, Whiteboard: [adt2] [patch])

Attachments

(1 file, 5 obsolete files)

patch incorporating dbaron's additional fix
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.
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Target Milestone: --- → Future
Blocks: 71668
Blocks: 21762
dbaron, would fixing this significantly help our badly hurting DHTML performace?
Blocks: 114584
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
I meant bug 97287. DHTML menus.
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.
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.

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.
How far away from a patch are we here? Should we pull this into mozilla0.9.9, or
mozilla1.0?
> 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?


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.
Keywords: mozilla1.0+
jesup, shaver, you may be interested in this one.
Any news on that - in view of bug 129115 ?
Blocks: 70156
Blocks: 139986
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
URL: http://www.world-direct.com/mozilla/d...
Keywords: testcase
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
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.
Blocks: 107091
*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





my test (avg of 3 launch)
trunk 20020620 | msie6 
-----------------------
      3174     | 1605

PIII, 500 Mhz, Windows 2000...
Same numbers as José here (trunk 2002061908 on win-xp,1.1ghz,512ram)
Keywords: perf
Blocks: 104593
Blocks: 133105
Blocks: 107746
This one blocks a large number of bugs. 
Can a fix be achieved till 1.2 ?
Keywords: mozilla1.2
similar results as in comment 22 .
Nominating for nsbeta1, as we should improve our perf on dhtml.
Keywords: nsbeta1, topembed
Whiteboard: [adt2]
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.
Attached patch completely different approach (obsolete) — — Splinter Review 
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.
URL: http://www.world-direct.com/mozilla/d...
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]
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


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 ????!
Looks good to me. I'd give this an r
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).
Keywords: topembed → topembed+
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
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...
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!
The issue Christopher Cook mentioned above is bug 157681.
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → Future
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..
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
Attached patch Fixed patch (obsolete) — — Splinter Review 
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+
I tweaked dbaron's fix a bit for speed.
Attachment #100173 - Attachment is obsolete: true
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+
This one looks like a good one to fix because it will help Mozilla DHTML perf. 

Can we retarget it for Mozilla1.2b?
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2][patch] → [adt2] [patch] [Needs sr=]
Comment on attachment 100190 [details] [diff] [review]
patch incorporating dbaron's additional fix

sr=kin@netscape.com
Attachment #100190 - Flags: superreview+
Keywords: approval
Whiteboard: [adt2] [patch] [Needs sr=] → [adt2] [patch]
marking fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
thanks very much for working on this; it really reduces the CPU usage for marquee!
petersen: can you pls verify this as fixed on the trunk? thanks!
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.)
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 :)
"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. 
dbaron: you are right, the improvement for marquee occurred sometime be 9-14 and
9-23 so it is not related to this.
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.  
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 :)
Sorry, I meant bug 170702 (but I know how much everyone enjoys spam..)
I verified that the marquee CPU load improvement (less) did happend on the day 
bug 164931 got checked in.
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
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 on attachment 100190 [details] [diff] [review]
patch incorporating dbaron's additional fix

oops, ignore me. wrong bug.
Attachment #100190 - Flags: approval+
So can something be done to fix bug 172031 or should this one be reopened?
We won't fix 172031 for 1.2 unfortunately. We'll fix it in 1.3.
Blocks: 178882
Closing this one out.
Status: RESOLVED → VERIFIED
No longer blocks: 21762
Blocks: 21762
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: