Last Comment Bug 117316 - repeated attribute changes lead to infinite rule tree growth
: repeated attribute changes lead to infinite rule tree growth
Status: RESOLVED FIXED
[patch]
: mlk, perf, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 major (vote)
: mozilla1.4alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Doron Rosenberg (IBM)
Mentors:
http://www.world-direct.com/mozilla/d...
: 142528 166298 (view as bug list)
Depends on: 166450
Blocks: 92580 166298 188803 193275
  Show dependency treegraph
 
Reported: 2001-12-29 05:17 PST by Markus Hübner
Modified: 2014-04-26 03:10 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduced testcase (374 bytes, text/html)
2001-12-29 08:02 PST, Markus Gerstel
no flags Details
patch (untested) (26.95 KB, patch)
2003-02-03 19:48 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (untested) (20.99 KB, patch)
2003-02-04 06:33 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (untested) (21.14 KB, patch)
2003-02-04 11:37 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (untested) (21.16 KB, patch)
2003-02-04 11:39 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (20.96 KB, patch)
2003-02-11 16:08 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (19.29 KB, patch)
2003-02-24 11:07 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (22.29 KB, patch)
2003-02-24 13:30 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (22.17 KB, patch)
2003-03-05 20:00 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Markus Hübner 2001-12-29 05:17:10 PST
1. just go to  http://www.formula-one.nu/dhtml/3d.htm
2. open the taskmanager
3. look at mozilla's memory usage - increasing about 300kb/sec !

memory usage will continue growing forever w/o never decreasing.

Verified using 0.9.7 and build 2001122803 on win2k and win-xp.
Comment 1 Arthur 2001-12-29 05:32:35 PST
Confirming on Linux 2001122821. Memory usage growth stops after closing window.
Comment 2 Olav Vitters 2001-12-29 05:33:48 PST
Also on Linux 2001122608. If I stop close the window and reopen it, it takes a
while for the memory usage to grow.
Comment 3 Markus Gerstel 2001-12-29 08:02:13 PST
Created attachment 63013 [details]
Reduced testcase

setAttribute("width") and ("height") seem to be the cause of this problem.
Comment 4 Boris Zbarsky [:bz] 2001-12-29 08:14:00 PST
Does adding |var widthHeight;| in global scope help?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-12-29 08:18:45 PST
The main sources of increase are the following:

18675361 malloc
  17746654 PR_Malloc
    17453053 PL_ArenaAllocate
      17505210 FrameArena::AllocateFrame(unsigned int, void **)
        17505210 PresShell::AllocateFrame(unsigned int, void **)
          17496980 nsPresContext::AllocateFromShell(unsigned int, void **)
            5411225 nsResetStyleData::operator new(unsigned int, ...
            3962745 nsStyleBorder::operator new(unsigned int, nsIPresContext *)
            3168550 nsInheritedStyleData::operator new(unsigned int, ...
            1555470 nsRuleNode::operator new(unsigned int, nsIPresContext *)
            1390870 nsStyleDisplay::operator new(unsigned int, nsIPresContext *)
            921760 nsRuleList::operator new(unsigned int, nsIPresContext *)
            609020 nsStylePosition::operator new(unsigned int, nsIPresContext *)
            489685 nsShellISupportsKey::operator new(unsigned int, ...
    585728 /builds/trunk/obj/debug/dist/bin/libxpcom.so+9585D
      585728 PL_HashTableRawAdd
        585728 nsHashtable::Put(nsHashKey *, void *)
          622496 nsRuleNode::Transition(nsIStyleRule *, nsRuleNode **)
            622496 nsRuleWalker::Forward(nsIStyleRule *)
  2116207 __builtin_new
    2016404 nsHTMLMappedAttributes::Clone(nsHTMLMappedAttributes **) const
      2016404 HTMLAttributesImpl::EnsureSingleMappedFor(nsIHTMLContent *, ...
        2016404 HTMLAttributesImpl::SetAttributeFor(nsIAtom *, ...
          2016404 HTMLStyleSheetImpl::SetAttributeFor(nsIAtom *, ...
            2016404 nsGenericHTMLElement::SetHTMLAttribute(nsIAtom *, ...
    1238368 HTMLAttribute::CopyHTMLAttributes(HTMLAttribute *, HTMLAttribute **)
      1238384 nsHTMLMappedAttributes::nsHTMLMappedAttributes(...
        1238384 nsHTMLMappedAttributes::Clone(nsHTMLMappedAttributes **) const
          1238384 HTMLAttributesImpl::EnsureSingleMappedFor(...
            1238384 HTMLAttributesImpl::SetAttributeFor(nsIAtom *, ...

(These numbers may be a bit skewed since my second log was truncated since my
disk filled up.)

Looks like rule tree growth, which is basically as-designed, since we want
reresolution to be fast, but perhaps we need to remove parts of the rule tree
when rules are removed?  I'm not really familiar with how the mapped attributes
stuff works, though.
Comment 6 Markus Gerstel 2001-12-29 08:29:11 PST
-- Does adding |var widthHeight;| in global scope help?

No



Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-12-29 08:41:21 PST
One possible solution (I think) would be to add a method |IsLive| to
|nsIStyleRule| and every time 1000 (or some large number) of rule nodes are
added to the rule tree, sweep the rule tree and remove all branches containing
nodes that are not live.  I *think* such a method could be implemented on
|nsHTMLMappedAttributes|, using its |mUseCount|.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-03-28 09:17:43 PST
The other thing such a solution would allow is keeping full rebuilds of the rule
tree rarer -- we could just do a sweep of the rule tree after a stylesheet
remove that would remove all rule nodes pointing to a rule node whose rule was
in a stylesheet that was no longer active.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-03-28 09:35:39 PST
...then again, that would cause a performance hit if a stylesheet were removed
and readded repeatedly.  But maybe that's rare enough that we don't care.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-04 15:59:23 PST
When we remove a rule node we'll need to clear the mapping in the style set
added for bug 99344.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-22 21:02:53 PDT
Does anybody really think this should be fixed for 1.0?  I never did, since it
involves pretty major changes.
Comment 12 Markus Hübner 2002-05-12 08:50:05 PDT
*** Bug 142528 has been marked as a duplicate of this bug. ***
Comment 13 jack.jia 2002-06-13 02:33:48 PDT
 I watch too many frames produced by the js.
   Edit-->Preferences-->Debug-->Event  and select show frames, clich OK

  Use the Reduced testcase, you will watch the frames' number become larger and
larger so fast.
Comment 14 jack.jia 2002-06-24 01:58:22 PDT
  This error caused by the javascript in the html file. The IE have the same 
result.

  -> won't fix
  
  I suggest to close it.
Comment 15 Markus Hübner 2002-06-24 04:09:37 PDT
Using MSIE6 the memory consumption remains exactly the same all over the 
animation!
Using trunk build 2002062308 the memory consumptions goes up rapidly - as 
already said abour 300kb/sec.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-06-24 09:02:47 PDT
This should be fixed at some point.
Comment 17 jack.jia 2002-07-04 02:42:45 PDT
   If it is a bug really,it caused by reflow process, because the frame number 
increase continuely and not decrease. The "reduced testcase" draw a rectangle 
ceaselessly, so it can produce many many frames and take much memory. who can 
tell me the life cycle of a frame? I want to release the frames in time. 

   With the reduced testcase, The IE will take 100% cpu, but I takes little 
mem, I think it has a good garbage collector. If i can release the mem fastly, 
mozilla can work as IE.

   David Baron: could you give me some suggestions?
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-07-04 12:42:08 PDT
This has nothing to do with reflow, it has to do with the rule tree.  I
understand what it is and I'm planning to fix it one of these days.
Comment 19 José Jeria 2002-07-06 14:59:25 PDT
Go to the URL and select 1 in the dropdown. The animation is soo awful that it
kind of reminds of bug 129953. Anyone else?

Using build 20020704 on Windows 2000
Comment 20 Markus Hübner 2002-07-12 11:09:42 PDT
"Go to the URL and select 1 in the dropdown"
where is that ?
Comment 21 José Jeria 2002-07-13 04:15:27 PDT
On this link:
http://www.formula-one.nu/dhtml/moz3d.htm

Select 1 in the select box, if you dont see what I mean, then bring up the
windows task manager and then close it. 
Comment 22 Markus Hübner 2002-07-14 06:11:41 PDT
I see heavy frame-drop offs on http://www.formula-one.nu/dhtml/3d.htm
see bug 157401.

Comment 23 Markus Hübner 2002-09-30 00:28:45 PDT
Bug 166298 seems also related to this.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-09-30 21:19:20 PDT
*** Bug 166298 has been marked as a duplicate of this bug. ***
Comment 25 Markus Hübner 2003-01-30 11:02:09 PST
The 3D Demo is now at 
http://www.world-direct.com/mozilla/dhtml/3dcube/index.htm
just 4 info :)

Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-03 19:46:56 PST
I hacked up a patch for this that I'll attach shortly.  I haven't attempted to
compile it yet, and I won't until I'm able to land some of the things I have in
my existing compiling trees and free up some build space.

Note that there is a circular dependency between this patch and bug 171830 (see
the XXX comment in this patch).  I believe there would be some crashes if this
patch lands before bug 171830 because of stale info in the rule mappings table.
 Likewise, bug 171830 needs to land at the same time as this one or else we'll
have significant memory increases in significantly more cases.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-03 19:48:28 PST
Created attachment 113465 [details] [diff] [review]
patch (untested)

Untested.  Not even compiled.  Don't try this at home.	It won't compile.  And
if it does it will crash.

Like I said in the previous comment, I'll finish this once I clean out some of
my trees.
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-04 06:33:09 PST
Created attachment 113496 [details] [diff] [review]
patch (untested)

Still untested, but a bunch of major errors now fixed.
Comment 29 Boris Zbarsky [:bz] 2003-02-04 11:32:52 PST
Before you test, add

mDependentBits &= ~NS_RULE_NODE_GC_MARK;

to Sweep() somewhere.

Looking pretty good otherwise...
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-04 11:37:19 PST
Created attachment 113514 [details] [diff] [review]
patch (untested)

Yep.  I even said I'd do that in nsRuleNode.h.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-04 11:39:39 PST
Created attachment 113515 [details] [diff] [review]
patch (untested)

Return too.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-11 16:08:26 PST
Created attachment 114148 [details] [diff] [review]
patch

This compiles, runs, and prevents memory use from increasing while displaying
http://www.world-direct.com/mozilla/dhtml/test.html
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-24 11:07:56 PST
Created attachment 115396 [details] [diff] [review]
patch

merged to tip
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-24 13:30:58 PST
Created attachment 115409 [details] [diff] [review]
patch

This allows for the fact that there can be multiple style contexts without a
parent.  I still saw one crash, but I can't reproduce it.  (I also have a
version merged with bz's patch to bug 171830.)
Comment 35 R.K.Aa. 2003-02-28 07:12:31 PST
*** Bug 195408 has been marked as a duplicate of this bug. ***
Comment 36 Boris Zbarsky [:bz] 2003-03-05 18:26:33 PST
David, are you happy enough with that patch that you want reviews for it?  Or
are you still testing?
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-03-05 19:02:35 PST
Well, I was playing around with a tree that had it and bug 171830 merged in a
single tree.  I crashed.  I looked at the stack quickly in the debugger, thought
I knew what was going on, quit the debugger, went to fix the code, and the
problem I thought was there didn't exist.  And I haven't been able to reproduce
the crash since.

I guess there's a chance the problem could be unrelated, but it seems unlikely.
 I guess you could review, though, and hopefully we'll find the crash later?
Comment 38 Boris Zbarsky [:bz] 2003-03-05 19:42:47 PST
Comment on attachment 115409 [details] [diff] [review]
patch

>+PRBool
>+nsRuleNode::Sweep()

OK, so Sweep() will Destroy() the rulenode and return PR_TRUE if it was
destroyed...

>+        if ((*children)->mRuleNode->Sweep()) {
>+          // This rule node was destroyed, so remove this entry, and
>+          // implicitly advance by making *children point to the next
>+          // entry.
>+          nsRuleList *todestroy = *children;
>+          *children = todestroy->mNext;
>+          todestroy->mNext = nsnull;
>+          todestroy->Destroy(mPresContext);

and this will call nsRuleList::Destroy which will turn around and call
Destroy() on its mRuleNode _again_ and then wipe out the rest of the list
(neither of which is desirable as far as I can tell).  Shouldn't that be a call
to DestroySelf instead?  Like:

  *children = (*children)->DestroySelf(mPresContext);

instead of those four lines of code?  (Now why didn't I read the definition of
Destroy() the forst time I read this patch?  <sigh>)

This seems like a reasonable candidate for that crash you were seeing.....


>+NS_IMETHODIMP
>+StyleSetImpl::NotifyStyleContextDestroyed(nsIPresContext* aPresContext,
>+                                          nsStyleContext* aStyleContext)
>+{

This should check mInShutdown and return without doing anything if that's set,
right?

Looks pretty good other than those two issues.
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-03-05 20:00:01 PST
Created attachment 116400 [details] [diff] [review]
patch

I haven't tested this yet, but it fixes the problems bz pointed out, plus a
missing return and the removal of the comment that won't be needed once bug
171830 lands.
Comment 40 Boris Zbarsky [:bz] 2003-03-05 20:38:26 PST
Comment on attachment 116400 [details] [diff] [review]
patch

Good catch on the return... (and on clearing mRoots when we start shutting
down).	r+sr=bzbarsky
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-03-06 14:56:05 PST
Fix checked in to trunk, 2003-03-06 11:14 PST (with bustage fix at 12:16 PST).

Note You need to log in before you can comment on or make changes to this bug.