{ib} extremely slow layout of page with many inline tags (<PRE> involved?)

VERIFIED FIXED in M18

Status

()

Core
Layout
P1
major
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: barath, Assigned: Chris Waterson)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX], URL)

Attachments

(5 attachments)

(Reporter)

Description

19 years ago
I'm using Milestone 11 on a standard Redhat Linux 6.1 box, and viewing an image
that has been turned into a graphic such as the url provided, mozilla goes
extremely slow and becomes unresponsive for a long time.  This is in contrast to
netscape communicator or IE, which render and scroll with such an image
(text-image) rather quickly.

Thanks.

Comment 1

19 years ago
the page is not there anymore

Comment 2

19 years ago
Reassigning all of leger's unscreened Browser-General bugs to nobody@mozilla.org
for pre-screening and triage.

Updated

19 years ago
Whiteboard: (12/28) invalid URL; need an example to proceed further

Comment 3

19 years ago
barath@uclink4.berkeley.edu : can you check the URL for this bug report. I
don't understand the problem from the description, and the URL provided is
404 Not Found.

Updated

19 years ago
Assignee: nobody → pierre
Component: Browser-General → Style System
Whiteboard: (12/28) invalid URL; need an example to proceed further

Comment 4

19 years ago
From barath@uclink4.berkeley.edu, by email to 3jrgm@qlink.queensu.ca:

  "Try using the program png2html to create a html based image and then view
  it in mozilla."

So, as an example page already on the web, we can just use the png2html home
page : http://www.engr.mun.ca/~holden/png2html.html [Barath : is this a good
example of what you are seeing? (Add comments to this bug report).]

Using WinNT 1999122208, this takes roughly twice the time to render, as compared
to Nav4.7 on WinNT. However, I note that this bug report is specifically for
Linux, so the display may be even slower on Linux (based on the original
description [barath: can you quantify how much slower vs. Nav4.7 on Linux]).

Setting to 'Style System'/pierre for a further look (but maybe this is a
painting issue). (It's a fairly degenerate example, of course, but may point
out some possible areas to tune up.)

Updated

19 years ago
Assignee: pierre → troy
Component: Style System → Layout
OS: Linux → All
Hardware: PC → All
Summary: extremely slow rendering of text-based images → [perf]extremely slow rendering of text-based images

Comment 5

19 years ago
It's slow on all 3 platforms. It is especially slow when:

- loading the page

- scrolling

- selecting several lines of text at once



Running with MOZ_PERF shows an unusally long reflow time. Reassigned to Troy.

Updated

19 years ago
Assignee: troy → kipp

Comment 6

19 years ago
On my machine running NT and with the latest build it loads in a very reasonable
time.

However, it could be faster and so since it's all done with preformatted text
this is the known issue that the block code needs to be optimized for
preformatted text

Updated

19 years ago
Target Milestone: M15

Comment 7

19 years ago
mass-moving bugs to M15

Updated

19 years ago
Keywords: perf
Summary: [perf]extremely slow rendering of text-based images → extremely slow rendering of text-based images

Comment 8

19 years ago
this is actually pretty darn fast now.  acceptable for beta, moving to M16, 
lowering priority.  won't close out yet, this is still an interesting test case 
to profile.  If anybody wants to run a quantify profile of this, it would be 
VERy helpful!
Keywords: helpwanted
Priority: P3 → P4
Target Milestone: M15 → M16

Comment 9

19 years ago
*** Bug 24766 has been marked as a duplicate of this bug. ***
Upping the severity of this, as it makes mozilla completely unusable for viewing
large files in lxr such as:

http://lxr.mozilla.org/seamonkey/source/include/allxpstr.h#7570
Severity: normal → major
Summary: extremely slow rendering of text-based images → extremely slow rendering of <PRE> blocks

Comment 11

19 years ago
kipp is very unlikely to fix these, since he's not working ont he project any 
longer.  so I'll take a look.
Assignee: kipp → buster

Comment 12

19 years ago
Now that we are post-beta, is there anyone actively working with this? It really
hurts LXR, and I have a gut feeling (but no data) that it's probably hard on
xmlterm too.

Comment 13

19 years ago
well, post-beta branch anyway. I submitted too quickly :-)

Comment 14

19 years ago
I doubt anyone will work on this before M17

Comment 15

18 years ago
performance issues are high priority now.  Will try to look at this for M16.
Status: NEW → ASSIGNED
Priority: P4 → P1

Comment 16

18 years ago
changed the summary.  It's a layout issue, not rendering. And <PRE> may or may 
not be involved at all.  It may be the sheer number of inline tags, in this 
case, <FONT>.  Trying to get quantify to play nice...
Summary: extremely slow rendering of <PRE> blocks → extremely slow layout of page with many inline tags (<PRE> involved?)

Updated

18 years ago
Target Milestone: M16 → M17

Comment 17

18 years ago
one thing I noticed in quantify is the {ib} code to find "special" frames is 
killing us on large documents with long text runs.  Nisheeth is scheduled to fix 
this soon.  Nisheeth, when this part of the problem is fixed, please re-assign 
this back to me so I can find the next problem.

basically, the {ib} 'special' frame code is killing us, even though no inline 
element contains a block element.  ugg!
Assignee: buster → nisheeth
Status: ASSIGNED → NEW
Summary: extremely slow layout of page with many inline tags (<PRE> involved?) → {ib} extremely slow layout of page with many inline tags (<PRE> involved?)

Updated

18 years ago
Blocks: 40246

Comment 18

18 years ago
Accepting bug...
Status: NEW → ASSIGNED
(Assignee)

Comment 19

18 years ago
taking
Assignee: nisheeth → waterson
Status: ASSIGNED → NEW
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 20

18 years ago
Discovered a couple of things.

1. Long text runs with inlines get hosed because nsTextRun stores the run in a 
void array. Each span ends up as an entry here.

2. <pre>-formatted text with markup in it gets royally screwed here: generally 
the preformatted text is long, and for some reason we don't break the text run 
at the newline. It seems like we should probably do that. Any reason not to?

3. FindNextText() is called after measuring each new word: when the nsTextRun 
gets long, this becomes geometric and blows up.

Comment 21

18 years ago
You'll have to experiment with suggestion (2).  Off-hand, it sounds like it 
should be ok.

Comment 22

18 years ago
*spam* changing qa contact from nobody@mozilla.org to me (BlakeR1234@aol.com) 
on 121 open or resolved (but not verified) bugs.  sorry for the spam everybody, 
but most of these bugs would just remain dormant and not checked by QA 
otherwise.  I'm not sure how so many bugs have nobody as their QA contact, but 
I suspect this is the fault of some sort of bugzilla corruption that happened 
at some point (most of these bugs are in the 20000-26000 range, and I don't see 
where in the activity log that QA contact explicitly changed to 
nobody@mozilla.org)

Anyways, sorry again for spam.  If you really get annoyed, I'm usually 
available in #mozilla on IRC for torture.
QA Contact: nobody → BlakeR1234
(Assignee)

Comment 23

18 years ago
Created attachment 11246 [details] [diff] [review]
use JS's double-hashtable to maintain the framelist in nsTextRun
(Assignee)

Comment 24

18 years ago
Created attachment 11330 [details] [diff] [review]
Better patch: get rid of nsTextRun altogether.
(Assignee)

Comment 25

18 years ago
So I think the second patch is a better approach. Namely, get rid of nsTextRun 
altogether, and make nsLineLayout::FindNextText() grovel over the frame tree to 
find the next text frame.

The only concern that I have with this patch is that I undid the lazy parent 
pointer fixup stuff that troy did to fix bug 5588. Specifically, when overflow 
frames are pulled to newly created line boxes, troy had it set up s.t. their 
parent pointers would continue to refer to their original frames until reflow 
occurred. This makes it difficult to walk the frame tree :-).

I experimented with the test cast in 5588. Specifically, I backed troy's 
changes out of an otherwise clean tree. I was not able to observe a significant 
difference in performance (using the ultra-scientific "one mississippi, two 
mississippi" method).
(Assignee)

Comment 26

18 years ago
     Subject: Re: nsTextRun
        Date: Thu, 13 Jul 2000 08:28:47 -0700
        From: Troy Chevalier <troy@tellme.com>
Organization: Tellme
          To: Chris Waterson <waterson@netscape.com>

Chris Waterson wrote: 

  There is a fly in the ointment, though. I needed to undo the change that 
  you made to lazily set the overflow frames' parent pointers when 
  reflowing new lineboxes that get created on resize reflows. (I suppose I 
  could leave the change in, and add another eager detection case, but...) 

  I went back to the original test case (that big javadoc thing), and 
  honestly couldn't see a visible difference in layout speed after 
  removing the lazy parent pointer stuff. Was there another, better test 
  case I should look at?

That's an interesting issue. The main reason you don't see a layout change is 
because of this code in nsInlineFrame::Reflow(): 

  // When pushing and pulling frames we need to check for whether any 
  // views need to be reparented. 
  nsHTMLContainerFrame::ReparentFrameViewList(aPresContext, prevOverflowFrames, 
                                              prevInFlow, this); 

That's a new function that I added. Previously what happened is that in a loop 
we would call ReparentFrameView() once for each frame in the overflow list. That 
was the major performance problem, because each call did the exact same [very
expensive] check to see whether we needed to reparent the view or not 

Now that we make just the one call we only do that work once. Note that there is 
lots of layout code that is still using the old way, and it doesn't matter in 
galley mode but it's a performance problem in page mode 

There was a second reason performance was suffering, and that was because 
nsFrameList::InsertFrames() was walking the child list twice. The first time 
through it was calling SetParent() for each frame, and then it was calling 
LastChild() to get the last frame in the list. I changed the code that sets the 
parent frame to remember the last frame, and that made in O(n) instead
of O(2n) 

That may not seem like a big deal, but the scenario that was causing problems 
was something like this: 

<body> 
<span> 
everything in this span (100,000 elements) 
</span> 
</body> 

That's the worst case scenario, because we reflow the first line and only five 
word fit. So we push 99,995 elements to the next line, and for each one we 
called ReparentFrameView() and did the O(99,995 * 2) InsertFrames() operation. 
Then we fit five frames on the next line and pushed 99,990 frames to the next 
line, ... 

You get the idea. 

So skipping setting of the parent frame pointer if possible is still a good 
idea, because it saves us O(n). It's only an issue for large documents, but 
that's always been the problem area for us 

-Troy 
  
(Assignee)

Comment 27

18 years ago
I tried the test case that troy described (1Mb of text in a span), and saw no 
measurable difference with and without my patch. I *did* see a small difference 
on the original test case from bug 5588:

  http://java.sun.com/products/jdk/1.3/docs/api/allclasses-frame.html

On my machine, 4.1s without the patch, 5.4s with the patch (that's a degradation  
of over 20%). I'm profiling now to see what I can find. That test case has a 
long run of formatted HTML inside a table cell.
(Assignee)

Comment 28

18 years ago
In a subsequent email, troy pointed out that my test case was invalid because it 
only contained a single, long text frame. I tried it on a much larger test case 
(an LXR page contained inside of a <span> instead of a <pre>), and noticed 
marginal differences: 29s without the patch, 32s with the patch (10% 
degradation).

On LXR *with* a <pre> tag, the patch is a bigger win: from 35s without the patch 
do 25s with the patch (28% improvement).

At this point, my feeling is that removing the lazy parenting, and doing frame 
crawling is a net win:

1. (+) we avoid updating nsTextRun during incremental reflows
2. (+) we avoid storage overhead of the nsTextRun data (which saves us an 
nsVoidArray and its contents per block frame)
3. (-) we need to keep the parent pointers in sync eagerly to support frame 
walking during line wrapping.

I'll update my patch to leave the mSetParentPointer in mInlineFrame, and will 
experiment with this later to see if it's possible to do both lazy parent 
setting and frame crawling.
Keywords: helpwanted → nsbeta3
(Assignee)

Comment 29

18 years ago
Created attachment 11390 [details] [diff] [review]
Even better patch, do it without messing with lazy-parent-setting
(Assignee)

Comment 30

18 years ago
Created attachment 11393 [details] [diff] [review]
ok, one more time. this patch isn't edited by hand.
(Assignee)

Comment 31

18 years ago
Created attachment 11424 [details] [diff] [review]
use nsStyleDisplay to detect outermost block frame, not nsLayoutAtoms::blockFrame
[HAVE FIX] for perf issue. PDT please approve.
Whiteboard: [HAVE FIX]
(Assignee)

Updated

18 years ago
Target Milestone: M17 → M18
(Assignee)

Updated

18 years ago
Blocks: 39133
(Assignee)

Updated

18 years ago
Blocks: 26030

Comment 33

18 years ago
Add self to CC

In an attempt to say something useful in this spam, this bug:
http://bugzilla.mozilla.org/show_bug.cgi?id=2611
has some performance numbers on Mac for JDK 1.1 JavaDoc, which may be of 
interest.
(Assignee)

Comment 34

18 years ago
fix checked in. bye bye, nsTextRun.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 35

18 years ago
Can you let us know which nightlies have this in, so I can check whether the 
Javaoc loading issues have been solved?
(Assignee)

Comment 36

18 years ago
M18 builds from 2000-07-28 and later have the fix.

Comment 37

18 years ago
Doing the same test on the JavaDoc file AllNames.html that I was talking about in 
bug 2611, the 28 Jul M18 build performs as follows:

Loading from network @ java.sun.com: 26.4 seconds
Loading from local disk:29.8 seconds

Conversley the M17 28 July build (which doesn't have this fix)

Loading from network: 60 secons
Loaing from disk: 90 seconds

So congrats - this fix is a huge improvement!

Still doesn't match Nav4 at 15 seconds from the local disk, but its pretty good.

I'm still curious as to how loading a file from disk can be *slower* than from 
the network. Should I file a bug on this?
(Assignee)

Comment 38

18 years ago
cc'ing gagan: what do you think of the above comments? (re: network faster than 
file)

Comment 39

18 years ago
QA assigning to petersen to verify
QA Contact: blakeross → petersen

Comment 40

18 years ago
Marking verified in the 2001010203 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.