Walter Reporter
	Description • 22 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030308
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030308

The right column at http://theburritosband.vivelared.com/index.php, wich uses
Javascript to scroll some links makes Mozilla1.3 to use 100% of cpu and then hangs.

I had the same problem with Phoenix0.5 (1.3) but it only lasted a seconds, after
that the cpu utilization used to go down to normal cpu use.

Reproducible: Always

Steps to Reproduce:
1. Visit http://theburritosband.vivelared.com/index.php
2. Watch your CPU utilization
3.




I have tested it on Linux and Windows2K.

	Matthias Versen [:Matti]
	Comment 1 • 22 years ago

wfm with win2k build 20030303.. (no hang)

why should this be a blocker ? -> critical

	Phil Schwartau
	Comment 2 • 22 years ago

I see the problem with Mozilla trunk binary 2003022808 on WinNT.
My CPU is 100% for almost a minute while the page loads. The 
browser doesn't hang: one can interact with it, but only slowly.

This does not appear to be a JavaScript problem. The site is 
very light on JavaScript: I see hardly any. The marquees are
not created via JavaScript, but by <marquee> elements in HTML:

<MARQUEE behavior= "scroll" align= "center" direction= "up" height="120"
scrollamount= "2" scrolldelay= "90" onmouseover='this.stop()'
onmouseout='this.start()'>

<MARQUEE behavior= "scroll" align= "center" direction= "up" height="420"
scrollamount= "2" scrolldelay= "20" onmouseover='this.stop()'
onmouseout='this.start()'>

I did some reductions via Composer and found that removing some of
the content to the left of the marquees, or above them, resulted in
a much faster load time. So perhaps it's a reflow issue of some sort?

Reassigning to Layout for further analysis -

	Daniel Wang
	Comment 3 • 22 years ago

the page contains lots of <STYLE="text-decoration: none">
removing them solves the problem

-> parser

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 22 years ago

What does this have to do with parser?

You should try profiling the page before making decisions like that; at a guess,
what's going on here is that we add lots of style sheets (one per <style>
element) and each time we add a sheet we have to reresolve style all over.

Perhaps we could pass along some boolean in StyleSheetAdded saying whether the
sheet actually has any rules?  Or the presshell could do that check before
blowing away style data...

	Daniel Wang
	Comment 6 • 22 years ago

note: there are lots of open <style=> and <font> tags. Removing some of
them solves the problem

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 22 years ago

Total hit count: 1180

Of these 1034 happen under PresShell::ReconstructStyleData(int).  1065 hits
happen under ReResolveStyleContext; the top of the flat profile is:

Total hit count: 1180
Count %Total  Function Name
 82   6.9     SelectorMatches(RuleProcessorData &, nsCSSSelector *, int,
nsIAtom *, signed char)
 55   4.7     FrameManager::ReResolveStyleContext(nsIPresContext *, nsIFrame *,
nsIContent *, int, nsIAtom *, nsStyleChangeList &, nsChangeHint, nsChangeHint
&)
 43   3.6     RuleHash::EnumerateAllRules(int, nsIAtom *, nsIAtom *,
nsVoidArray &, void (*)(nsICSSStyleRule *, void *), void *)
 34   2.9     RuleProcessorData::RuleProcessorData(nsIPresContext *, nsIContent
*, nsRuleWalker *, nsCompatibility *)
 33   2.8     PL_DHashTableOperate
 26   2.2     nsCOMPtr_base::begin_assignment(void)
 26   2.2     SearchTable
 22   1.9     nsCOMPtr_base::~nsCOMPtr_base(void)
 21   1.8     ContentEnumFunc(nsICSSStyleRule *, void *)

Pretty much what you'd expect given the amount of time spent rebuilding the
style context tree....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 22 years ago

Comment on attachment 119596 [details] [diff] [review]
possible patch, untested

OK, this patch speeds up this testcase by a factor of 12 or so according to
jprof (and "noticeably" according to wall clock).

Also fixes a bug where we could incorrectly leave rulenodes in the tree when a
style sheet got disabled...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 22 years ago

Comment on attachment 119596 [details] [diff] [review]
possible patch, untested

The other option, of course, is to pass along in the notification itself
whether the sheet contains any rules....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 22 years ago

.

	Markus Hübner
	Comment 11 • 22 years ago

Great, can we get this into 1.4beta ?

	David Baron :dbaron:
	Comment 12 • 21 years ago

I was actually thinking of relying on the rule tree GC (and perhaps tuning the
parameters) and getting rid of the rule tree rebuilding code here.

The explicit CSS stuff seems ugly -- how about a new method on nsIStyleSheet? 
(|HasRules|?)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 21 years ago

Comment on attachment 119596 [details] [diff] [review]
possible patch, untested

The problem is not the ruletree rebuilding -- it's not rebuilt, in fact, since
sheets are being added, not removed.

The problem is the top-down reresolve of style on every single piece of content
in the document.

Yes, HasRules is probably a better idea; I'll do something like that once I get
back to working on Moz.

	David Baron :dbaron:
	Comment 14 • 21 years ago

I was just mentioning the rule tree rebuilding because I didn't see any reason
to add more of it.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 21 years ago

Oh, the second hunk.  I see.  Yeah, that's a good point.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 21 years ago

Comment on attachment 126862 [details] [diff] [review]
More ambitious patch

David?	What do you think?

	David Baron :dbaron:
	Comment 18 • 21 years ago

Comment on attachment 126862 [details] [diff] [review]
More ambitious patch


>+NS_IMETHODIMP_(PRBool)
>+CSSStyleSheetImpl::HasRules() const
>+{
>+  if (!mInner || !mInner->mOrderedRules) {
>+    return PR_FALSE;
>+  }

Why bother?  StyleRuleCount does the same thing.

>+
>+  PRInt32 count;
>+  StyleRuleCount(count);
>+  return count != 0;
>+}

>+NS_IMETHODIMP_(PRBool)
>+HTMLCSSStyleSheetImpl::HasRules() const
>+{
>+  return mFirstLineRule || mFirstLetterRule;
> }

This should unconditionally return true, because of inline style rules, and
because the firstline and firstletter rules are allocated as needed.

> NS_IMETHODIMP
> PresShell::StyleSheetAdded(nsIDocument *aDocument,

>+
>+  if (!applicable) {
>+    return NS_OK;
>   }
> 
>-  return NS_OK;
>+  if (!aStyleSheet->HasRules()) {
>+    return NS_OK;
>+  }

how about:

if (!applicable || !aStyleSheet->HasRules) {
  return NS_OK;
}

(and the same for StyleSheetRemoved)

r+sr=dbaron

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 21 years ago

Checked in.

	Daniel Wang
	Comment 21 • 21 years ago

v