Closed Bug 663271 Opened 9 years ago Closed 8 years ago

Add memory reporters for the DOM

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: mounir)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

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).
Blocks: DarkMatter
No longer blocks: MemShrinkTools
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.
Whiteboard: MemShrink:P1 → [MemShrink:P1]
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
That's missing most of the memory (e.g. the memory needed by attributes, child lists, text in textnodes, etc, etc)...
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.
Assignee: nobody → mounir.lamouri
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.
Duplicate of this bug: 340372
For the first step, do we want to mix content and chrome or just have content?
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.
(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...
(In reply to comment #9)
> And it looks like the bug number your pasted isn't correct...

Sorry, I meant bug 666075.
Depends on: 667468
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).
Status: NEW → ASSIGNED
Depends on: 667470
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...
Depends on: 667474
Depends on: 667485
Depends on: 667497
(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.
Depends on: 667883
Depends on: 667887
Depends on: 668013
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.
Depends on: 669308
Depends on: 669330
Depends on: 669886
Depends on: 669904
Depends on: 670962
Depends on: 670965
Depends on: 672539
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.
> 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.
Depends on: 674112
Depends on: 674113
Depends on: 674114
Depends on: 674115
Depends on: 674116
Depends on: 677503
Depends on: 677504
Depends on: 677506
(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.
> 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.
Depends on: 679499
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.)
Depends on: 682264
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.
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
Depends on: 684698
This is pretty fixed.  We can file new bugs for followups that we think are important as needed.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(and by "think are important", I mean, where we have some specific evidence they would account for enough memory to be useful)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.