60494 - Mozilla is extremly slow rendering this page lots of <font> tags

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Jens-Uwe]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.14 i686; en-US; m18) Gecko/20001116
BuildID:    20001116

When going to this web page, Mozilla requires ages to render the page. To make
things even worser, Mozilla blocks all other windows for quite some time!
NS4.x and Konqueror render this page very quickly.


Reproducible: Always
Steps to Reproduce:
1.go to www.dooyoo.de
2.go to shopping -> Partner Shops


Actual Results:  Mozilla renders this page extremly slowly and blocks all other
browser Windows for quite some time. It doesn't matter, if JavaScript is enabled
or not.



Expected Results:  Mozilla should be WAY faster with this

I reproduced it several times here with a 2Mbit connection to the internet, the
same connection I used for the Netscape 4.x and Konqueror Tests, so I doubt that
this Problem is caused by a slow Internet connection.
Its "only" a performance problem, but perhaps an Optimization here could benefit
on other positions too?

Comment 1

[Matthias Versen [:Matti]]

I can confirm this.
I see several JS errors in the console :
JavaScript error:
http://194.221.80.102/rollover.js line 2: event is not defined

I send this bug to JS

That is the direct URL :
http://d39f2bd329263650955038b7b811b32dc.dooyoo.de/servlets/GuideConnector?template=shop_prtnr_list&sessionID=&pid=&s=

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

Updating summary to reflect that this is a rendering issue..

Comment 3

[Matthias Versen [:Matti]]

I forgot somethink :
my build : 2000111604 on win2k
changing OS from Linux to ALL

Comment 4

[Matthias Versen [:Matti]]

Sorry for the SPAM
My URL don´t work, please use the URL from the reporter !
It seems that this page generate this everytime new.

Comment 5

[Jens-Uwe]

Matti:
Dooyoo is generating the web-server adress new for each user. Dooyoo uses a
program to track the movements of the users, even if they leave their web site.

As the reporter I am strongly against moving this to "JavaScript Engine", the
main bug also occurs with JavaScript disabled !!! That there are JS-Problems
too, but they should be a new bug I think!

Comment 6

[Asa Dotzler [:asa]]

OK, this isn't javascript engine or even javascript related.  This is another
case of "we aren't very performant when we have to parse a million <font> tags.
 I'll attach two test cases.  The first is the page with poor performance (with
all the font tags). The second attachment is the page stripped of all the font
tags.  You will see that the second page is much faster than the first.

Comment 7

[Asa Dotzler [:asa]]

Attachment: testcase demonstrating really bad performance

Comment 8

[Asa Dotzler [:asa]]

Attachment: same testcase with all the millions of <font> tags removed (much faster)

Comment 9

[Doron Rosenberg (IBM)]

performance issue

Comment 10

[Jens-Uwe]

Asa: I am not sure, if this is really the font tag, since I don't see much of a
difference between both test cases.
Anyway, try this page once with Pictures enabled and once with pictures
disabled. With disabled pictures the page is rendered quite fast, while it is
extremly slow with pictures enabled! This might be a reason why the testcase
seemed to be much faster for you.
I am still not sure, why this is so slow. I would love to see what a profiler
says to this... Is there a nice, easy to use tool for Linux??

Comment 11

[Doron Rosenberg (IBM)]

hmm, I have to agree, both testcases are slow, the images turned off speeds
this. cc: pnunn

Comment 12

[Jens-Uwe]

Ok, I managed to get jprof running, and here is, what jprof told me. Sadly some
of the code in question is not well documented, so I could not see that much in
the short time I had so far, except that is has something to do with tables of
course ;-)
On my system (K6-2+/500, 256MB) and all datas taken from a local proxy, this
page needs arround 2 minutes until finished. 15 seconds before it begins with
the deletes the old page and starts with the new one, the rest is required for
rendering. I measured the rendering time only! Netscape4 needs less then a
second for the same page!

Count   Function Name
177     BasicTableLayoutStrategy::ComputeNonPctColspanWidths(int,
nsTableCellFrame *, int, int, int, int &, float)
165     nsVoidArray::ElementAt(int) const
147     _init
119     nsVoidArray::Count(void) const
108     nsCellMap::GetMapCellAt(nsTableCellMap &, int, int, int)
73     nsTableColFrame::GetWidth(unsigned int)
67     nsTableFrame::GetCellMap(void) const
66     nsCellMap::GetCellInfoAt(nsTableCellMap &, int, int, int *, int *)
60     nsTableFrame::GetNumCellsOriginatingInCol(int) const
53     nsCellMap::GetEffectiveColSpan(nsTableCellMap &, int, int, int &)
49     nsTableFrame::GetCellInfoAt(int, int, int *, int *)
42     BasicTableLayoutStrategy::ComputeNonPctColspanWidths(nsHTMLReflowState &,
int, float)
41     nsTableCellMap::GetCellInfoAt(int, int, int *, int *)
37     nsCellMap::GetCellAt(nsTableCellMap &, int, int)
35     nsTableCellMap::GetNumCellsOriginatingInCol(int) const
34     nsSplittableFrame::GetFirstInFlow(void) const

Comment 13

[Jens-Uwe]

it seems, that
BasicTableLayoutStrategy::ComputeNonPctColspanWidths(nsHTMLReflowState &, int,
float) is guilty.
1. its called way too often, probably always with the same input (several
variables are always the same)
2. its algorithm runs amok at the partner-shop page of www.dooyoo.de. numRows =
283, numCols = 6 and NUM_MAJOR_WIDTHS = 3 (as I said, the variables have always
the same values!), and since some parts of the code is called
NumRows*numCols*NUM_MAJOR_WIDTHS times, this is a high number!
3. it calls BasicTableLayoutStrategy::ComputeNonPctColspanWidths(int,
nsTableCellFrame *, int, int, int, int &, float) way too often. in the if
(MIN_CON == widthX) condition the call is done 217 times in IF, and 217 times in
ELSE. This is VERY often!!!

should this go to Layout?

Comment 14

[Doron Rosenberg (IBM)]

possible dupe/related to 60731?

Comment 15

[Jens-Uwe]

doron: it might be related to bug 60731. We also have hundreds of font-tags, and
not all of them are good <font> </font> pairs!
What does the standard say, what should happen, when we have a <font>, and then
comes a new <font>, without a </font> in between?

Comment 16

[Doron Rosenberg (IBM)]

nested fonts are fine, as long as you close all ;)
we currently (according to harishd on the other bug) have a limit to 100 nested
tags. This is only temporary until some memory issues are resolved.

Comment 17

[Jens-Uwe]

doron or asa:
could one of you please move this to "layout"? Perhaps there is someone knowing
more about the code in question...
The problem in this bug is arround the function
BasicTableLayoutStrategy::ComputeNonPctColspanWidths. This function is called
with values, which makes it running amok (see my comments some lines above).
This function is located in /layout/html/table/src, so I guess it should go to
layout!

Comment 18

[Doron Rosenberg (IBM)]

layout it is

Comment 19

[Jens-Uwe]

doron: thanks

Okay, could perhaps someone at layout explain me in a few words, what the
BasicTableLayoutStrategy::ComputeNonPctColspanWidths function does and why it
might behave badly?
I've played a bit with the sources but somehow I don't find a starting point :-/

Comment 20

[Jens-Uwe]

little problem: Dooyoo made a relaunch and it seems this page disappeared :-/
what to do now?

Comment 21

[Jens-Uwe]

Attachment: the page for offline testing

Comment 22

[Jens-Uwe]

Since I think that this bug will probably pop up on another web site I think it
still should be fixed. I found a copy of the page in my local proxy server and
changed it so that you can (hopefully) use it for testing.
Please unzip with sub directories and do an "Open File" on the
GuideConnector.html. It still takes ages, even with all files stored on the
local disk.
Hope you enjoy :-)

Comment 23

[Bernd]

I am looking in to it

Comment 24

[Bernd]

BasicTableLayoutStrategy.cpp -> table

Comment 25

[Bernd]

 I dont think that BasicTableLayoutStrategy.cpp is guilty it is more the victim. 
I have turned on the reflow statistics in nsTabeleFrame.cpp and looks that the 
whole page sees if not an infinite then a very large number of reflows. I give 
up to make a testcase from this. It is definitely connected with the images. I 
used the viewer and and somehow managed to get back without killing the viewer, 
when I then pressed the forward button, the page loaded pretty fast, without the 
excessive reflow cycle. In order to make a testcase one has to kill every time 
the viewer, to prevent the image caching.

Comment 26

[karnaze (gone)]

Marking mozilla0.8. I need to verify that incremental table reflow is not doing 
any more work than necessary. 

Comment 27

[Bernd]

*** Bug 63917 has been marked as a duplicate of this bug. ***

Comment 28

[Diego]

See also bug 57966: "Extremely poor scalability on nested tables"
and its dupes
(The url http://www.kuro5hin.org/?op=displaystory;sid=2000/10/20/16261/167
contains lots of font tags)

Comment 29

[karnaze (gone)]

Attachment: attachment #1 with some new lines added

Comment 30

[karnaze (gone)]

Attachment: attachment #2 with some new lines added

Comment 31

[karnaze (gone)]

Attachment: patch improving performance by removing unnecessary max width calculations

Comment 32

[karnaze (gone)]

On my home system (Pen350 + 128MB), the patch makes Viewer almost comparable to 
Nav4.x without relying on anything being cached. Mozilla is considerably slower 
than Viewer (1.5X to 2X), but it may be partly because I'm using a debug build. 
Without the patch Mozilla eventually hangs up after some scrolling. With the 
patch, I'm not seeing that.

There are still problems - (1) resizing is slow & (2) By putting any attachment 
into a simple nested table, the max width calculations (which the patch 
removes for the non nested cases) kick in again and the performance is bad.

Comment 33

[Jens-Uwe]

while Mozilla still seems to sleep for a short second, Mozilla now renders this
page really fast :-)
Karanaze, you get my hero of the day again, just a few days after fixing 63678
so fast a few days ago :-) I am surprised, how much speed up such a little patch
can do!
When will this patch be checked in?

Comment 34

[buster]

sr=buster.  please submit additional bugs for the remaining problems:
1) resizing is slow
2) reflow coalescing
3) nested table issues (chris thinks he already has a related bug for this that
should be noted here.)

Comment 35

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

Adding patch, testcase

See also bug 54542

Comment 36

[Marc Attinasi]

r=attinasi

Comment 37

[Jens-Uwe]

karnaze: this bug has now a "sr=buster" and an "r=attinasi". Can you please
check in the patch after filing the new bugs requested by buster?? :-)

Comment 38

[karnaze (gone)]

The patch is in.

Comment 39

[karnaze (gone)]

I just checked in code that fixes the problem of 
BasicTableLayoutStrategy::ComputeNonPctColspanWidths getting called excessively. 
The total time (on my Pentium II 450MHZ) spent in table reflow went from 10 
seconds to 3 seconds on the 1st attachment. The 10 seconds was with the patch 
(attachment #6 [details]).

Comment 40

[James Green]

I'm not seeing much slowness on the given URL following this patch. The first
testcase attach loads reasonably well, certainly nothing that could be
considered "extreemly slow", and it doesn't lock up during render. The second
testcase renders in no time. I guess you could say verified on Linux :).

Comment 41

[Jens-Uwe]

james: attachment 3 [details] [diff] [review] is the extremly slow one. It took more then 2 minutes before
karnaze's first patch, which reduced it to just a few seconds :-)
attachment 3 [details] [diff] [review] is a .zip file. If you still have an older build (older then last
friday;) lying arround and you want to see the mentioned effect, just download
attachement 3, unzip it in subdirs and load the html-file... 
I also have Mozilla on Linux...

Comment 42

[Amarendra Hanumanula]

QA contact update

Comment 43

[Amarendra Hanumanula]

 The given URL is rendering fast enough.. Marking verifed