├────117,604,456 B (08.81%) -- layout │ └──117,604,456 B (08.81%) -- all It would be very nice if I could gain some insight on how much memory which part of layout is using...

Should the title of this bug be something like "Split up the memory reporters for layout"?

No, this isn't a duplicate. Bug 6783149 is splitting per-site. This bug is about splitting by the type of thing being allocated from the arena.

Here's what I see today: ├───8.45 MB (01.75%) -- layout │ ├──6.41 MB (01.33%) ── style-sets [4] │ └──2.04 MB (00.42%) -- (2 tiny) │ ├──2.04 MB (00.42%) ── arenas [4] │ └──0.00 MB (00.00%) ── text-runs Is this a useful amount of granularity, or would you like more?

This series of patches splits out some of the work I did in bug 686795 to provide better layout memory reporters. The first patch makes the list of frame types reusable by other parts of the browser, forthcoming in the next patch.

This second part exposes information about each frame type allocated to about:memory. Doing this for all frame types might be a little verbose, but about:memory will take care of hiding things that we don't want to see. Nicholas, is accounting this way going to cause problems with DMD? I'm not at a machine for running DMD at the moment.

This patch reports on the most common types of objects found in the PresShell's arena, per the bug 686795 case. More object types are certainly possible, and nsIPresShell says we shouldn't be using AllocateMisc anyway, so this is a win from that perspective. But this is enough to expose problems with style context sharing.

(In reply to Nathan Froyd (:froydnj) from comment #6) > Nicholas, is accounting this way going to cause problems with DMD? I'm not > at a machine for running DMD at the moment. I guess so, because we're no longer going to go through functions generated from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN*, yes? So we'd have to retain the loop to calculate memory used by the arena pools held by nsPresArena and throw away the value computed?

Comment on attachment 629489 [details] [diff] [review] part 2 - report on individual frame types in about:memory Review of attachment 629489 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with having more layout reporters. Heaven knows we have enough JS ones :) Your concern about DMD is well-founded: I've been moving away from computing memory consumption and using malloc_usable_size because (a) it's less error-prone, and (b) it integrates with DMD. What I'd suggest is to continue using malloc_usable_size for the "explicit" measurements, but consider reporting some more detailed computed sizes in the "Other Measurements" section. You couldn't do per-window measurements, but you could get global totals, which will probably still be pretty useful. Note that I'm going to allow trees in "Other Measurements" in bug 760352, which might be useful for this purpose.

Comment on attachment 629490 [details] [diff] [review] part 3 - report on specific objects in arenas Review of attachment 629490 [details] [diff] [review]: ----------------------------------------------------------------- f- for the same reason as part 2.

(In reply to Nicholas Nethercote [:njn] from comment #9) > Your concern about DMD is well-founded: I've been moving away from > computing memory consumption and using malloc_usable_size because (a) it's > less error-prone, and (b) it integrates with DMD. > > What I'd suggest is to continue using malloc_usable_size for the "explicit" > measurements, but consider reporting some more detailed computed sizes in > the "Other Measurements" section. You couldn't do per-window measurements, > but you could get global totals, which will probably still be pretty useful. Agreed that computing memory consumption is tedious and error-prone. But we're not using malloc to hand out pieces of memory with nsPresArena; we're using malloc to carve out a huge chunk of memory and handing out bits from that. malloc_usable_size doesn't give us enough insight in a situation like that. (It's possible that jemalloc obviates the need for nsPresArena, but that is an entirely separate discussion.) Comment 8, I think, addresses the way to keep DMD happy. As for global statistics, I'm skeptical. Saying "we need to provide more detail on this per-window category" and then "but let's not provide the detail on a per-window basis" doesn't seem like the right direction. Per-window statistics are the most useful thing about about:memory, IMHO. But maybe that's a moot point if malloc_usable_size vs. explicit computation is ironed out.

> Agreed that computing memory consumption is tedious and error-prone. But > we're not using malloc to hand out pieces of memory with nsPresArena; we're > using malloc to carve out a huge chunk of memory and handing out bits from > that. malloc_usable_size doesn't give us enough insight in a situation like > that. (It's possible that jemalloc obviates the need for nsPresArena, but > that is an entirely separate discussion.) Comment 8, I think, addresses the > way to keep DMD happy. I *really* don't like the idea of taking one set of measurements and throwing it away to keep DMD happy, while taking a second set of measurements that we actually show in about:memory. > As for global statistics, I'm skeptical. Saying "we need to provide more > detail on this per-window category" and then "but let's not provide the > detail on a per-window basis" doesn't seem like the right direction. > Per-window statistics are the most useful thing about about:memory, IMHO. Sure. But if you only have one window open, the global stats and the per-window stats are basically the same. For cases like bug 686795 that's good enough.

(In reply to Nicholas Nethercote [:njn] from comment #12) > > Agreed that computing memory consumption is tedious and error-prone. But > > we're not using malloc to hand out pieces of memory with nsPresArena; we're > > using malloc to carve out a huge chunk of memory and handing out bits from > > that. malloc_usable_size doesn't give us enough insight in a situation like > > that. (It's possible that jemalloc obviates the need for nsPresArena, but > > that is an entirely separate discussion.) Comment 8, I think, addresses the > > way to keep DMD happy. > > I *really* don't like the idea of taking one set of measurements and > throwing it away to keep DMD happy, while taking a second set of > measurements that we actually show in about:memory. I know, it's ugly. I suppose we could explore changing nsPresArena to be more about:memory-friendly, but other than that, I'm at a loss to see a way forward to keep the One True Way of computing memory statistics and simultaneously provide finer-grained information. > > As for global statistics, I'm skeptical. Saying "we need to provide more > > detail on this per-window category" and then "but let's not provide the > > detail on a per-window basis" doesn't seem like the right direction. > > Per-window statistics are the most useful thing about about:memory, IMHO. > > Sure. But if you only have one window open, the global stats and the > per-window stats are basically the same. Granted. That some # of statistics won't be that useful unless you only have one window open seems...not useful (or discoverable), though. ;) > For cases like bug 686795 that's good enough. For that particular instance, it's good enough. But in a post-this-bug-is-fixed world, it doesn't seem worthwhile for a developer (or informed user) to discover a page that's sucking memory and require a restart of Firefox, dropping the current session, and looking at that page in isolation to get good information for a bug report or as a start to fixing the bug. Likewise, a side-by-side comparison of bug 686795-page vs. bug 686795-fixed-page requires separate profiles or separate computers or...ugh.

Without taking sides on the debate about implementation, and without begging the question as to whether we can do this sanely without rejiggering how layout allocates its things: It stm that if fine-grained layout reporters are useful, then we should strive to present them on a per-tab (read per presshell, or whatever) basis, not just as an aggregate in "other measurements".

> > I *really* don't like the idea of taking one set of measurements and > > throwing it away to keep DMD happy, while taking a second set of > > measurements that we actually show in about:memory. > > I know, it's ugly. I suppose we could explore changing nsPresArena to be > more about:memory-friendly, but other than that, I'm at a loss to see a way > forward to keep the One True Way of computing memory statistics and > simultaneously provide finer-grained information. If you can find a way to assert that your computed size(s) matches the malloc_usable_size, and you write a very clear comment explaining what's going on, then you might be able to convince me. One difficulty there is that computed sizes don't account for slop bytes whereas malloc_usable_size does. But then, nsPresArena should be using power-of-two sized blocks with no slop, so maybe this won't be a problem.

(In reply to Nicholas Nethercote [:njn] from comment #15) > One difficulty there is that computed sizes don't account for slop bytes > whereas malloc_usable_size does. But then, nsPresArena should be using > power-of-two sized blocks with no slop, so maybe this won't be a problem. You're right, this is a sticky situation. There's going to be slop from the allocated objects not fitting into the arena compactly. (The allocated objects are not all power-of-two size bytes.) That slop should be counted in about:memory, but it wouldn't be correctly counted by a computed-size approach. (We could account for that in a layout-arena-slop category, with malloc_usable_size() - computed_size, but that feels like piling a hack on top of a hack.)

Comment on attachment 629488 [details] [diff] [review] part 1 - make frame id lists for reuse Review of attachment 629488 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrameIdList.h @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +FRAME_ID_DESC(BRFrame) Just call the macro FRAME_ID I think.

The basic idea of reporting object types from nsPresArena sounds fine to me. (In reply to Nathan Froyd (:froydnj) from comment #16) > (We could account for that in a layout-arena-slop category, with > malloc_usable_size() - computed_size, but that feels like piling a hack on > top of a hack.) That sounds reasonable to me actually.

> > (We could account for that in a layout-arena-slop category, with > > malloc_usable_size() - computed_size, but that feels like piling a hack on > > top of a hack.) > > That sounds reasonable to me actually. Me too :)

OK, we seem to have consensus, hooray! We should just go ahead and count all of nsPresArena::State. I initially thought we needed to do this to avoid wacky "negative" size values, but have since convinced myself that won't happen. Still, if we're going the "compute malloc size, then subtract" route, I think it's in our best interest to make the malloc size as accurate as possible.

Just the frame types. I've elected to not provide separate sub-categories for each of the frame types, instead gathering them in a window-objects-layout-frames category.

Comment on attachment 630213 [details] [diff] [review] part 1a - account for all of nsPresArena::State Review of attachment 630213 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresArena.cpp @@ +365,5 @@ > while (arena) { > n += aMallocSizeOf(arena); > arena = arena->next; > } > + return n + mFreeLists.SizeOfExcludingThis(SizeOfFreeListEntryExcludingThis, Nit: can you do this? n += mFreeLists.SizeOf... return n; That's the style used across most SizeOf functions.

Comment on attachment 630215 [details] [diff] [review] part 2 - report on individual frame types in about:memory Review of attachment 630215 [details] [diff] [review]: ----------------------------------------------------------------- IIUC, you've increased the maximum number of entries in about:memory per window by 168, i.e. the number of entries in nsFrameIdList.h? That is... rather a lot. What does about:memory look like in practice now -- can you post sample output for some complex pages? I don't want each window to be bloated with heaps of tiny entries. (If there is bloat, you could use the "sundries" approach that I've used for JS compartments, which I'll be happy to explain if it's necessary.) Other than that, it mostly looks good, but I'll give r- because I'd like that clarified before proceeding. Thanks! ::: content/base/src/nsDocument.cpp @@ +9664,4 @@ > &aWindowSizes->mLayoutArenas, > &aWindowSizes->mLayoutStyleSets, > &aWindowSizes->mLayoutTextRuns, > &aWindowSizes->mLayoutPresContext); You mentioned using a struct for these in the other bug. Five numbers is enough that I agree it would be worthwhile, so feel free to structify this if you can be bothered. ::: layout/base/nsPresArena.cpp @@ +381,5 @@ > + static PLDHashOperator FreeListEnumerator(FreeList* aEntry, void* aData) > + { > + EnumerateData* data = static_cast<EnumerateData*>(aData); > + size_t totalSize = aEntry->mEntrySize * aEntry->mEntriesEverAllocated; > + size_t* p; This confused me greatly at first. I was wondering why you would be iterating over free lists -- surely you want to count all the objects, not just the ones that have been freed. Eventually I realized that there is one freelist per frame kind, and you're iterating *over* the freelists but not *within* each freelist; instead you're using mEntriesEverAllocated to compute the size of all the allocations done for each frame kind. A comment explaining this sure would be nice! @@ +410,5 @@ > + // we wouldn't be able to use aMallocSizeOf on them, since they were > + // allocated out of malloc'd chunks of memory. So we compute the > + // size of the arena as known by malloc, then we add up the sizes of > + // all the objects that we care about, and then subtract the two > + // quantities to give the "true" size of the arena. I don't understand this '"true" size'. Doesn't that subtraction give you the arena slop plus anything skipped over by the switch in FreeListEnumerator()? You have an |mOther| field in nsFrameMemoryStats that seems appropriate for this value, but you haven't used that field anywhere. @@ +417,5 @@ > + > + EnumerateData data = { aFramesSize, 0 }; > + mFreeLists.EnumerateEntries(FreeListEnumerator, &data); > + > + return mallocSize - data.total; I think you should do this here: aFramesSize->mOther = mallocSize - data.total; And then this function wouldn't need a return value.

Comment on attachment 630215 [details] [diff] [review] part 2 - report on individual frame types in about:memory Review of attachment 630215 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsIPresShell.h @@ +37,5 @@ > #include "nsGUIEvent.h" > #include "nsInterfaceHashtable.h" > #include "nsEventStates.h" > #include "nsPresArena.h" > +#include "nsFrameMemoryStats.h" Don't #include this here, just forward declare |struct nsFrameMemoryStats;|. Then you'll be able to inline nsFrameMemoryStats.h into nsWindowMemoryReporter.h.

Comment on attachment 630216 [details] [diff] [review] part 3 - report on specific objects in arenas Review of attachment 630216 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable, but given the comments below plus the r- on the previous patch, I'd like to see it again. Sorry for being such a brute with my reviews today! ::: dom/base/nsWindowMemoryReporter.h @@ +29,5 @@ > mMallocSizeOf = aMallocSizeOf; > } > nsMallocSizeOfFun mMallocSizeOf; > nsFrameMemoryStats mFrames; > + nsArenaObjectMemoryStats mArenaObjects; Would it be easier to merge these two types into |nsArenaMemoryStats|? That makes particular sense once you start using the |mOther| field like I suggested in the previous review. ::: layout/base/nsPresArena.cpp @@ +440,4 @@ > { > + if (mState) > + return mState->SizeOfIncludingThis(aMallocSizeOf, aFramesSize, aObjectsSize); > + return 0; Please revert this change. If the line is too long, just do: return mState ? mState->SizeOfIncludingThis(aMallocSizeOf, aFramesSize) : 0; ::: layout/base/nsPresArena.h @@ +10,5 @@ > > #include "nscore.h" > #include "nsQueryFrame.h" > #include "nsFrameMemoryStats.h" > +#include "nsArenaObjectMemoryStats.h" Don't #include this. Just forward declare "struct nsArenaObjectMemoryStats;" Once you've done that, nsArenaObjectMemoryStats.h can be folded into nsWindowMemoryReporter.h. @@ +43,5 @@ > > enum ObjectID { > nsLineBox_id = nsQueryFrame::NON_FRAME_MARKER, > + nsRuleNode_id, > + nsStyleContext_id, This change, and the changing of the AllocateFromShell() calls, are beyond my knowledge to review as part of this change -- can you get roc to look at them?

(In reply to Nicholas Nethercote [:njn] from comment #26) > Seems reasonable, but given the comments below plus the r- on the previous > patch, I'd like to see it again. Sorry for being such a brute with my > reviews today! Hey, if everybody r+'d everything, just think what the tree would soon look like! :) > Would it be easier to merge these two types into |nsArenaMemoryStats|? I like that, will do. > @@ +10,5 @@ > > > > #include "nscore.h" > > #include "nsQueryFrame.h" > > #include "nsFrameMemoryStats.h" > > +#include "nsArenaObjectMemoryStats.h" > > Don't #include this. Just forward declare "struct > nsArenaObjectMemoryStats;" Once you've done that, > nsArenaObjectMemoryStats.h can be folded into nsWindowMemoryReporter.h. I think the forward declaration is a good idea. I was less sure about including the "DOM" header into layout/ bits; I assumed that the reason for passing separate pointers into nsIPresShell::SizeOfIncludingThis rather than nsWindowSizes* was to avoid this DOM -> layout knowledge. I will fold the two structs together but leave the headers in layout/. roc to weigh in on pushing the structs to nsWindowMemoryReporter.h separately. > @@ +43,5 @@ > > > > enum ObjectID { > > nsLineBox_id = nsQueryFrame::NON_FRAME_MARKER, > > + nsRuleNode_id, > > + nsStyleContext_id, > > This change, and the changing of the AllocateFromShell() calls, are beyond > my knowledge to review as part of this change -- can you get roc to look at > them? You bet. Figured I'd get the about:memory-specifics ironed out and then roc could look at the layout-relevant changes.

(In reply to Nicholas Nethercote [:njn] from comment #24) > IIUC, you've increased the maximum number of entries in about:memory per > window by 168, i.e. the number of entries in nsFrameIdList.h? That is... > rather a lot. What does about:memory look like in practice now -- can you > post sample output for some complex pages? I don't want each window to be > bloated with heaps of tiny entries. I don't have a good benchmark for "complex" pages; a new browser session with a couple pages open looks like: ├──108.68 MB (39.46%) -- window-objects │ ├───72.32 MB (26.26%) -- top(http://hg.mozilla.org/mozilla-central/rev/c76497029f0d, id=7) │ │ ├──71.97 MB (26.13%) -- active/window(http://hg.mozilla.org/mozilla-central/rev/c76497029f0d) │ │ │ ├──38.67 MB (14.04%) ── dom [2] │ │ │ ├──33.26 MB (12.08%) -- layout │ │ │ │ ├──14.67 MB (05.33%) -- frames │ │ │ │ │ ├───7.56 MB (02.74%) ── nsInlineFrame │ │ │ │ │ ├───7.06 MB (02.56%) ── nsTextFrame │ │ │ │ │ └───0.06 MB (00.02%) ++ (19 tiny) │ │ │ │ ├──14.18 MB (05.15%) ── style-contexts │ │ │ │ └───4.41 MB (01.60%) ++ (5 tiny) │ │ │ └───0.04 MB (00.01%) ── style-sheets │ │ └───0.35 MB (00.13%) ++ cached/window(about:home) │ ├───11.42 MB (04.15%) -- top(http://us.battle.net/wow/en/forum/4629801/, id=50) │ │ ├───8.13 MB (02.95%) -- cached │ │ │ ├──4.49 MB (01.63%) ++ (2 tiny) │ │ │ └──3.64 MB (01.32%) ++ window(http://us.battle.net/wow/en/forum/) │ │ └───3.30 MB (01.20%) ++ active/window(http://us.battle.net/wow/en/forum/4629801/) │ ├────7.32 MB (02.66%) -- top(https://tbpl.mozilla.org/, id=45)/active/window(https://tbpl.mozilla.org/) │ │ ├──3.90 MB (01.42%) ── dom [2] │ │ ├──3.30 MB (01.20%) ++ layout │ │ └──0.12 MB (00.04%) ── style-sheets │ ├────4.30 MB (01.56%) -- top(http://www.nytimes.com/, id=24)/active │ │ ├──3.83 MB (01.39%) ++ window(http://www.nytimes.com/) │ │ └──0.47 MB (00.17%) ++ (3 tiny) │ ├────3.87 MB (01.40%) -- top(http://global.nytimes.com/, id=38)/active │ │ ├──3.86 MB (01.40%) ++ window(http://global.nytimes.com/) │ │ └──0.01 MB (00.00%) ++ window(http://www.nytimes.com/packages/html/recommendations/ad.html) │ ├────3.62 MB (01.32%) ++ top(http://www.bbc.co.uk/, id=18)/active/window(http://www.bbc.co.uk/) │ ├────3.51 MB (01.27%) -- top(http://www.reddit.com/, id=54) │ │ ├──3.28 MB (01.19%) -- active │ │ │ ├──3.07 MB (01.11%) ++ window(http://www.reddit.com/) │ │ │ └──0.22 MB (00.08%) ++ window(http://www.redditmedia.com/ads/) │ │ └──0.22 MB (00.08%) ++ cached/window(http://i.imgur.com/EV3g9.png) │ └────2.32 MB (00.84%) ++ (3 tiny) Explicitly opening the layout/frames/X subtree on the reddit.com tree does look ugly: │ ├────3.51 MB (01.27%) -- top(http://www.reddit.com/, id=54) │ │ ├──3.28 MB (01.19%) -- active │ │ │ ├──3.07 MB (01.11%) -- window(http://www.reddit.com/) │ │ │ │ ├──1.33 MB (00.48%) -- layout │ │ │ │ │ ├──0.38 MB (00.14%) ── style-sets │ │ │ │ │ ├──0.27 MB (00.10%) -- frames │ │ │ │ │ │ ├──0.09 MB (00.03%) ── nsInlineFrame │ │ │ │ │ │ ├──0.07 MB (00.03%) ── nsBlockFrame │ │ │ │ │ │ ├──0.06 MB (00.02%) ── nsTextFrame │ │ │ │ │ │ ├──0.04 MB (00.01%) ── nsHTMLScrollFrame │ │ │ │ │ │ ├──0.01 MB (00.00%) ── nsPlaceholderFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsBulletFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsContinuingTextFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsImageFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsScrollbarButtonFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTextControlFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsHTMLButtonControlFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsSliderFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsScrollbarFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsButtonBoxFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsBoxFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsBCTableCellFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableRowFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableColFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableOuterFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsCanvasFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableColGroupFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsTableRowGroupFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── nsSubDocumentFrame │ │ │ │ │ │ ├──0.00 MB (00.00%) ── ViewportFrame │ │ │ │ │ │ └──0.00 MB (00.00%) ── nsGfxCheckboxControlFrame │ │ │ │ │ ├──0.26 MB (00.09%) ── arenas │ │ │ │ │ ├──0.16 MB (00.06%) ── pres-contexts │ │ │ │ │ ├──0.16 MB (00.06%) ── style-contexts │ │ │ │ │ ├──0.05 MB (00.02%) ── rule-nodes │ │ │ │ │ ├──0.04 MB (00.01%) ── line-boxes │ │ │ │ │ └──0.01 MB (00.00%) ── text-runs │ │ │ │ ├──0.91 MB (00.33%) ── dom [2] │ │ │ │ └──0.82 MB (00.30%) ── style-sheets My guess is that some small number of frames will show up for larger pages (like the hg.mozilla.org tree above) and other places they will be inconsequential unless you navigate to the frames/ subtree. Opening a frames/ subtree (or even multiple ones with "verbose") does get a little overwhelming, though. Limiting to frames with > 10k bytes looks like it would be pretty reasonable, along with an "other" category for everything else. That would add ~5 subtrees for each of the examples abovee. > ::: content/base/src/nsDocument.cpp > @@ +9664,4 @@ > > &aWindowSizes->mLayoutArenas, > > &aWindowSizes->mLayoutStyleSets, > > &aWindowSizes->mLayoutTextRuns, > > &aWindowSizes->mLayoutPresContext); > > You mentioned using a struct for these in the other bug. Five numbers is > enough that I agree it would be worthwhile, so feel free to structify this > if you can be bothered. Will do in a separate rs= commit. > ::: layout/base/nsPresArena.cpp > @@ +381,5 @@ > > + static PLDHashOperator FreeListEnumerator(FreeList* aEntry, void* aData) > > + { > > + EnumerateData* data = static_cast<EnumerateData*>(aData); > > + size_t totalSize = aEntry->mEntrySize * aEntry->mEntriesEverAllocated; > > + size_t* p; > > This confused me greatly at first. I was wondering why you would be > iterating over free lists -- surely you want to count all the objects, not > just the ones that have been freed. > > Eventually I realized that there is one freelist per frame kind, and you're > iterating *over* the freelists but not *within* each freelist; instead > you're using mEntriesEverAllocated to compute the size of all the > allocations done for each frame kind. > > A comment explaining this sure would be nice! Will add. > @@ +417,5 @@ > > + > > + EnumerateData data = { aFramesSize, 0 }; > > + mFreeLists.EnumerateEntries(FreeListEnumerator, &data); > > + > > + return mallocSize - data.total; > > I think you should do this here: > > aFramesSize->mOther = mallocSize - data.total; > > And then this function wouldn't need a return value. Yeah, if I combine into an nsArenaMemoryStats, I think the mOther value is a better place to stick the return value.

(In reply to Nathan Froyd (:froydnj) from comment #28) > Limiting to frames with > 10k bytes looks like it would be pretty > reasonable, along with an "other" category for everything else. That would > add ~5 subtrees for each of the examples abovee. Another option would be to simply record "frames" as a single number and not break out per-type measurements, either by site or in "Other Measurements". I don't know what'd be most useful for the layout folks; I asked Ehsan to comment since he filed the bug originally.

(In reply to Nathan Froyd (:froydnj) from comment #29) > (In reply to Nathan Froyd (:froydnj) from comment #28) > > Limiting to frames with > 10k bytes looks like it would be pretty > > reasonable, along with an "other" category for everything else. That would > > add ~5 subtrees for each of the examples abovee. > > Another option would be to simply record "frames" as a single number and not > break out per-type measurements, either by site or in "Other Measurements". > I don't know what'd be most useful for the layout folks; I asked Ehsan to > comment since he filed the bug originally. I would very much like us to have detailed information here. Getting rid of the excessive amount of information in case it's not useful can (and should) be done as part of the post-processing, IMHO.

I think I addressed all the comments. r?'ing roc for general layout style comments. I'm curious as to whether defining nsArenaMemoryStats in dom/base/nsWindowMemoryReporter.h and then including that header in nsPresArena.cpp would be OK, or whether the current separation between DOM and layout bits is desirable.

Combining everything into one struct certainly made this patch easier to write. r? roc on this one for the allocation changes.

Is there a missing patch? I can't find the definition of FRAME_ID_STAT_FIELD

> My guess is that some small number of frames will show up for larger pages > (like the hg.mozilla.org tree above) and other places they will be > inconsequential unless you navigate to the frames/ subtree. Opening a > frames/ subtree (or even multiple ones with "verbose") does get a little > overwhelming, though. > > Limiting to frames with > 10k bytes looks like it would be pretty > reasonable, along with an "other" category for everything else. That would > add ~5 subtrees for each of the examples abovee. See patch 1 in bug 755583 for example code to do this -- in the JS engine we combine all the per-compartment reports that are less than 8KB into a single "sundries" entry. (Actually, it's complicated by the fact that the compartment reports involve both HEAP and NONHEAP memory, so we actually need two "sundries", but you won't have to do that.) > I would very much like us to have detailed information here. Getting rid of > the excessive amount of information in case it's not useful can (and should) > be done as part of the post-processing, IMHO. If by post-processing you mean within aboutMemory.js, the problem is that if aboutMemory.js has to process many reports, the amount of memory it consumes itself increases proportionally, thus perturbing what it's measuring. Combining uninteresting fields within the multi-reporter avoids this.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > Is there a missing patch? I can't find the definition of FRAME_ID_STAT_FIELD FRAME_ID_STAT_FIELD is in part 2, layout/base/nsArenaMemoryStats.h. It's certainly possible I missed a bit, but everything built OK and there were no untracked files in my srcdir, so I think I got everything.

Comment on attachment 630638 [details] [diff] [review] part 2 - report on individual frame types in about:memory Review of attachment 630638 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see the "sundries" change before this lands, please. (BTW, you've written "r=njn" on the patch description, which isn't actually true yet! :) ::: layout/base/nsPresShell.cpp @@ +8928,2 @@ > *aArenasSize = aMallocSizeOf(this); > + *aArenasSize += aArenaObjectsSize->mOther; Hmm, this isn't quite right. You're still reporting ".../layout/arenas" and you've changed what it means without updating the description. It makes more sense to me to remove the aArenasSize parameter here, and add |aMallocSizeOf(this)| to |aArenaObjectsSize->mOther|. And then rename |mOther| to |mArenasOther| and rename ".../layout/arenas" to something like ".../layout/arenas-other" and update the description.

(In reply to Nicholas Nethercote [:njn] from comment #36) > I'd like to see the "sundries" change before this lands, please. (BTW, > you've written "r=njn" on the patch description, which isn't actually true > yet! :) Will address the sundries change and submit new patch. The r= in the patch description is a holdover from checkin-needed days. :) > ::: layout/base/nsPresShell.cpp > @@ +8928,2 @@ > > *aArenasSize = aMallocSizeOf(this); > > + *aArenasSize += aArenaObjectsSize->mOther; > > Hmm, this isn't quite right. You're still reporting ".../layout/arenas" and > you've changed what it means without updating the description. > > It makes more sense to me to remove the aArenasSize parameter here, and add > |aMallocSizeOf(this)| to |aArenaObjectsSize->mOther|. And then rename > |mOther| to |mArenasOther| and rename ".../layout/arenas" to something like > ".../layout/arenas-other" and update the description. My preference would be renaming the aArenasSize parameter to aPresShellSize, like so: *aPresShellSize = aMallocSizeOf(this); *aPresShellSize += aArenaObjectsSize->mOther; and then renaming layout/arenas to layout/pres-shell, which I think more accurately reflects reality.

(In reply to Nicholas Nethercote [:njn] from comment #36) > I'd like to see the "sundries" change before this lands, please. (BTW, > you've written "r=njn" on the patch description, which isn't actually true > yet! :) I always do that. You can r+ the patch secure in the knowledge that you're making the description correct :-)

> My preference would be renaming the aArenasSize parameter to aPresShellSize, > like so: > > *aPresShellSize = aMallocSizeOf(this); > *aPresShellSize += aArenaObjectsSize->mOther; > > and then renaming layout/arenas to layout/pres-shell, which I think more > accurately reflects reality. Sounds good.

Implemented sundries support and /layout/arenas -> /layout/pres-shell renaming. I think this series is ready after r+ on this patch. sundries support seems to knock the number of nodes under /layout/frames/ to 5-7, not all of which may be displayed due to tinyness constraints.

Comment on attachment 631123 [details] [diff] [review] part 2 - report on individual frame types in about:memory Review of attachment 631123 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +8929,4 @@ > { > + mFrameArena.SizeOfExcludingThis(aMallocSizeOf, aArenaObjectsSize); > + *aPresShellSize = aMallocSizeOf(this); > + *aPresShellSize += aArenaObjectsSize->mOther; This feels wrong. I think aArenaObjectsSize->mOther should be reported under ".../layout/frames/other". In other words, all the arena numbers should be put together -- the slop + unmeasured stuff shouldn't be put in with PresShell numbers. Then aPresShellSize would really be measuring just the PresShell. (The description for ".../layout/pres-shell" would need tweaking too.) r=me with that changed. Apologies if this conflicts with anything I've suggested earlier.

(In reply to Nicholas Nethercote [:njn] from comment #41) > ::: layout/base/nsPresShell.cpp > @@ +8929,4 @@ > > { > > + mFrameArena.SizeOfExcludingThis(aMallocSizeOf, aArenaObjectsSize); > > + *aPresShellSize = aMallocSizeOf(this); > > + *aPresShellSize += aArenaObjectsSize->mOther; > > This feels wrong. I think aArenaObjectsSize->mOther should be reported > under ".../layout/frames/other". In other words, all the arena numbers > should be put together -- the slop + unmeasured stuff shouldn't be put in > with PresShell numbers. I don't think that's correct. aArenaObjectsSize->mOther isn't measuring frames. We can measure the frames usage precisely through frame IDs, hence the 150+ frame fields in nsArenaMemoryStats. mOther is measuring (as of this patch) slop + non-frame objects. After the last patch in the series, it's measuring slop + non-{frame,nsPresArena::ObjectID} objects. Ascribing that memory to the PresShell seems as good a solution as any. (I don't think the PresShell is so big otherwise that it would merit its own subnode, either.)

Fair enough. I think the pres-shell description still needs a slight update.

(In reply to Nicholas Nethercote [:njn] from comment #43) > Fair enough. I think the pres-shell description still needs a slight update. It's not clear to me what needs to be changed. Could you elaborate?

(In reply to Nathan Froyd (:froydnj) from comment #44) > (In reply to Nicholas Nethercote [:njn] from comment #43) > > Fair enough. I think the pres-shell description still needs a slight update. > > It's not clear to me what needs to be changed. Could you elaborate? We have this: REPORT("/layout/pres-shell", windowSizes.mLayoutPresShell, "Memory used by layout PresShell and other related " "areas within a window."); IIRC, this value is now: sizeof(nsPresShell) + arena slop + arena things not measured elsewhere Is that right? If so, the description should encompass that. I'll trust you to write something appropriate :)

(In reply to Nicholas Nethercote [:njn] from comment #45) > We have this: > > REPORT("/layout/pres-shell", windowSizes.mLayoutPresShell, > "Memory used by layout PresShell and other related " > "areas within a window."); > > IIRC, this value is now: > > sizeof(nsPresShell) + arena slop + arena things not measured elsewhere > > Is that right? If so, the description should encompass that. I'll trust > you to write something appropriate :) What we had before: REPORT("/layout/arenas", windowSizes.mLayoutArenas, "Memory used by layout PresShell, PresContext, and other related " "areas within a window."); measured the exact same thing. Save for removing PresContext and renaming to /layout/pres-shell, which the patch already does, I'm not sure what you find unsatisfactory about the description. Mentioning "arena things not measured elsewhere" seems redundant; discussing arena slop seems too specific. It's certainly possible I'm just being dense or stubborn. :) Perhaps you wanted an explicit mention of the arena measurements? I've gone ahead and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/68c6b4ab5821 https://hg.mozilla.org/integration/mozilla-inbound/rev/142fc9589c23 https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfcf09bcb24 https://hg.mozilla.org/integration/mozilla-inbound/rev/6bdce5e0d905 with the text: REPORT("/layout/pres-shell", windowSizes.mLayoutPresShell, "Memory used by layout's PresShell, along with any structures " "allocated in its arena and not measured elsewhere, " "within a window."); Please let me know if you'd like to see further edits.

> REPORT("/layout/pres-shell", windowSizes.mLayoutPresShell, > "Memory used by layout's PresShell, along with any structures " > "allocated in its arena and not measured elsewhere, " > "within a window."); That's great. Sorry for any confusion!

https://hg.mozilla.org/mozilla-central/rev/68c6b4ab5821 https://hg.mozilla.org/mozilla-central/rev/142fc9589c23 https://hg.mozilla.org/mozilla-central/rev/9bfcf09bcb24 https://hg.mozilla.org/mozilla-central/rev/6bdce5e0d905

Is nsArenaMemoryStats supposed to be a struct or a class?

(In reply to Siddharth Agarwal [:sid0] (unavailable until late June) from comment #49) > Is nsArenaMemoryStats supposed to be a struct or a class? See bug 764376.