Closed Bug 824395 Opened 7 years ago Closed 7 years ago

Separate out unused and used stack traces in DMD's output

Categories

(Core :: DMD, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

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

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #695368 - Flags: review?(n.nethercote)
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-
> 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.  :)
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
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
Attached patch Patch, v2Splinter Review
The about:memory dumps above were from v2, not v1.
Attachment #695382 - Flags: review?(n.nethercote)
Attachment #695368 - Attachment is obsolete: true
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+
> 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!
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+
https://hg.mozilla.org/mozilla-central/rev/71b0dfee3df2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: about:memory → DMD
Product: Toolkit → Core
You need to log in before you can comment on or make changes to this bug.