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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Chris Waterson]

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.

Comment 1

[Johnny Stenback (:jst)]

dbaron, would fixing this significantly help our badly hurting DHTML performace?

Comment 2

[Daniel Bratell]

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.

Comment 3

[Daniel Bratell]

I meant bug 97287. DHTML menus.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, the code mentioned is in nsBlockFrame::Reflow.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The bug troy seems to have been fixing was bug 20687.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... which was probably fixed in another / a better way by later patches.

Comment 8

[Daniel Bratell]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Johnny Stenback (:jst)]

How far away from a patch are we here? Should we pull this into mozilla0.9.9, or
mozilla1.0?

Comment 11

[Daniel Bratell]

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

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

jesup, shaver, you may be interested in this one.

Comment 15

[Markus Hübner]

Any news on that - in view of bug 129115 ?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will not be fixed in bug 129115.

Comment 17

[Markus Hübner]

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

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Kai Lahmann (is there, where MNG is)]

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

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This bug is about a specific code-level problem, so detailed discussion of
performance testcases may not be relevant.

Comment 21

[Corrado Berti]

*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

[José Jeria]

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

PIII, 500 Mhz, Windows 2000...

Comment 23

[Markus Hübner]

Same numbers as José here (trunk 2002061908 on win-xp,1.1ghz,512ram)

Comment 24

[Markus Hübner]

This one blocks a large number of bugs. 
Can a fix be achieved till 1.2 ?

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

similar results as in comment 22 .

Comment 26

[Jaime Rodriguez, Jr.]

Nominating for nsbeta1, as we should improve our perf on dhtml.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Waterson is otherwise engaged.  Anyone interested in attacking this one?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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].

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking.

Comment 33

[José Jeria]

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

[José Jeria]

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 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good to me. I'd give this an r

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: above patch, but with a few comments added

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also filed bug 162191 for fixing this bug for nsPositionedInlineFrame.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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?)

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 94846 [details] [diff] [review]
above, but don't compute overflow until done

r/sr=roc+moz

I'm sold

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Christopher Cook]

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

[kinmoz]

The issue Christopher Cook mentioned above is bug 157681.

Comment 45

[Christopher Cook]

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

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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);

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch incorporating dbaron's additional fix

I tweaked dbaron's fix a bit for speed.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 51

[Jaime Rodriguez, Jr.]

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

[kinmoz]

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

sr=[e-mail]

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fix checked in.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

marking fixed

Comment 55

[kill this account]

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

Comment 56

[Jaime Rodriguez, Jr.]

petersen: can you pls verify this as fixed on the trunk? thanks!

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Christopher Cook]

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

[Kevin McCluskey (gone)]

"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

[kill this account]

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

[Kevin McCluskey (gone)]

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

[Christopher Cook]

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

[Christopher Cook]

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

Comment 64

[kill this account]

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

Comment 65

[Henrik Gemal]

this bug is causing bug 170688

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Worth noting here, but that doesn't make it any less fixed.

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This also seems to have caused bug 172031.

Comment 68

[Asa Dotzler [:asa]]

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

a=asa for checkin to 1.2 (on behalf of drivers)

Comment 69

[Asa Dotzler [:asa]]

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

oops, ignore me. wrong bug.

Comment 70

[Susie Wyshak]

So can something be done to fix bug 172031 or should this one be reopened?

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We won't fix 172031 for 1.2 unfortunately. We'll fix it in 1.3.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect this caused bug 183756.

Comment 73

[Chris Petersen]

Closing this one out.