Closed Bug 97815 Opened 23 years ago Closed 3 years ago

Font statement is causing performance problems

Categories

(Core :: Layout, defect)

defect
Not set
major

Tracking

()

RESOLVED WORKSFORME
Future

People

(Reporter: ire0, Unassigned)

References

()

Details

(Keywords: memory-footprint, perf, testcase, Whiteboard: [residual style])

Attachments

(3 files)

I have a large customer testcase that has the following statement after the body statement <font face="ARIAL"> . Removing the font statement helps response time by more than 4 seconds (26 secs drops to 21.5). I know they don't have a </font> statement; however, when I default to Ariel without the font statement I still save the 4 seconds. In addition, if I have 10 <font face="ARIAL"> statements in a row I add 40+ seconds to my time (I am running on a 333 Mhz OS/2 and a 333 MHz NT machine). The testcase is a large unordered list. Also, if I change the font statement to use a make believe font name <font face="XYZ"> it still costs me 4 seconds for every statement if I leave out the </font>. 10 font face statements (with out the /font) cost me over 40 seconds additional. Why should a font statement with an invalid font face cost me so much time? Why should multiple badly coded font statements cost so much?
I tried the same tests using 4.7 and found no performance problems. The problem has been around at least since .601 version. The important thing to note here is if there is no font statement and I default the same font I improve over 4 seconds. If a put a </font> in right before </body> I still get charged 4 seconds for the font statement. Why should the cost be anything more than the time it takes to parse the statement if using it isn't the cost?
Is it the case that only *unmatched* <font> tags are causing this slowdown?
If I place the </font> just before the </body> I get the same results.
Data - Putting some printf's into some of the methods seeming to be involved,Below is only snippet of trace and only some methods have had trace printf added. There's probably more going on than in this one area, but this one area stood out to me.... (anyway, enough disclaimers....) WithOUT a font statement get sequences (hundreds/thousands) as such: 07:53:24.200 HTMLStyleSheetImpl::SetAttributeFor 07:53:24.200 HTMLAttributesImpl::SetAttributeFor $Y. 07:53:24.200 HTMLAttributesImpl::SetAttributeName $Y. 07:53:24.200 (exit) HTMLAttributesImpl::SetAttributeFor $Y. 07:53:24.200 (exit) HTMLStyleSheetImpl::SetAttributeFor WITH a font statement get sequences (hundreds/thousands) as such: 07:53:37.360 HTMLStyleSheetImpl::SetAttributeFor 07:53:37.360 HTMLAttributesImpl::SetAttributeFor $Y. 07:53:37.360 HTMLAttributesImpl::SetAttributeName $Y. 07:53:37.360 HTMLAttributesImpl::EnsureSingleMappedFor 07:53:37.360 nsHTMLMappedAttributes::*constructor* 07:53:37.360 nsHTMLMappedAttributes::Init 07:53:37.360 (exit) HTMLAttributesImpl::EnsureSingleMappedFor 07:53:37.360 nsHTMLMappedAttributes::SetAttribute 07:53:37.360 (exit) nsHTMLMappedAttributes::SetAttribute 07:53:37.360 HTMLAttributesImpl::UniqueMapped »jÿ«j 07:53:37.360 HTMLStyleSheetImpl::UniqueMappedAttributes 07:53:37.360 (exit) HTMLStyleSheetImpl::UniqueMappedAttributes 07:53:37.360 (exit) HTMLAttributesImpl::UniqueMapped »jÿ«j 07:53:37.360 (exit) HTMLAttributesImpl::SetAttributeFor $Y. 07:53:37.360 (exit) HTMLStyleSheetImpl::SetAttributeFor It's really unclear to me what's going awry but, as a guess, appears a set of attribute mappings are being many times created/discarded and recreated,rather than reused, throughout the parsing of the document. Sam
"Clarence (Andreas M. Schneider)" <c@c07.de> suggested: "Try this: <style> font { border: 1px solid red } </style> <font> <p>text</p> <p>text</p> <p>text</p> and you'll see why it's expensive." Indeed, give it a try! My questions are - Is this "valid HTML"? The text renders differently on NS 4.x, Mozilla, and Opera. (try it!) Which is 'correct rendering'? or maybe..... why is mozilla choosing to implement what is clearly the most expensive implementation of this bit of html? Sam
If the last <p> is unclosed, behavior is like in testcase 1, even if <font> is closed. All examples are invalid HTML.
Data - using ye'ol HTML Reference Manual (previously http://www.sandia.gov/sci_compute/html_ref.html but removed from there a year or two ago, is there on-line pointer to it now?) So then, load time reported by mozilla status time on 700MHz Thinkpad, html_ref 'as is': 2.3 sec Add 1 <font face> 3.7 sec Add 2 <font face> 5.6 sec Add 3 <font face> 7.4 sec Add 4 <font face> 9.1 sec Add 5 <font face> 11.2 sec ... Add 10 <font face> 20.5 sec Seems pretty clear. Sam
Oh, forgot to add, same html_ref <1 second netscape 4.7 in all cases. Sam
Is the fix for bug 96031 going to change the behavior here?
Apparently, fix for bug 96031 does seem to change the behavior. Will have to discuss with Marc about it.
Ivan, do you mean that the problem exists in 6.01? If so, that would mean it had to exist prior to my style system rewrite.
The problem does exist in 6.01 (using the OS/2 version for my information).
Keywords: perf
Blocks: 71668
Possible fix for 2 performance bugs (106513 and 97815) Problem: Large text documents that use style tags such as <font> run much faster When carriage returns (\r) and endlines (\n) are removed. By placing nine <font> Tags immediately after the <body> tag in my large text document the rendering slowed by over 10 seconds. Removing the all carriage returns / endlines speeded up the performance by the full 10 seconds. A single <font> costs about a second but I used 9 to show how the problem scales. Possible fix: I tracked down a test in CNavDTD::OpenTransientStyles as the cause of the slowdown. nsresult CNavDTD::OpenTransientStyles(eHTMLTags aChildTag){ nsresult result=NS_OK; // No need to open transient styles in head context - Fix for 41427 if((mFlags & NS_PARSER_FLAG_ENABLE_RESIDUAL_STYLE) && eHTMLTag_newline!=aChildTag && mOpenHeadCount==0) { By removing the eHTMLTag_newline!=aChildTag from the code I got a major performance Benefit. I did not notice other changes because of this fix. I do not understand why the test for newline is there, only that it dramatically slows down some of my tests. I don't know if removing the test will cause other problems; however, it should be investigated. I can create the same performance results by generating a whitespace token and adding it to the mTokenDeque after placing a newline token on the mTokenDeque or replacing endline tokens with whitespace tokens. I assume that a fix in this area will benefit any documents using Residual style tags such as Font, small, tt , etc. Ivan
harishd?
-->harishd.
Assignee: karnaze → harishd
Blocks: 56854
No longer blocks: 71668
*** Bug 106513 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
harish, not sure what your latest (comment#20) meant to mean. That seems to be the place the questionable if(eHTMLTag_newline!=aChildTag) was added..... but the code all around this has changed a whole lot since then. Sam
Sam: Sorry! I just added the checkin to show that it was added for a reason...but unfortunately that checkin wasn't documented at all.
>but unfortunately that checkin wasn't documented at all. Yea, with sadness, I noticed that too ;-) Allow me to step back and recap what we've found on this. This bug (97815) opened when we found multiple font/style/attribute changes, even if identical to previous style (thus not even really a change), each adds significant time to document. Later, we got into investigating why it was the case that for some cases we were getting great response time improvement from use of 'html cleaner/stripper' utilities. Such utilities just compress out non-formatting blanks, CRLF, and comments from html source without altering content. In some cases the performance improvement from use such utilities was quite surprisingly large. Then, to our amazement and confusion, we found these two things somehow related. When html source is stripped of newline line ends, the performance problem with multiple style changes goes away. Ivan isolated the single line of code that somehow is crux of the problem. Removing the newline conditional in that line of code makes the stripped and unstripped performance/behavior largely the same. The CRLFs being stripped are no way related to doc latout (no more than line ends in C++ source code related to compiled C++ execution), so its a quite surprising result to find these things somehow related. The suggested code change may not be the best/right change, but at least gives another clue to the problem.
Sam: When you mention CRLFs do you really mean CR-LF combo?
By CRLF, I really meant generic line ending char(s), depending on what platform, text editor, html generator, etc, created the file. Outside of preformated text block, CR or LF or CRLF are all equivalent, I think. Sam
Perhaps I'm missing something here, but it sounds to me like the problem is that within an open <font> tag (incorrectly extended over blocks, quirks mode, blah blah), residual style handling kicks in. These line endings between tags are treated as whitespace by the residual style handler, and mayhem ensues. Theoretically, I suppose this could be dealt with by a full fix of bug 2750 if we treated misnestings like <p> in <font> as "improper subelements," but I'm sure trying to apply this bit of SGML pedantry in quirks mode would lead to disaster.
Christopher, not sure this fits into your theory, but if the all instances of line end chars are replaced by blanks then performance problem goes away. Problem seems unique to line end chars, not whitespace. Sam
Perhaps there's a difference between the handling of whitespace tokens and newline tokens that there shouldn't be?
that's of course why bug 106513 was duped against this bug (comment 19)
Out of time --> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
OS: Windows NT → All
I'm assuming this won't make 0.9.8. There are lots of pages with lots and lots of unclosed <font> tags, and on long pages apparently this makes a significant hit.
The problem is not unclosed <font> tags. Its any residual style (font is a residual style) that has lots of heirarchical tags (like <ul> tags within <ul> tags. I found that placing a single blank (future whitespace token) after the first <ul> speeded up my performance by seconds. I am investigating how a whitespace tag is processed in CNavDTD::OpenTransientStyles. It seems to prevent many OpenContainer call on the font node. I am not sure how this causes slowdowns down stream when we are formatting/reflowing.
Out of time. Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Mass moving to 1.1.
Target Milestone: mozilla0.9.9 → mozilla1.1
Keywords: mozilla1.0+
Hardware: PC → All
*** Bug 142143 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.1alpha → mozilla1.1beta
*** Bug 99435 has been marked as a duplicate of this bug. ***
I revisited this bug today using the 1.0.1 nightly. The problem is still there. When I place a single blank after the first UL in my testcase (http://bugzilla.mozilla.org/attachment.cgi?id=54915&action=view) my time drops 2 full seconds (from 17.3 to 15.3). This problem also went away when I removed the eHTMLTag_newline!=aChildTag from the CNavDTD::OpenTransientStyles code. See line 2910 for existing code. nsresult CNavDTD::OpenTransientStyles(eHTMLTags aChildTag){ 2906 nsresult result=NS_OK; 2907 2908 // No need to open transient styles in head context - Fix for 41427 2909 if((mFlags & NS_DTD_FLAG_ENABLE_RESIDUAL_STYLE) && 2910 eHTMLTag_newline!=aChildTag && mOpenHeadCount==0) { There has not been any activity on this bug for months. Thanks, Ivan
Keywords: mozilla0.9.9mozilla1.2
[was 1.1beta]
Target Milestone: mozilla1.1beta → Future
regarding comment #32 shouldn't we change the bug summary?
*** Bug 165860 has been marked as a duplicate of this bug. ***
*** Bug 168636 has been marked as a duplicate of this bug. ***
Blocks: 71668
Blocks: 123267
Blocks: 81993
I'm seeing the same DOM constructed whether or not there are newlines in the source.... I wish someone had attached the testcase where removing newlines had n effect to this bug. :(
Whiteboard: residual style
Whiteboard: residual style → [residual style]
*** Bug 243325 has been marked as a duplicate of this bug. ***
Markus in comment #40 > regarding comment #32 shouldn't we change the bug summary? what do you suggest? FWIW - Ivan (reporter) writes "I haven't looked at the browser stuff in 6 years. Not sure this is still a problem."
Assignee: harishd → nobody
Status: ASSIGNED → NEW
QA Contact: chrispetersen → layout
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4) Gecko/20100503 Firefox/3.6.4 Unable to reproduce using Firefox 3.6.4.
This bug doesn't applies to Firefox 7.

Marking this as Resolved > Worksforme since the issue doesn't happen on Windows 10 on either of the Firefox versions.
Also last comments suggest this issue might have been fixed long time ago.
If anyone is still able to reproduce this please re-open or file a new issue.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: