808467 - Rewrite the cycle collector's memory reporter.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

The cycle collector's memory reporter can be improved.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Rewrite the cycle collector's memory reporter.

This patch:

- Rewrites the cycle collector memory reporter to be traversal-based instead of
  counter-based.  This is less error-prone and integrates with DMD.

- Converts the reporter to a multi-reporter that measures five different
  things: collector-object, graph-nodes, graph-edges, white-nodes-array,
  purple-buffer.  I've been unable to see the middle three have any value
  other than zero, which matches what the existing comments say should happen
  (i.e. they should only be non-zero during CC).

- Tweaks the size of nsPurpleBuffer::mEntries to waste less space, and adds a
  static assertion to make sure the size is as expected.
  
- Removes some unnecessary whitespace at the end of lines.

It would be nice if the first purple buffer block wasn't within 
nsCycleCollector, but I didn't go to the effort of changing that.

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 678207 [details] [diff] [review]
Rewrite the cycle collector's memory reporter.

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

Thanks for fixing this! The level of detail in the reporters seems like overkill, but I guess there's no reason not to have it now that the code is written.

You may or may not want to add a similar explanatory comment for why SizeOf for the node and edge structures don't follow the pointers, to match those for other structures.

::: xpcom/base/nsCycleCollector.cpp
@@ +737,5 @@
> +        // - On 32-bit platforms sizeof(nsPurpleBufferEntry) is 12, so mEntries
> +        //   is 16,380 bytes, which leaves 4 bytes for mNext.
> +        // - On 64-bit platforms sizeof(nsPurpleBufferEntry) is 24, so mEntries
> +        //   is 32,544 bytes, which leaves 8 bytes for mNext.
> +        nsPurpleBufferEntry mEntries[1365];

I don't know if 5 more entries helps much, but thanks for the explanation here. :)

@@ +3002,5 @@
> +    *aObjectSize = aMallocSizeOf(this);
> +
> +    mGraph.SizeOfExcludingThis(aMallocSizeOf, aGraphNodesSize, aGraphEdgesSize);
> +
> +    // No need to measure what the entries point to;  the pointers are

double space after ;

@@ +3041,5 @@
> +                                        &objectSize, &graphNodesSize,
> +                                        &graphEdgesSize, &whiteNodesSize,
> +                                        &purpleBufferSize);
> +
> +    #define REPORT(_path, _amount, _desc)                                     \

Could this be a method instead of a macro? Macros are evil!

@@ +3058,5 @@
> +        REPORT("explicit/cycle-collector/collector-object", objectSize,
> +               "Memory used for the cycle collector object itself.");
> +
> +        REPORT("explicit/cycle-collector/graph-nodes", graphNodesSize,
> +               "Memory used for the cycle collector's graph's nodes. "

"for the nodes of the cycle collector's graph" maybe? The two possessives in a row bothers me. ;)  Same with edges below.

@@ +3084,5 @@
> +    }
> +};
> +
> +NS_IMPL_ISUPPORTS1(CycleCollectorMultiReporter
> +                  , nsIMemoryMultiReporter

Is it really necessary to use this future-proof style?  Is this likely to ever change?

@@ +3100,5 @@
>      if (sCollector)
>          sCollector->RegisterJSRuntime(rt);
>      if (regMemReport) {
>          regMemReport = false;
> +        sCollectorReporter = new CycleCollectorMultiReporter;

The placement of the creation/destruction of the reporter in register/forgoetJSRuntime seems a little off to me.  In a browser without XPConnect (which happens in tests for reasons I don't entirely fathom) we'll end up leaking the reporter.

Can they go in nsCycleCollector_startup/nsCycleCollector_shutdown instead?

@@ +3106,4 @@
>      }
>  }
>  
>  void 

might as well remove this trailing whitespace if you are feeling ambitious

Comment 3

[Nicholas Nethercote [inactive]]

> Thanks for fixing this! The level of detail in the reporters seems like
> overkill, but I guess there's no reason not to have it now that the code is
> written.

It's the same level of detail as before :)


> > +    #define REPORT(_path, _amount, _desc)                                     \
> 
> Could this be a method instead of a macro? Macros are evil!

Not easily, because it has a |NS_ENSURE_SUCCESS| embedded in it.  If it helps, variants on that macro appear in several other places.


> The placement of the creation/destruction of the reporter in
> register/forgoetJSRuntime seems a little off to me.  In a browser without
> XPConnect (which happens in tests for reasons I don't entirely fathom) we'll
> end up leaking the reporter.

Until this patch we never unregistered the reporter, so we would have always leaked it...

> Can they go in nsCycleCollector_startup/nsCycleCollector_shutdown instead?

... sure.

Comment 4

[Andrew McCreight [:mccr8]]

Ah, right.  We'll have a dangling pointer, not a leak. ;)

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95bb2214c2bf

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/95bb2214c2bf