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

VERIFIED FIXED in Future

Status

()

P1
normal
VERIFIED FIXED
18 years ago
5 years ago

People

(Reporter: waterson, Assigned: roc)

Tracking

(Blocks: 1 bug, {testcase, topembed+, topperf})

Trunk
Future
testcase, topembed+, topperf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [patch])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

18 years ago
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

18 years ago
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Target Milestone: --- → Future
(Reporter)

Updated

17 years ago
Blocks: 71668

Updated

17 years ago
Blocks: 21762
dbaron, would fixing this significantly help our badly hurting DHTML performace?

Updated

17 years ago
Blocks: 114584

Comment 2

17 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

Comment 3

17 years ago
I meant bug 97287. DHTML menus.

Updated

17 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

17 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.
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?

Comment 11

17 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?

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

17 years ago
Keywords: mozilla1.0+
jesup, shaver, you may be interested in this one.

Comment 15

17 years ago
Any news on that - in view of bug 129115 ?
This will not be fixed in bug 129115.

Updated

17 years ago
Blocks: 70156

Updated

17 years ago
Blocks: 139986

Comment 17

17 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
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.

Updated

17 years ago
Blocks: 107091

Comment 21

17 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

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

PIII, 500 Mhz, Windows 2000...

Comment 23

17 years ago
Same numbers as José here (trunk 2002061908 on win-xp,1.1ghz,512ram)
Keywords: perf

Updated

17 years ago
Blocks: 104593

Updated

17 years ago
Blocks: 133105

Updated

17 years ago
Blocks: 107746

Comment 24

17 years ago
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 .

Comment 26

17 years ago
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+
Created attachment 93801 [details] [diff] [review]
some work in progress from bug 154230

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.
Created attachment 93811 [details] [diff] [review]
completely different approach

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

16 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

16 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 ????!
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).

Updated

16 years ago
Keywords: topembed → topembed+
Created attachment 94845 [details] [diff] [review]
above patch, but with a few comments added
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.
Created attachment 94846 [details] [diff] [review]
above, but don't compute overflow until done

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...

Comment 43

16 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!

Comment 44

16 years ago
The issue Christopher Cook mentioned above is bug 157681.
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → Future

Comment 45

16 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..
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
Created attachment 100173 [details] [diff] [review]
Fixed patch

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+
Created attachment 100190 [details] [diff] [review]
patch incorporating dbaron's additional fix

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+

Comment 51

16 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?
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2][patch] → [adt2] [patch] [Needs sr=]

Comment 52

16 years ago
Comment on attachment 100190 [details] [diff] [review]
patch incorporating dbaron's additional fix

sr=kin@netscape.com
Attachment #100190 - Flags: superreview+

Updated

16 years ago
Keywords: approval
Whiteboard: [adt2] [patch] [Needs sr=] → [adt2] [patch]
marking fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 55

16 years ago
thanks very much for working on this; it really reduces the CPU usage for marquee!

Comment 56

16 years ago
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.)

Comment 58

16 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 :)
"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

16 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.
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

16 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 63

16 years ago
Sorry, I meant bug 170702 (but I know how much everyone enjoys spam..)

Comment 64

16 years ago
I verified that the marquee CPU load improvement (less) did happend on the day 
bug 164931 got checked in.

Comment 65

16 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
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
This also seems to have caused bug 172031.

Comment 68

16 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

16 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

16 years ago
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.

Updated

16 years ago
Blocks: 178882

Comment 73

16 years ago
Closing this one out.
Status: RESOLVED → VERIFIED

Updated

14 years ago
No longer blocks: 21762
Blocks: 21762
You need to log in before you can comment on or make changes to this bug.