Last Comment Bug 663271 - Add memory reporters for the DOM
: Add memory reporters for the DOM
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
: 340372 (view as bug list)
Depends on: 681199 682432 684698 667468 667470 667474 667485 667497 667883 667887 668013 669308 669330 669886 669904 670962 670965 672539 674112 674113 674114 674115 674116 677503 677504 677506 679499 682264 682435
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-06-09 16:50 PDT by Nicholas Nethercote [:njn]
Modified: 2012-04-17 17:41 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
estimate DOM memory usage using cycle collector infrastructure (4.02 KB, patch)
2011-06-15 18:18 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-06-09 16:50:28 PDT
There are no memory reporters for the DOM.  There should be, because DOM structures can (presumably) grow quite large.

jst told me there used to be a SizeOf method on DOM objects, but it was never very reliable.  Hopefully now that about:memory is getting more usage, any feature like that will get more use and so be less likely to rot.

FWIW, there are two basic models for implementing memory reporters:

- The first is to have some global counter, and to increment/decrement it every time an allocation/deallocation of the appropriate kind happens.  

- The second is to have some way of computing the size from scratch on demand, eg. by iterating over all relevant structures.

I've been tending towards using the second kind in the JS engine, even though it make about:memory a little slower, because they seem less error-prone -- we had some counters of the first kind that ended up reporting ridiculous numbers because the code changed over time and the increment/decrement points weren't added/removed/changed as necessary (see bug 648490).
Comment 1 Boris Zbarsky [:bz] 2011-06-09 16:56:11 PDT
We want to do the second, I think.

One issue with SizeOf is that it tended to not get updated when members got added...  Maybe we'll do better this time.
Comment 2 Andrew McCreight [:mccr8] 2011-06-15 18:18:52 PDT
Created attachment 539700 [details] [diff] [review]
estimate DOM memory usage using cycle collector infrastructure

The cycle collector actually gets the size of the object corresponding to each object in its graph (well, more or less...).  We can just get the total size of all of the ref counted objects it sees as a very rough estimate of the memory used by the DOM.  I haven't tried this patch out yet.
Comment 3 Boris Zbarsky [:bz] 2011-06-15 18:28:29 PDT
That's missing most of the memory (e.g. the memory needed by attributes, child lists, text in textnodes, etc, etc)...
Comment 4 Andrew McCreight [:mccr8] 2011-06-17 14:47:03 PDT
Yeah, I guess that is true.  Ah well.  Anyways, when I ran this, it was reporting 2.5megs to 5.5megs for browsing around.

This sounds like the cycle collector problem, where fields have to be added to some magic function, but people forget to update it.
Comment 5 Nicholas Nethercote [:njn] 2011-06-21 16:42:04 PDT
FWIW:  just a single "explicit/dom" reporter would be a great start here.  That would make it obvious whether we need sub-reporters to give more information.  If we do need sub-reporters, the multi-reporter interface (bug 666075) might make them a lot easier.
Comment 6 Nicholas Nethercote [:njn] 2011-06-24 04:13:51 PDT
*** Bug 340372 has been marked as a duplicate of this bug. ***
Comment 7 Mounir Lamouri (:mounir) 2011-06-25 06:02:47 PDT
For the first step, do we want to mix content and chrome or just have content?
Comment 8 Nicholas Nethercote [:njn] 2011-06-25 14:10:38 PDT
My first suggestion is to get as much as possible included in the measurement.  So chrome and content should be measured.  But then, if it's easy to measure them separately (and take a look again at bug 666705) that's probably worth trying.
Comment 9 Mounir Lamouri (:mounir) 2011-06-25 14:16:27 PDT
(In reply to comment #8)
> My first suggestion is to get as much as possible included in the
> measurement.  So chrome and content should be measured.  But then, if it's
> easy to measure them separately (and take a look again at bug 666705) that's
> probably worth trying.

I splitting them should not be a problem. And it looks like the bug number your pasted isn't correct...
Comment 10 Nicholas Nethercote [:njn] 2011-06-25 14:53:40 PDT
(In reply to comment #9)
> And it looks like the bug number your pasted isn't correct...

Sorry, I meant bug 666075.
Comment 11 Mounir Lamouri (:mounir) 2011-06-27 08:32:39 PDT
If no one object, I'm going to use this bug as a tracker. Splitting the patches should make everything easier (from writing the code to the reviews) and hopefully, will let me push different patches without everything being finished (behind a compilation flag or a preference option).
Comment 12 Boris Zbarsky [:bz] 2011-06-27 08:41:56 PDT
I think making this a tracker is fine.  I'd just push things as they come online; the reporter will just end up being more and more accurate...
Comment 13 Nicholas Nethercote [:njn] 2011-06-27 18:16:17 PDT
(In reply to comment #12)
> I think making this a tracker is fine.  I'd just push things as they come
> online; the reporter will just end up being more and more accurate...

bz++.  That's what I've been doing with the about:memory changes in general.
Comment 14 Nicholas Nethercote [:njn] 2011-07-04 18:36:35 PDT
In bug 669117 comment 5 bz suggested this:

> There's a network-related one that's really DOM: XHR.  The data retrieved by
> XHR is stored in memory in many cases; we should add an XHR memory reporter.
Comment 15 Nicholas Nethercote [:njn] 2011-07-24 16:19:53 PDT
A commenter on my blog (Ahmad) noticed that this URL:

  http://www.w3.org/2000/10/swap/test/plist/iTunesMusicLibrary.xml

results in heap-unclassified going over 60%, and I can reproduce it.  Is this something that the DOM reporters will cover?  Or is it due to bug 291643?  I'm not sure if it deserves its own bug.
Comment 16 Boris Zbarsky [:bz] 2011-07-24 19:47:46 PDT
> Is this something that the DOM reporters will cover? Or is it due to
> bug 291643?

"Yes."  We have a pretty good idea of what's up there -- lots of DOM nodes.
Comment 17 Nicholas Nethercote [:njn] 2011-08-12 00:12:40 PDT
(In reply to Boris Zbarsky (:bz) from comment #16)
> 
> "Yes."  We have a pretty good idea of what's up there -- lots of DOM nodes.

With DOM reporters enabled, here's what I get:

293.10 MB (100.0%) -- explicit
├──160.33 MB (54.70%) -- heap-unclassified
├───81.16 MB (27.69%) -- layout
│   ├──80.55 MB (27.48%) -- arenas
│   └───0.61 MB (00.21%) -- (1 omitted)
├───23.96 MB (08.18%) -- js
│   ├──11.14 MB (03.80%) -- compartment([System Principal], 0x7fbc24d5a000)
│   │  ├───6.29 MB (02.15%) -- gc-heap
│   │  │   ├──2.97 MB (01.01%) -- objects
│   │  │   ├──2.09 MB (00.71%) -- shapes
│   │  │   └──1.23 MB (00.42%) -- (5 omitted)
│   │  ├───2.69 MB (00.92%) -- (7 omitted)
│   │  └───2.16 MB (00.74%) -- scripts
│   ├───8.00 MB (02.73%) -- stack
│   ├───2.64 MB (00.90%) -- (7 omitted)
│   └───2.18 MB (00.74%) -- gc-heap-chunk-dirty-unused
├───21.50 MB (07.34%) -- dom
├────4.75 MB (01.62%) -- storage
│    └──4.75 MB (01.62%) -- sqlite
│       ├──2.42 MB (00.83%) -- (10 omitted)
│       └──2.33 MB (00.79%) -- places.sqlite
│          ├──2.02 MB (00.69%) -- cache-used [4]
│          └──0.31 MB (00.10%) -- (2 omitted)
└────1.39 MB (00.48%) -- (3 omitted)

So, DOM isn't much of it -- only 21.5MB.  And that's after I fixed the nsPresArena waste in bug 678422.  I'll have to run DMD on this one, but I'm out of time this week.  It may end up being similar to the issues in bug 678376 comment 13.
Comment 18 Boris Zbarsky [:bz] 2011-08-12 06:36:29 PDT
> So, DOM isn't much of it -- only 21.5MB.

The DOM reporter only reports explicit (non-anonymous) DOM elements at the moment.  Most of the DOM in the XML-viewer case is anonymous XBL-bound stuff.  We need to add that to the DOM reporter....  In particular, we should consider using nsChildIterator in nsIDocument::SizeOf.  And even then, I'm not sure it would hit all the nodes.
Comment 19 Nicholas Nethercote [:njn] 2011-08-18 00:14:50 PDT
DMD shows this DOM memory as important:

==31189== Unreported: 9,644,518 (cumulative: 32,915,270) bytes in 3,609 heap block(s) in record 3 of 11524:
==31189==  Requested bytes unreported: 8,798,046 / 8,798,046
==31189==  Slop      bytes unreported: 846,472 / 846,472
==31189==    at 0x4029F90: malloc (vg_replace_malloc.c:235)
==31189==    by 0x403B090: moz_malloc (mozalloc.cpp:120)
==31189==    by 0x785A5C6: NS_Alloc_P (nsMemoryImpl.cpp:198)
==31189==    by 0x662FA60: nsMemory::Alloc(unsigned long) (nsMemory.h:68)
==31189==    by 0x6BAA629: nsTextFragment::SetTo(unsigned short const*, int) (nsTextFragment.cpp:282)
==31189==    by 0x6B695F3: nsGenericDOMDataNode::SetTextInternal(unsigned int, unsigned int, unsigned short const*, unsigned int,
int, CharacterDataChangeInfo::Details*) (nsGenericDOMDataNode.cpp:342)

volkmar, is that covered by existing reporters?  (I haven't taught DMD about DOM reporters yet.)
Comment 20 Nicholas Nethercote [:njn] 2011-08-28 17:45:55 PDT
I'm downgrading this to MemShrink:P2, because a decent chunk of the DOM is covered now.  CSS stuff is a bigger fraction of the dark matter now.
Comment 21 Andrew McCreight [:mccr8] 2012-04-17 17:40:54 PDT
This is pretty fixed.  We can file new bugs for followups that we think are important as needed.
Comment 22 Andrew McCreight [:mccr8] 2012-04-17 17:41:29 PDT
(and by "think are important", I mean, where we have some specific evidence they would account for enough memory to be useful)

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