Closed
Bug 824395
Opened 11 years ago
Closed 11 years ago
Separate out unused and used stack traces in DMD's output
Categories
(Core :: DMD, defect)
Core
DMD
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file, 1 obsolete file)
6.06 KB,
patch
|
n.nethercote
:
review+
justin.lebar+bug
:
approval-mozilla-aurora+
justin.lebar+bug
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
Before we go crazy trying to reduce the size of DMD's stack traces, we should determine whether we have to keep all of these stacks around -- perhaps most of them are GC'able. Patch in a moment.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #695368 -
Flags: review?(n.nethercote)
Comment 2•11 years ago
|
||
Comment on attachment 695368 [details] [diff] [review] Patch, v1 Review of attachment 695368 [details] [diff] [review]: ----------------------------------------------------------------- Basic idea is fine, but there are enough nits I'd like to see a 2nd version. Thanks! Thinking ahead, if we do some kind of stack trace GC I think it probably shouldn't be a ref-counting style thing with immediate freeing. I can imagine loops where we malloc then free repeatedly, and that could lead to a lot of stack trace table churn if we had immediate stack trace freeing. ::: memory/replace/dmd/DMD.cpp @@ +1863,5 @@ > { > if (!gIsDMDRunning) { > aSizes->Clear(); > return; > } I just realized that SizeOf() is called from Dump(), where the global lock is held and intercepts are blocked, and from the memory reporter where those aren't true. Would you mind splitting SizeOf() into two functions, one exported and one non-exported? The exported one should do lock+block and then call the non-exported one. And the non-exported one should have this at the top: MOZ_ASSERT(gStateLock->IsLocked()); MOZ_ASSERT(aT->InterceptsAreBlocked()); @@ +1870,5 @@ > + InfallibleAllocPolicy> usedStackTraces; > + usedStackTraces.init(1024); > + > + for(BlockTable::Range r = gBlockTable->all(); !r.empty(); r.popFront()) { > + usedStackTraces.put(r.front().AllocStackTrace()); You need to look for reported stack traces, too. (I was going to ask what kind of numbers you are seeing, but without those they'll be inaccurate.) ::: xpcom/base/nsMemoryReporterManager.cpp @@ +598,5 @@ > > + REPORT("explicit/dmd/used-stack-traces", > + sizes.mUsedStackTraces, > + "Memory used by stack traces which correspond to at least " > + "one heap block DMD is tracking."); Can you make it "explicit/dmd/stack-traces/used"? Then it'll add used+unused for us. The member names will need to change to mStackTraces{Used,Unused}. If you want to change "explicit/dmd/stack-trace-table" to "explicit/dmd/stack-traces/table" at the same time, that'd be good.
Attachment #695368 -
Flags: review?(n.nethercote) → review-
Assignee | ||
Comment 3•11 years ago
|
||
> Thinking ahead, if we do some kind of stack trace GC I think it probably shouldn't be a
> ref-counting style thing with immediate freeing.
I agree, if only because refcounting means extra storage bits devoted to a refcount. :)
Assignee | ||
Comment 4•11 years ago
|
||
On mac 10.8, debug build, right after startup. ├───2.77 MB (04.13%) -- dmd │ ├──2.02 MB (03.01%) -- stack-traces │ │ ├──1.19 MB (01.78%) ── unused │ │ └──0.82 MB (01.23%) -- (2 tiny) │ │ ├──0.57 MB (00.86%) ── used │ │ └──0.25 MB (00.37%) ── table
Assignee | ||
Comment 5•11 years ago
|
||
And this is mac 10.8, debug build after opening a few tabs and then closing them all and minimizing memory usage: ├────9.52 MB (06.26%) -- dmd │ ├──8.02 MB (05.28%) -- stack-traces │ │ ├──6.28 MB (04.13%) ── unused │ │ └──1.75 MB (01.15%) -- (2 tiny) │ │ ├──1.00 MB (00.66%) ── table │ │ └──0.75 MB (00.49%) ── used
Assignee | ||
Comment 6•11 years ago
|
||
The about:memory dumps above were from v2, not v1.
Attachment #695382 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•11 years ago
|
Attachment #695368 -
Attachment is obsolete: true
Comment 7•11 years ago
|
||
Comment on attachment 695382 [details] [diff] [review] Patch, v2 Review of attachment 695382 [details] [diff] [review]: ----------------------------------------------------------------- Neato. ::: memory/replace/dmd/DMD.cpp @@ +1881,5 @@ > + for(BlockTable::Range r = gBlockTable->all(); !r.empty(); r.popFront()) { > + const Block& b = r.front(); > + usedStackTraces.put(b.AllocStackTrace()); > + usedStackTraces.put(b.ReportStackTrace1()); > + usedStackTraces.put(b.ReportStackTrace2()); The report stack traces can be nullptr. This code will work fine, but I wonder if it would be clearer if you only |put| them if they were non-nullptr? @@ +1907,5 @@ > +SizeOf(Sizes* aSizes) > +{ > + aSizes->Clear(); > + > + // If DMD is not running, we can't do AutoLockState. Nit: I don't think we need that comment. Every exported DMD function checks |!gIsDMDRunning| at the start. ::: memory/replace/dmd/DMD.h @@ +57,1 @@ > size_t mStackTraceTable; Can you change these three fields to match the path names? I.e. mStackTracesUsed, mStackTracesUnused, mStackTracesTable.
Attachment #695382 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 8•11 years ago
|
||
> This code will work fine, but I wonder if it would be clearer if you only |put| them if
> they were non-nullptr?
eh, it works and nullchecks would make it three times as long. Unless you really don't like it, I'll leave it.
I'll fix the rest of it and push. Thanks for the reviews!
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/71b0dfee3df2
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 695382 [details] [diff] [review] Patch, v2 I'd like this for b2g, since that's where we care about how much memory dmd is using.
Attachment #695382 -
Flags: approval-mozilla-b2g18+
Attachment #695382 -
Flags: approval-mozilla-aurora+
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71b0dfee3df2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 12•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/01fae2cfcf4f https://hg.mozilla.org/releases/mozilla-b2g18/rev/0df8cd8c5913
Updated•11 years ago
|
Component: about:memory → DMD
Product: Toolkit → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•