I realize we expire some of this stuff as we go, but if any of it is at all long-lived, it would be good to account for it in about:memory.

See bug 678376 comment 10.

This has been on my todo list for a while, I think I'll get to it soon.

I was wrong about getting to this soon. bz says jfkthame might be able to take it?

This tracks our total textrun memory usage as textruns are created and destroyed, so that the current usage can be reported. It's a little smarter than the (DEBUG-only) accounting that's currently in the tree, in that it tries to account for memory used for DetailedGlyph and GlyphRun records, as there may be significant numbers of these in some cases. (It still doesn't deal properly with the gfxSkipChars, though.) If we do this, we'll need to watch carefully for possible performance impact of the added housekeeping on textrun creation and destruction. (I suspect it'll be insignificant, but if that's not true then we may have to reconsider how badly we want to track this.) An alternative "zero-overhead" approach would be to make the reporter crawl over all live textruns and add up their sizes when asked, but I don't see an easy way to accomplish this. We can find _some_ of the textruns fairly easily via the textrun cache, but there are also a lot of runs that aren't referenced from the cache and so we'd miss them.

Comment on attachment 561439 [details] [diff] [review] patch, track memory used by gfxTextRuns Review of attachment 561439 [details] [diff] [review]: ----------------------------------------------------------------- Seems like the TEXT_RUN_STORAGE_ACCOUNTED setting and checking could be debug-only and we could just assert that every deallocated textrun was previously recorded? I do wonder if the zero-overhead version would be better... We could have PresShell::MemoryReporter ask the presshell to walk the frame tree, find any textframes and report their attached textruns, and then ask the textrun cache to report the size of any textruns that haven't already been counted. What do you think? That would let us report textruns per-document which would also be nice.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > Seems like the TEXT_RUN_STORAGE_ACCOUNTED setting and checking could be > debug-only and we could just assert that every deallocated textrun was > previously recorded? Yes, that'd be reasonable (though the benefit of making it debug-only would be pretty marginal, compared to the computation that's being done). > I do wonder if the zero-overhead version would be better... We could have > PresShell::MemoryReporter ask the presshell to walk the frame tree, find any > textframes and report their attached textruns, and then ask the textrun > cache to report the size of any textruns that haven't already been counted. > What do you think? That would let us report textruns per-document which > would also be nice. I'll try to work up an alternative patch along those lines, and see how hairy it gets. Meanwhile, I did some tryserver runs to see whether the accounting has a detectable impact on Tp numbers. Results of 10 runs on each of the desktop platforms, without the patch (for a baseline) and with it: baseline: Linux Linux64 OSX OSX64 Win7 WinXP 382.32 360.04 418.08 417.92 331.34 383.11 382.89 358.77 420.47 416.39 329.48 381.60 383.57 361.55 413.55 414.45 333.39 377.97 383.39 358.46 418.08 412.45 324.37 378.59 383.39 366.97 417.44 420.95 331.57 385.46 383.11 360.37 416.94 416.52 330.46 384.19 383.80 349.54 419.92 421.06 329.14 380.42 386.56 363.83 419.88 415.24 329.66 377.04 386.03 358.19 417.37 413.20 329.51 384.20 383.39 363.48 419.91 426.57 333.41 383.91 Avg/StdDev 383.85 360.12 418.16 417.48 330.23 381.65 1.36 4.65 2.06 4.32 2.58 2.99 with textrun-memory accounting: 386.43 361.43 418.90 430.65 333.06 385.25 383.10 361.13 414.65 414.21 330.85 384.71 383.52 363.07 416.35 417.02 332.66 382.31 384.66 365.95 414.74 418.78 334.26 378.52 384.15 361.49 413.94 415.57 334.72 384.26 385.01 360.29 415.09 412.99 334.26 378.59 385.74 365.95 417.60 417.39 329.39 376.24 383.52 362.59 417.37 416.18 329.61 381.09 384.66 365.59 414.74 416.52 333.86 384.26 384.93 355.61 420.57 413.28 333.49 382.31 Avg/StdDev 384.57 362.31 416.40 417.26 332.62 381.75 1.04 3.16 2.17 5.05 1.97 3.08 Change 0.73 2.19 -1.77 -0.22 2.38 0.11 %change 0.19 0.61 -0.42 -0.05 0.72 0.03 The "improvements" on both OS X and OS X 64 are surely not real - there's no way this patch could give a true performance win - I'm sure that they'd disappear over the course of a large enough number of tests, but the effect here is so small that it is swamped by Talos noise. Still, the majority of the results here do hint at a tiny regression, so let's see if we can avoid that.

OK, here's a version that doesn't add any accounting overhead during normal operations; instead it goes looking for all current textruns when the memory reporter is asked for the total. It's a bit of a pain because we may end up visiting the same textrun multiple times, both while walking the frame tree from the presShell and from entries in the gfxTextRunWordCache. So to avoid double-accounting, we need to walk all the frame trees and the cache twice, using a flag in the textrun (good thing there seem to be some spare flag bits!) to mark which ones have been seen.

Forgot to mention - this relies on the patch in bug 682437 (to provide nsTHashtable::SizeOf), so it won't build unless that is applied.

Comment on attachment 561439 [details] [diff] [review] patch, track memory used by gfxTextRuns Review of attachment 561439 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +1008,5 @@ > +} > + > +NS_MEMORY_REPORTER_IMPLEMENT( > + TextRuns, > + "explicit/graphics/text-runs", Elsewhere we have reporters that use "gfx" in their path, so this reporter should do likewise. Having said that, the "crawl over data structures" approach is preferred, because it's less error prone than maintaining counters, and it makes life *much* easier for DMD (bug 676724).

Comment on attachment 562734 [details] [diff] [review] alternative patch - report memory used for textruns by finding them via the frame tree and textrun cache Review of attachment 562734 [details] [diff] [review]: ----------------------------------------------------------------- Other than that, looks great. ::: gfx/thebes/gfxFont.cpp @@ +4595,5 @@ > + total += mDetailedGlyphs->ComputeSize(); > + } > + > + if (mGlyphRuns.Length() > 1) { > + total += mGlyphRuns.Length() * ((sizeof(GlyphRun) + 3) & ~3); The rounding here is not correct on 64-bit platforms I think. I think we need some kind of macro or template that does something like sizeof(T[2])/2, but surely that already exists for other memory reporters? Nick? ::: gfx/thebes/gfxFont.h @@ +1389,5 @@ > * other effects. > */ > + TEXT_OPTIMIZE_SPEED = 0x0100, > + > + TEXT_RUN_SIZE_ACCOUNTED = 0x0200 Needs a comment. Might make more sense to just have a hash-set that we keep gfxTextRun pointers in. You could make it a pointer in a global variable so you don't have to pass the set around through your enumerators, or you could just fold it into the userdata. Then we wouldn't have to enumerate the textruns twice, which might end up being simpler code. ::: layout/base/nsLayoutUtils.cpp @@ +4318,5 @@ > + continue; > + } > + child = nsPlaceholderFrame::GetRealFrameFor(child); > + GetTextRunMemoryForFrames(child, aTotal); > + } I think you can simplify this by not doing the placeholder::GetRealFrameFor thing and just enumerating all child lists. Also, it's not safe to skip non-first-continuations here.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > > I think we need some kind of macro or template that does something like > sizeof(T[2])/2, but surely that already exists for other memory reporters? > Nick? Not that I know of. Also, I've been trying (and failing) to get people to use moz_malloc_usable_size in their memory reporters...

(In reply to Nicholas Nethercote [:njn] (on vacation Sep 12--Oct 17) from comment #11) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > > > > I think we need some kind of macro or template that does something like > > sizeof(T[2])/2, but surely that already exists for other memory reporters? > > Nick? > > Not that I know of. Actually we don't need that, since I think C/C++ guarantee that sizeof(T) leads to proper alignment. So sizeof(T) === sizeof(T[2])/2 and the rounding code in Jonathan's patch is not needed. > Also, I've been trying (and failing) to get people to use > moz_malloc_usable_size in their memory reporters... Yes, we should use that here! Also, we could use nsTArray::SizeOf() here, but it seems to me that that has a bug ... it should return 0 for nsAutoTArrays, shouldn't it?

In fact, nsTArray::SizeOf should use moz_malloc_usable_size itself. Right?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13) > In fact, nsTArray::SizeOf should use moz_malloc_usable_size itself. Right? Probably. And with fallback code for the case where moz_malloc_usable_size returns 0 because it's not supported.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > Might make more sense to just have a hash-set that we keep gfxTextRun > pointers in. You could make it a pointer in a global variable so you don't > have to pass the set around through your enumerators, or you could just fold > it into the userdata. Then we wouldn't have to enumerate the textruns twice, > which might end up being simpler code. I considered that approach, but was reluctant to do it because it seems unfortunate if the process of memory-reporting itself causes significant additional memory pressure. The number of textruns can get quite large - I hit 75,000 easily just by loading a couple of (large, text-heavy) tabs - and I didn't want to create such a large additional data structure (even temporarily) just for the sake of the reporter. > ::: layout/base/nsLayoutUtils.cpp > @@ +4318,5 @@ > > + continue; > > + } > > + child = nsPlaceholderFrame::GetRealFrameFor(child); > > + GetTextRunMemoryForFrames(child, aTotal); > > + } > > I think you can simplify this by not doing the placeholder::GetRealFrameFor > thing and just enumerating all child lists. Also, it's not safe to skip > non-first-continuations here. To help my understanding of the frame stuff - why would this be different from the GetFontFacesForFrames() version?

(In reply to Jonathan Kew (:jfkthame) from comment #15) > I considered that approach, but was reluctant to do it because it seems > unfortunate if the process of memory-reporting itself causes significant > additional memory pressure. The number of textruns can get quite large - I > hit 75,000 easily just by loading a couple of (large, text-heavy) tabs - and > I didn't want to create such a large additional data structure (even > temporarily) just for the sake of the reporter. I really don't think this matters. We're talking about 8-16 bytes overhead per textrun. Nick? > > ::: layout/base/nsLayoutUtils.cpp > > @@ +4318,5 @@ > > > + continue; > > > + } > > > + child = nsPlaceholderFrame::GetRealFrameFor(child); > > > + GetTextRunMemoryForFrames(child, aTotal); > > > + } > > > > I think you can simplify this by not doing the placeholder::GetRealFrameFor > > thing and just enumerating all child lists. Also, it's not safe to skip > > non-first-continuations here. > > To help my understanding of the frame stuff - why would this be different > from the GetFontFacesForFrames() version? In GetFontFacesForFrames we wanted to traverse the frames for the content rooted at a particular element. That's why we had to traverse placeholders; because a float or positioned child element might not be found just traversing the regular frame tree. E.g. the parent frame for a fixed-pos element's frame would be the viewport, even if the fixed-pos element is a child of some DIV in the body. Here we only care about finding all the frames in the presshell, so a simple walk of the frame tree suffices.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > I really don't think this matters. We're talking about 8-16 bytes overhead > per textrun. Nick? Sure, it's not huge - but it can be a megabyte or so without much difficulty, potentially more in extreme cases. That's enough that it should appear in about:memory in its own right! (OK, not really, because it would be transient. But it's a lot of extra heap churn to create and populate such a hashset, just to throw it away again.) IMO, the two-pass approach of marking the runs isn't significantly more complex, and seems less disruptive overall. > Here we only care about finding all the frames in the presshell, so a simple > walk of the frame tree suffices. OK, that makes sense.

Tidied up in the light of comments above, including use of nsTArray::SizeOf rather than manual computations. This still uses the approach of marking runs as they're counted, rather than remembering them in a hash. Let me know if you'd strongly prefer that approach, otherwise my inclination is to stay with this version. A question re nsLayoutUtils::GetTextRunMemoryForFrames ... is there any possibility (either in current code or a theoretical future) of a textFrame having children or continuation/special-sibling frames that we also ought to visit, or will it always be correct to treat textFrames as ending the current branch of the tree-walk?

Textframes can most definitely have continuations. Testcase: <div style="width: 0">Some text</div> They can't have children or special siblings.

Comment on attachment 563100 [details] [diff] [review] alternative patch, v2 Review of attachment 563100 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +4482,5 @@ > + total += GetLength() * > + ((GetFlags() & gfxTextRunFactory::TEXT_IS_8BIT) ? 1 : 2); > + total += sizeof(CompressedGlyph) - 1; > + total &= ~(sizeof(CompressedGlyph) - 1); > + } I think you should be using moz_malloc_usable_size here (passing "this"), and falling back to this code if that returns 0. @@ +4489,5 @@ > + total += mDetailedGlyphs->SizeOf(); > + } > + > + if (mGlyphRuns.Length() > 1) { > + total += mGlyphRuns.SizeOf(); I think we should just fix nsTArray::SizeOf to return 0 for nsAutoTArrays (and use moz_malloc_usable_size too!) ::: gfx/thebes/gfxFont.h @@ +2050,5 @@ > + // nsTransformedTextRun needs to override this as it holds additional data > + virtual PRUint32 ComputeSize(); > + > + bool SizeAccounted() { return mFlags & gfxTextRunFactory::TEXT_RUN_SIZE_ACCOUNTED; } > + void SetSizeAccounted() { mFlags |= gfxTextRunFactory::TEXT_RUN_SIZE_ACCOUNTED; } I think it would simplify things to have a method AccountForSize(PRUint64* aTotal) that a) bails if SIZE_ACCOUNTED b) sets SIZE_ACCOUNTED c) adds the size to *aTotal. ::: gfx/thebes/gfxTextRunWordCache.cpp @@ +134,5 @@ > mGeneration++; > #endif > } > > + void ComputeStorage(PRUint32 *aTotal); All these totals should be PRUint64. ::: layout/base/nsLayoutUtils.cpp @@ +4312,5 @@ > + for (int i = 0; i < NS_ARRAY_LENGTH(childLists); ++i) { > + nsFrameList children(aFrame->GetChildList(childLists[i])); > + for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) { > + GetTextRunMemoryForFrames(e.get(), aTotal); > + } You really need to iterate through all child lists. ::: layout/generic/nsTextRunTransformations.cpp @@ +109,5 @@ > + if (mOwnsFactory) { > + // this may not quite account for everything > + // (e.g. nsCaseTransformTextRunFactory adds a couple of members) > + // but I'm not sure it's worth the effort to track more precisely > + total += sizeof(nsTransformingTextRunFactory); Use moz_malloc_usable_size.

(In reply to Jonathan Kew (:jfkthame) from comment #18) > This still uses the approach of marking runs as they're counted, rather than > remembering them in a hash. Let me know if you'd strongly prefer that > approach, otherwise my inclination is to stay with this version. OK. > A question re nsLayoutUtils::GetTextRunMemoryForFrames ... is there any > possibility (either in current code or a theoretical future) of a textFrame > having children or continuation/special-sibling frames that we also ought to > visit, or will it always be correct to treat textFrames as ending the > current branch of the tree-walk? textframes have continuations but won't ever have children. Having continuations is no problem since they're also in the frame tree.

Comment on attachment 563423 [details] [diff] [review] textruns memory reporting, v3 Review of attachment 563423 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +4483,5 @@ > + if (!(GetFlags() & gfxTextRunFactory::TEXT_IS_PERSISTENT)) { > + total += GetLength() * > + ((GetFlags() & gfxTextRunFactory::TEXT_IS_8BIT) ? 1 : 2); > + total += sizeof(CompressedGlyph) - 1; > + total &= ~(sizeof(CompressedGlyph) - 1); you're depending on sizeof(CompressedGlyph) being a power of 2, so probably should write PR_STATIC_ASSERT(sizeof(CompressedGlyph) == 4) and just use that. ::: layout/base/nsLayoutUtils.cpp @@ +4311,5 @@ > + !childLists.IsDone(); childLists.Next()) > + { > + for (nsFrameList::Enumerator e(childLists.CurrentList()); > + !e.AtEnd(); e.Next()) > + { { on same line as for ::: layout/generic/nsTextRunTransformations.cpp @@ +103,5 @@ > +nsTransformedTextRun::ComputeSize() > +{ > + PRUint32 total = gfxTextRun::ComputeSize(); > + total += sizeof(nsTransformedTextRun) - sizeof(gfxTextRun); > + total += mStyles.SizeOf(); This isn't right since if gfxTextRun::ComputeSize uses moz_malloc_usable_size, sizeof(nsTransformedTextRun) - sizeof(gfxTextRun) has already been accounted for. So I think we need to call moz_malloc_usable_size(this) here and check if it's 0.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > ::: gfx/thebes/gfxFont.cpp > @@ +4483,5 @@ > > + if (!(GetFlags() & gfxTextRunFactory::TEXT_IS_PERSISTENT)) { > > + total += GetLength() * > > + ((GetFlags() & gfxTextRunFactory::TEXT_IS_8BIT) ? 1 : 2); > > + total += sizeof(CompressedGlyph) - 1; > > + total &= ~(sizeof(CompressedGlyph) - 1); > > you're depending on sizeof(CompressedGlyph) being a power of 2, so probably > should write PR_STATIC_ASSERT(sizeof(CompressedGlyph) == 4) and just use > that. Actually, this was structured incorrectly anyway because the glyph storage is not fused with the textrun object, it's a separate allocation pointed to by mCharacterGlyphs. So we need to use moz_malloc_usable_size on that pointer as well, and fall back to a computation if that's not supported. So in the end, I've factored the calculation out of gfxTextRun::AllocateStorage into a local static function; that way we can re-use the exact same calculation here. > This isn't right since if gfxTextRun::ComputeSize uses > moz_malloc_usable_size, sizeof(nsTransformedTextRun) - sizeof(gfxTextRun) > has already been accounted for. So I think we need to call > moz_malloc_usable_size(this) here and check if it's 0. Good point, fixed.

Comment on attachment 563688 [details] [diff] [review] textruns memory reporting, v4 Review of attachment 563688 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that fixed ::: gfx/thebes/gfxFont.cpp @@ +3116,3 @@ > #endif > > // Helper for textRun creation to preallocate storage for glyphs and text; This comment belongs before AllocateStorage, doesn't it?

Updated per Roc's review. Flagging for r? from Justin as this required merging with recent ns[Auto]TArray changes; please check that it's still OK in that area. (The primary change was to make nsTArray::SizeOf return zero for auto-arrays; implementing this involved some other const-ifying.)

Comment on attachment 566832 [details] [diff] [review] textrun memory reporter, v5 Making UsesAutoArrayBuffer const is good. But I'd like to avoid duplicating that code, if possible. Can we const-cast our way to victory (e.g., [1])? I'd like a const version of GetAutoArrayBuffer(), too, so we don't have to call the unsafe one to get const-ness. [1] http://stackoverflow.com/questions/123758/how-do-i-remove-code-duplication-between-similar-const-and-non-const-member-funct/123995#123995

Here's a version that uses const_cast to avoid duplicating the guts of GetAutoArrayBufferUnsafe. I wondered whether to try this approach, but tend to feel uneasy about casting away const-ness too freely... seems OK here, though.

Comment on attachment 566874 [details] [diff] [review] textrun memory reporter, v6 > + Header* GetAutoArrayBufferUnsafe(size_t elemAlign) { > + return const_cast<Header*>(GetAutoArrayBufferUnsafe(elemAlign)); > + } Does this actually call the const GetAutoArrayBufferUnsafe? You don't have to static cast |this| to a const version? > + Header* GetAutoArrayBuffer(size_t elemAlign) { > + return const_cast<Header*>(GetAutoArrayBuffer(elemAlign)); > + } Similarly here. This one is so small I'm fine copy-pasting it; whatever you'd prefer is fine.

(In reply to Justin Lebar [:jlebar] from comment #29) > Comment on attachment 566874 [details] [diff] [review] [diff] [details] [review] > textrun memory reporter, v6 > > > + Header* GetAutoArrayBufferUnsafe(size_t elemAlign) { > > + return const_cast<Header*>(GetAutoArrayBufferUnsafe(elemAlign)); > > + } > > Does this actually call the const GetAutoArrayBufferUnsafe? You don't have > to static cast |this| to a const version? Hmm, yes, of course that's needed! (And my test build agrees.)

Comment on attachment 566914 [details] [diff] [review] textrun memory reporter, v7 > + // We can safely use the "...Unsafe" version of GetAutoArrayBuffer here > + // because we already checked above whether this is in fact an autoarray, > + // so the extra assertion in the "safe" version is completely redundant. You don't need this and these changes to GetAutoArrayBufferUnsafe now that we have a const GetAutoArrayBuffer, right? That whole function is horribly unsafe, but I'd rather not make it even worse. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/56c7568767f1 (In reply to Justin Lebar [:jlebar] from comment #32) > You don't need this and these changes to GetAutoArrayBufferUnsafe now that > we have a const GetAutoArrayBuffer, right? > > That whole function is horribly unsafe, but I'd rather not make it even > worse. :) OK, I landed it without these changes, so you can sleep peacefully at night. ;) (ISTM that we don't actually gain any "safety" from using the non-Unsafe functions here, as UsesAutoArrayBuffer starts out by checking whether we're an auto-array and bailing early if not. So the assertion in GetAutoArrayBuffer is redundant; it's just asserting the same thing that UsesAutoArrayBuffer already checked a few lines previously. But the assertions are DEBUG-only anyhow, so I suppose it doesn't really matter.) BTW, I just noticed what looks like a mis-statement in the comment here, if I'm understanding things correctly: // Note that this means that we can't store elements with alignment 16 in an - // nsTArray, because GetAutoArrayBuffer(16) could lie outside the memory + // nsAutoTArray, because GetAutoArrayBuffer(16) could lie outside the memory // owned by this nsAutoTArray. We statically assert that elem_type's Maybe worth correcting this sometime, to avoid confusing future readers.