320378 - [Firefox 1.5 and 2.x] Rendering of very simple pages takes over 20 seconds

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P1)

Description

[Stefan van den Oord]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/416.12 (KHTML, like Gecko) Safari/416.13
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

I attached a page showing the problem. Rendering the page, both on Mac OS X and on Windows, takes well over 20 seconds. On Firefox 1.0.7 the page is rendered instantaneous.

I have isolated the issue: it is between the lines "START COMMENT OUT HERE" and "FINISH COMMENT OUT HERE" in the style sheet. Removing that part makes the page render instantaneous. (The layout is not as intended in that case, obviously.)

Reproducible: Always

Steps to Reproduce:
1. Extract the attached ZIP file
2. Load the page "example.html" in Firefox 1.5

Actual Results:  
Rendering the page takes over 20 seconds, both on Windows and on Mac OS X

Expected Results:  
The page should render instantaneously, as it does in Firefox 1.0.7.

I found a bug report that is probably not related to this, but I'm not sure, so I give you the bug ID: #319644.

The reason that I set the severity to "major" is that this issue makes our CMS back-end unusable for Firefox 1.5 users.

Comment 1

[Stefan van den Oord]

Attachment: Example page demonstrating the issue

Comment 2

[Steve England [:stevee]]

Attachment: testcase from reporter's attached .zip

Comment 3

[Steve England [:stevee]]

Attachment: testcase from reporter's attached .zip

Comment 4

[Steve England [:stevee]]

Dupe of one of the many 'firefox slow tables' bugs?

Comment 5

[Stefan van den Oord]

I don't think so, because this is a non-table css only layout.

Comment 6

[Ria Klaassen (not reading all bugmail)]

This changed between 24 and 25 Nov 2004.
Checkins:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-11-24+06%3A00%3A00&maxdate=2004-11-25+09%3A00%3A00&cvsroot=%2Fcvsroot

Comment 7

[Peter van der Woude [:Peter6]]

nested divs and tables don't behave much different (performance wise)
Martijn, can you figure this one out ?

Comment 8

[Martijn Wargers (dead)]

Attachment: partly minimised testcase

Seems to be happening because of the floats and the use of lists.

Comment 9

[Martijn Wargers (dead)]

With that regression range, it is very likely a regression from bug 209694.

Comment 10

[Martijn Wargers (dead)]

Attachment: minimised testcase

This renders slow for me (lags for 5 sec or so), while it is still pretty fast in Mozilla1.7.

Comment 11

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051217 Firefox/1.6a1 ID:2005121705

It loads normally, but it maximizes slowly :)

Comment 12

[Peter van der Woude [:Peter6]]

(In reply to comment #11)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051217
> Firefox/1.6a1 ID:2005121705
> 
> It loads normally, but it maximizes slowly :)
> 

You need a slow computer like Martijn (600MHz) and I(850MHz) have Ria ;-)
It takes ~5 secs to display and a repaint after window resize is ~10 secs.

Comment 13

[Stefan van den Oord]

I don't know if it's relevant, but please note that in my original example I could not get it to work by using other constructs than float, such as position absolute (from the right of the screen), position relative, etc. (All my attempts tried to retain the intended layout. Just displaying the 'other' div and the divs inside that as 'inline' makes it render instantaneous again, but destroys the layout.)

Comment 14

[Markus Hübner]

Using the latest trunk build is this still valid?

Comment 15

[Stefan van den Oord]

If you can direct me to a Mac OS X build of that version, I will be happy to test it...

(In reply to comment #14)
> Using the latest trunk build is this still valid?

Comment 16

[Mikko Rantalainen]

(In reply to comment #14)
> Using the latest trunk build is this still valid?

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060531 Minefield/3.0a1 ID:2006053104 [cairo]

Going slower than ever with https://bugzilla.mozilla.org/attachment.cgi?id=205960
Rendering the test case takes 100% of CPU for 50 seconds running on Athlon XP 2800+! On the plus side, the test case displays incrementally. Even closing the tab that has the test case open takes about 10 seconds! Just tested the very same test case with Opera/9.00 (X11; Linux i686; U; en) and it has no problems with it.

Comment 17

[Stefan van den Oord]

I just wanted to point out that this bug has not been solved in Firefox 2.0 beta 1. Is the version field (currently '1.8 Branch') correct? Shouldn't this be 'trunk'?

Comment 18

[Boris Zbarsky [:bzbarsky]]

This is almost certainly a regression from bug 209694.  For what it's worth, a trunk build loads the "minimised testcase" in 17 seconds, while a reflow branch build does so in 6.  Which is still way too much...

Comment 19

[Boris Zbarsky [:bzbarsky]]

So for the minimized testcase, I tried logging the times through the |while (PR_TRUE)| loop in nsBlockFrame::ReflowBlock.

We hit the beginning of that loop 44463 for 186 distinct blocks; an average of 250 times a block.

84 blocks got reflown 512 times.
3 blocks got reflown 256 times.
2 blocks got reflown 128 times.
3 blocks got reflown 64 times.
2 blocks got reflown 32 times.
3 blocks got reflown 16 times.
3 blocks got reflown 8 times.
3 blocks got reflown 4 times.
2 blocks got reflown 3 times.
4 blocks got reflown 2 times.
77 blocks got reflown 1 time.

Sadly, the "reflow counts" option in the layout debugger seems broken.  :(  So not sure which blocks we're reflowing that much.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So yeah... removing the margin-collapse retry makes this page render beautifully.

Which makes sense -- the page has a set of deeply nested blocks each of which ends up needing the retry.  So the time taken is exponential in the depth of the nesting.  :(

So this looks like a different issue from bug 273293, and one we should really try to deal with for Gecko 1.9, imo.

Comment 21

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

After reflow branch has landed, I'll try to make the second-pass reflow more incremental so it doesn't have to reflow block children that haven't changed.

Comment 22

[Martijn Wargers (dead)]

(In reply to comment #21)
> After reflow branch has landed, I'll try to make the second-pass reflow more
> incremental so it doesn't have to reflow block children that haven't changed.

In case you haven't noticed, the reflow branch has landed ;)

Comment 23

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

Attachment: proof of concept

This patch nails the performance problems completely, as far as I can tell. The idea is to abort reflow as soon as we detect that a second pass will be needed. I believe this reduces possible O(2^N) behaviour to O(N).

However, this approach to aborting reflow is, at best, a huge hack. David, what do you think the right interface should be to allow Reflow() to abort this way? A magic rv, a new flag in nsHTMLReflowMetrics, or a new nsRelowStatus? And how specific should the abort signal be...

Comment 24

[Eli Friedman]

Attachment: Simplified version of proof of concept

It seems to me that we don't need another flag or special return; we already have aState.mReflowState.mDiscoveredClearance to indicate this situation.

Comment 25

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

Comment on attachment 273031 [details] [diff] [review]
Simplified version of proof of concept

Mmmmm ... yeah!!!

Comment 26

[Eli Friedman]

Checked in.

Comment 27

[Martijn Wargers (dead)]

Would be nice if this could be tested in some way.

Comment 28

[Eli Friedman]

We could just toss the testcase into reftests so that they'd fail if this regressed.

Comment 29

[Martijn Wargers (dead)]

Ok, but I especially meant the performance part of the bug. I mean, I think it's important to not regress this again.

Comment 30

[Eli Friedman]

If we put a long enough version of the testcase into reftests, the testing scripts will kill it :)

I think reftests automatically fail if they take more than 20 secs or something like that.

Comment 31

[pal-moz]

maybe regression by this check-in, see below.
http://forums.mozillazine.org/viewtopic.php?p=2980171#2980171

Comment 32

[Steve England [:stevee]]

I filed bug 389398 on the apparent regression.

Comment 33

[Eli Friedman]

Backed out due to the regression + freeze.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I really think this should be blocking, not just wanted....

Comment 36

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

This is pretty bad, and it blocks a blocker, so we'd better reevaluate it.

Comment 37

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

Attachment: updated patch v7

Here's a revised version of Eli's patch. The main change is that instead of returning from ReflowDirtyLines when we detect the clearance-change, we keep going, we just don't reflow any of the dirty lines (treating them as not dirty). This fixes the regression that patch caused, which I think was caused by SlideLine not being called so we forget that lines have moved. In general it's a more conservative fix because we execute mostly the same ReflowDirtyLines code in the "clearance changed" case as the normal case. It still fixes performance in the testcases mentioned in this bug nicely, though.

The other change is to make CheckFloats not spew warnings when lines haven't had their float caches updated. If there are dirty lines, then we will reflow again so it's too early to spew warnings.

Comment 38

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

I don't know where "v7" came from...

Comment 39

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

P1 because it's moderately risky and fixes real pages

Comment 40

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

Comment on attachment 286522 [details] [diff] [review]
updated patch v7

OK, r+sr=dbaron.  Sorry for the delay.

Comment 41

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

checked in.