Closed Bug 667085 Opened 13 years ago Closed 13 years ago

Improve about:memory reporter descriptions

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(2 files, 1 obsolete file)

Part 2, v1. Add KIND_PLACEHOLDER.
12.64 KB, patch
n.nethercote
: review-
Details | Diff | Splinter Review
Part 1, v2
22.03 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review 
Offhand, I think these can be improved:

- heap-used

>  Memory mapped by the heap allocator that is available for use by the 
>  application.  This may exceed the amount of memory requested by the 
>  application due to the allocator rounding up request sizes. 
>  (The exact amount requested is not measured.

This is the size of all allocated chunks, rounded up however the allocator does
that, right?

I don't like the current wording, since I think of an unallocated chunk that's
just sitting there, waiting for me to call malloc, as "available for use".

- heap-unused

>  Memory mapped by the heap allocator and not available for use by the 
>  application.  This can grow large if the heap allocator is holding onto 
>  memory that the application has freed.

- heap-dirty

> Memory mapped by the heap allocator that is committed but unused.

So heap-unused is memory mapped by the allocator that isn't part of a malloc'ed
chunk?  And heap-dirty is unused memory that shares a page with a malloc'ed
chunk, so it's committed?  I think we should be much more explicit about what
these mean, because it took me a long time to even come up with a guess.

- explicit

> The sum of all entries below "explicit".

This one has a real meaning; can we shim a description into about:memory
without creating a separate memory reporter for it?

- heap-unclassified

> Memory not classified by a more specific reporter. This includes memory
> allocated by the heap allocator in excess of that requested by the
> application; this can happen when the heap allocator rounds up request
> sizes.

We've been saying that the allocator "maps" memory from the OS and "allocates"
it to the application.  Do we mean memory *mapped* by the heap allocator in
excess of that requested by the app?  How is this part of heap-unclassified
different from heap-unused (which I thought didn't count as "explicit")?

- layout, layout/all, layout/bidi

It seems kind of silly that we have a reporter for "all" layout data that
doesn't actually contain all the layout data.

- images/{content,chrome}/{used,unused}

These four folders have meaning, but that meaning isn't revealed until we get
all the way down to {raw,unused}.
> Do we mean memory *mapped* by the heap allocator in
> excess of that requested by the app?  How is this part of heap-unclassified
> different from heap-unused (which I thought didn't count as "explicit")?

Oh, I'm just being slow today.  I understand that this is talking about internal fragmentation in the heap.  Maybe we should use that phrase.
(In reply to comment #0)
> Offhand, I think these can be improved:
> 
> - heap-used
> 
> >  Memory mapped by the heap allocator that is available for use by the 
> >  application.  This may exceed the amount of memory requested by the 
> >  application due to the allocator rounding up request sizes. 
> >  (The exact amount requested is not measured.
> 
> This is the size of all allocated chunks, rounded up however the allocator
> does
> that, right?

Yes.  Basically:

  heapUsed = 0
  for each malloc'd block B
    heapUsed += malloc_usable_size(B)

 
> I don't like the current wording, since I think of an unallocated chunk
> that's
> just sitting there, waiting for me to call malloc, as "available for use".

I don't -- the program shouldn't touch it until it's been handed over by malloc.


> - heap-unused
> 
> >  Memory mapped by the heap allocator and not available for use by the 
> >  application.  This can grow large if the heap allocator is holding onto 
> >  memory that the application has freed.
> 
> - heap-dirty
> 
> > Memory mapped by the heap allocator that is committed but unused.
> 
> So heap-unused is memory mapped by the allocator that isn't part of a
> malloc'ed
> chunk?  And heap-dirty is unused memory that shares a page with a malloc'ed
> chunk, so it's committed?  I think we should be much more explicit about what
> these mean, because it took me a long time to even come up with a guess.

Feel free to submit a patch.  That's probably the best way forward, it'll make the discussion more concrete.

 
> - explicit
> 
> > The sum of all entries below "explicit".
> 
> This one has a real meaning; can we shim a description into about:memory
> without creating a separate memory reporter for it?

It's awkward.  If you hover over "Explicit allocations" you get a description.


> - heap-unclassified
> 
> > Memory not classified by a more specific reporter. This includes memory
> > allocated by the heap allocator in excess of that requested by the
> > application; this can happen when the heap allocator rounds up request
> > sizes.
> 
> We've been saying that the allocator "maps" memory from the OS and
> "allocates"
> it to the application.  Do we mean memory *mapped* by the heap allocator in
> excess of that requested by the app?  How is this part of heap-unclassified
> different from heap-unused (which I thought didn't count as "explicit")?
> 
> - images/{content,chrome}/{used,unused}
> 
> These four folders have meaning, but that meaning isn't revealed until we get
> all the way down to {raw,unused}.

That's hard to avoid, because that's the level the reporters exist.  Doesn't seem like a big problem to me.
>> I don't like the current wording, since I think of an unallocated chunk that's
>> just sitting there, waiting for me to call malloc, as "available for use".

> I don't -- the program shouldn't touch it until it's been handed over by malloc.

My point is that there are multiple ways of interpreting the current wording, not that your interpretation is necessarily invalid.  It depends on the point of view: From the point of view of the allocator (which I think is at least a reasonable pov here), memory is "in use" as soon as the program calls malloc, so if it's "available for use", it's "available to be handed over by malloc".

I'm still not sure what heap-unused and heap-dirty are, but I'll have a look at the jemalloc source and see if I can understand.
> It's awkward.  If you hover over "Explicit allocations" you get a description.

I see.  In that description:

  "[it's] the single best number to focus on when trying to reduce memory usage"

s/single best/best single, as in the description of RSS.

I'll spin a patch with all this and whatever I can figure out about heap-{dirty,unused}.
How about, for shimming in a description for "explicit" into the tree, we add KIND_PLACEHOLDER?  When building the tree, if we see a memory reporter with KIND_PLACEHOLDER, we place its description into the tree but calculate its amount as though the reporter weren't there.

Is that too much of a hack?
Watching [1] it seems that you get a dirty page when:

* Application calls malloc, and jemalloc maps memory from the OS, then
* application writes to the allocated page, then
* application calls free.

The page is dirty until jemalloc returns this page to the OS.

I'm still trying to figure out what mechanism jemalloc uses to track which pages are written by the application.

[1] http://www.facebook.com/video/video.php?v=696488619305&oid=9445547199&comments&ref=mf
> * application writes to the allocated page, then

This is necessary because mmap allocates physical pages to the virtual mapping lazily.  So if the application never modifies the mmap'ed pages, when we free, only those pages which we touched are taking up physical memory.
Looking at the code, my best guess is that a page is considered dirty once the application *may* have modified it -- that is, it's dirty as soon as it's malloc'ed.
Moving on to the Mac stats: Josh, do you know if struct mstats (in malloc/malloc.h) is documented anywhere?  (I guess this won't matter so much once we have jemalloc on mac, but that's been RSN for a while now...)
I don't know.

        Attachment #542261 -
        Flags: review?(nnethercote)

    
    

    
  


  
This doesn't feel like as much of a hack as I thought it would, for whatever that's worth.  :)

        Attachment #542270 -
        Flags: review?(nnethercote)

    
    

    
  
(In reply to comment #10)
> I don't know.

Okay, well I'll just guess for now.  That's no worse than what we were apparently doing before.

    
    

    
  
Comment on attachment 542261 [details] [diff] [review]
Part 1, v1 - Update existing descriptions.

Review of attachment 542261 [details] [diff] [review]:
-----------------------------------------------------------------

All the changes are fine except for replacing heap-unused with heap-mapped, which I don't like.  r=me with that removed.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +412,5 @@
>        _kind: KIND_HEAP,
>        _description:
>          "Memory not classified by a more specific reporter. This includes " +
> +        "waste due to internal fragmentation in the heap allocator (caused when " +
> +        "the allocator rounds up request sizes).",

It's not necessarily waste;  if you use malloc_usable_size you can use all of the space.  But given how often malloc_usable_size is used in the Mozilla codebase (I just checked on MXR) it's close enough to true that I'm happy, and your version is more direct.  So:  sounds good.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +258,5 @@
>  #endif  // MOZ_MEMORY
>  
>  #if HAVE_JEMALLOC_STATS
>  
> +static PRInt64 GetHeapMapped()

You're removed heap-unused and replaced it with heap-mapped.  I did the exact opposite when I first revamped about:memory because I think heap-unused is a more interesting number than heap-mapped.

        Attachment #542261 -
        Flags: review?(nnethercote) → review+

    
    

    
  
Comment on attachment 542270 [details] [diff] [review]
Part 2, v1.  Add KIND_PLACEHOLDER.

Review of attachment 542270 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't like this.  Too much hackery to fix what I consider a very small problem.

        Attachment #542270 -
        Flags: review?(nnethercote) → review-

    
    

    
  
(In reply to comment #14)
> You're removed heap-unused and replaced it with heap-mapped.  I did the
> exact opposite when I first revamped about:memory because I think
> heap-unused is a more interesting number than heap-mapped.

jemalloc measures three quantities:

 * heap-mapped -- bytes malloc has mapped from the OS
 * heap-allocated -- mapped bytes part of an active allocation, quantized (i.e, rounded up)
 * heap-dirty -- mapped bytes once part of an active allocation but now not part of an active allocation.  The allocator conservatively assumes these pages are committed.

What's interesting about the quantity

  mapped - (allocated + dirty)

?  That seems about as useful as vsize - committed and seems kind of out of place with the rest of the reporters, which either report virtual memory (committed plus uncommitted) or physical memory.  heap-unused (maybe heap-uncommitted, or an-upper-bound-on-heap-committed would be a better name, at least on jemalloc where we have a bound on the dirty size) is vmem only uncommitted.

I guess the mapped quantity isn't particularly interesting in itself either, but at least I can compare it to vsize to see how much of the vsize is due to the allocator.

    
    

    
  
(In reply to comment #15)
> Comment on attachment 542270 [details] [diff] [review] [review]
> Part 2, v1.  Add KIND_PLACEHOLDER.
> 
> Review of attachment 542270 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I don't like this.  Too much hackery to fix what I consider a very
> small problem.

FWIW I never would have found the tooltip hiding on "explicit allocations".  And in case it wasn't clear, the idea was to have more of these placeholders, filling in other parts of the tree.  For instance, there are a bunch of "X.sqlite" allocators.  I think it would be nice to say "Memory used by X.sqlite.  Each SQLite database has a separate cache, statement pool, and whatnot..."

Similarly, I think it would be nice to describe what "used" and "unused" images are.  (If the image isn't used, why are we storing it?)  But right now we have to force the description all the way down...

    
    

    
  
Okay, better question (sorry, it's late here):

Is there some mechanism for adding these descriptions that you'd consider r+'ing?  We could have a long list in aboutmemory.js, but I think it's much better to keep the placeholder reporters next to the real reporters in the code.

    
    

    
  
Also, is heap-allocated + heap-dirty different than heap-committed?  Should heap-unused be heap-mapped - heap-committed?  I really don't know...

    
    

    
  
Do you mind if I ignore this bug for a week?  FF7 development ends in one week.  You want to finish bug 664486 before then, maybe also bug 664291.  I want to finish bug 666075, bug 661474, bug 653627, bug 660731 and bug 663423.  All of those bugs are more important than this one, IMO.

Sound reasonable?

    
    

    
  
> Sound reasonable?

Sounds good to me!

    
    

    
  
(In reply to comment #17)
> 
> FWIW I never would have found the tooltip hiding on "explicit allocations". 
> And in case it wasn't clear, the idea was to have more of these
> placeholders, filling in other parts of the tree.  For instance, there are a
> bunch of "X.sqlite" allocators.  I think it would be nice to say "Memory
> used by X.sqlite.  Each SQLite database has a separate cache, statement
> pool, and whatnot..."
> 
> Similarly, I think it would be nice to describe what "used" and "unused"
> images are.  (If the image isn't used, why are we storing it?)  But right
> now we have to force the description all the way down...

I thought some more about this.  I'm sympathetic to moving the description of "explicit" away from the heading;  that is hard to find.  I'd be happy if that was added with a single special case in aboutMemory.js.

The ones like "used" and "unused" in images I'm less sympathetic to.  It's not hard to find the meanings.  And for the ones like SQLite, it's obvious that the sub-reporters contribute to the parent reporter's total.  I don't want to clog up nsIMemoryReporter with hacks just for those.

    
    

    
  
I guess part of what I find objectionable about the current setup is that there's no way to tell when a reporter has an interesting description or when it has a boring "sum of children" description.  So if I want to find out what "used" means, I have to hover over it first, and then over "raw".  Surely this problem won't get any better as the hierarchy of reported objects gets more complex.

I think it might be improved if we visually indicated what has and what doesn't have tooltips.  But that's also somewhat orthogonal to the main thrust of adding tooltips to arbitrary paths.

I'm not quite ready to give up on this.  What if nsIMemoryReporterManager had a function associateTooltip(path, description) and a way to enumerate these descriptions?

If the objection is to hackery more than the functionality, I think we can do this in a way which is both clean and functional.  But of course it's going to add *some* amount of complexity.

    
    

    
  
(In reply to comment #23)
> I guess part of what I find objectionable about the current setup is that
> there's no way to tell when a reporter has an interesting description or
> when it has a boring "sum of children" description.

The rule is that all non-child nodes have boring descriptions, with the exception of explicit/storage/sqlite.  It's an annoying exception, I'd love to get rid of it.  It would require having a global list of all current SQLite connections.

    
    

    
  
njn, ping on this bug, now that we branched.

The two outstanding places of disagreement here are:

1) Replacing heap-unused with heap-mapped.

2) Adding the ability to associate descriptions with arbitrary reporters.

Let's worry about (2) in a separate bug.

With respect to (1), heap-mapped matches vsize in that it's all of virtual memory, not just uncommitted virtual memory.  heap-mapped is also directly measurable from jemalloc, while I'm not sure heap-unused is calculated correctly (and I'm even less certain on mac).  At best, it's a bound on the uncommitted heap.

heap-mapped is also a simpler concept to understand than heap-unused, I think.

    
    

    
  
(In reply to comment #25)
> 
> With respect to (1), heap-mapped matches vsize in that it's all of virtual
> memory, not just uncommitted virtual memory.  heap-mapped is also directly
> measurable from jemalloc, while I'm not sure heap-unused is calculated
> correctly (and I'm even less certain on mac).

heap-unused = heap-mapped - heap-used.  There's no uncertainty.

I like showing two numbers that don't overlap.  There's no right answer, it's just a preference.

> heap-mapped is also a simpler concept to understand than heap-unused, I
> think.

I think they're equally simple.

Getting back to an earlier point:  after some thought, I'm now more sympathetic to the notion of visually distinguishing numbers that are directly measured vs those that are the result of combining other numbers.

    
    

    
  
> heap-unused = heap-mapped - heap-used.  There's no uncertainty.

Does heap-unused include heap-dirty?  I am not certain.  The allocator isn't using it (it's not part of heap-used -- or is it?), but it's memory being used, in contrast to heap-unused.

I think I'd be OK if we had "heap-uncommitted" and called heap-used "heap-active" (no dirty pages).  I'd rather we had an explicit "heap-committed" field to accompany heap-uncommitted, but I think we want to report active and dirty separately.

We can explain in the description of heap-uncommitted that the committed pages are heap-active plus heap-dirty, and since heap-dirty has been really small (<2mb), that shouldn't require people to do heavy lifting to compute heap-committed.

What do you think?

    
    

    
  
Oh, we already have heap-committed, as a separate measurement from heap-active and heap-dirty.  Even better.

    
    

    
  
Although jemalloc.c has at the end of jemalloc_stats():

  #ifndef MALLOC_DECOMMIT
    stats->committed = stats->mapped;
  #endif

MALLOC_DECOMMIT is not defined on Linux, although maybe it should be.

    
    

    
  
> heap-unused = heap-mapped - heap-used.  There's no uncertainty.

Or is heap-unused (aka heap-uncommitted?) mapped - committed?  I don't believe that the committed figure jemalloc reports is equal to used + dirty.

    
    

    
  
heap-uncommitted is always 0 on Linux, presumably because there's no MALLOC_DECOMMIT (see bug 669877).

    
    

    
  
Here's a completely different reason to prefer heap-mapped over heap-uncommitted:

From bug 669877 comment 2, it seems that the heap-committed figure is not totally correct when MALLOC_DECOMMIT is defined.  It might be totally not correct.

If we report heap-uncommitted (mapped - committed), then that figure is also not meaningful.  But if we report heap-mapped, then errors don't propagate.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1, v2
        — 
          — Splinter Review
      

    


  
I renamed heap-used to heap-allocated and renamed heap-unused to heap-unallocated, which is now mapped - allocated.  (Before, it was [mapped - allocated] on Mac and [mapped - committed] on jemalloc.)

I'd still prefer to report just plain heap-mapped instead of [mapped - allocated], but I'd also not like to block this patch on this one issue.

    
  

        Attachment #542261 -
        Attachment is obsolete: true

    
  

        Attachment #545181 -
        Flags: review?(nnethercote)

    
    

    
  
Comment on attachment 545181 [details] [diff] [review]
Part 1, v2

Review of attachment 545181 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
@@ +70,5 @@
>    }
>  
>    var fakeReporters = [
> +    f("", "heap-allocated",          OTHER,  500 * MB),
> +    f("", "heap-unallocated",        OTHER,  100 * MB),

Indentation.

@@ +124,5 @@
>    dummy = mgr.resident;
>  
>    var fakeReporters2 = [
> +    f("2nd", "heap-allocated",       OTHER, 1000 * MB),
> +    f("2nd", "heap-unallocated",     OTHER,  100 * MB),

Indentation

@@ +141,2 @@
>      // reporters, leaf-reporters, and "other" reporters.
> +    f("3rd", "heap-allocated",       OTHER, kUnknown),

Indentation.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +402,5 @@
> +    "Memory mapped by the heap allocator that is not part of an active "
> +    "allocation. Much of this memory may be uncommitted -- that is, it does not "
> +    "take up space in physical memory or in the swap file. Committed and "
> +    "unallocated memory, perhaps a result of fragmentation, is reported in "
> +    "heap-dirty, if that measure is available.")

Single quotes around 'heap-dirty' to match other descriptions.

@@ +404,5 @@
> +    "take up space in physical memory or in the swap file. Committed and "
> +    "unallocated memory, perhaps a result of fragmentation, is reported in "
> +    "heap-dirty, if that measure is available.")
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(HeapAllocated,

You swapped the order of HeapAllocated and HeapUnallocated unnecessarily, please swap them back so the match the order in which they are registered.

        Attachment #545181 -
        Flags: review?(nnethercote) → review+

    
    

    
  
> Indentation.

When I went through that file, I was wondering why the heck the indention was so bad.  Only now did I realize it was my fault.  :)

    
    

    
  
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
Whiteboard: [inbound]

    
    

    
  
And repushed to inbound. http://hg.mozilla.org/integration/mozilla-inbound/rev/3c675415362d
Whiteboard: [inbound]

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3c675415362d
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

    
  
Assignee: nobody → justin.lebar+bug
Component: General → about:memory
OS: Linux → All
QA Contact: general → about.memory
Hardware: x86_64 → All
Target Milestone: --- → mozilla8

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: