Closed Bug 689583 Opened 8 years ago Closed 8 years ago

Add names for memory multi-reporters

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

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

References

Details

Attachments

(1 file)

In bug 688233, I noticed that it would be helpful to add a name to memory multi-reporters.  As is, if a multi-reporter throws an error before returning any results, we have no idea which reporter failed.
I'll do this.  It'll also help if you want to only query one multi-reporter, e.g. for telemetry.
Assignee: nobody → nnethercote
Blocks: 697041
Blocks: 715025
Attached patch patchSplinter Review
This patch:

- Adds the |name| field to nsIMemoryMultiReporter, and implements it for all
  existing multi-reporters.

- Tweaks aboutMemory.js so that it skips the "map" multi-reporter in
  non-verbose mode.  This reduces the amount of allocation (and thus
  perturbation) caused when generating about:memory.  Also, the ignoring of
  map entries (which can come from children processes and so still must be 
  handled) is now more efficient, involving one traversal of the reporter 
  list instead of four.
Attachment #594627 - Flags: review?(justin.lebar+bug)
Blocks: 702300
Comment on attachment 594627 [details] [diff] [review]
patch

>diff --git a/dom/base/nsDOMMemoryReporter.cpp b/dom/base/nsDOMMemoryReporter.cpp
>--- a/dom/base/nsDOMMemoryReporter.cpp
>+++ b/dom/base/nsDOMMemoryReporter.cpp
>@@ -231,16 +231,23 @@ PLDHashOperator
> GetWindows(const PRUint64& aId, nsGlobalWindow*& aWindow, void* aClosure)
> {
>   ((WindowArray *)aClosure)->AppendElement(aWindow);
> 
>   return PL_DHASH_NEXT;
> }
> 
> NS_IMETHODIMP
>+nsDOMMemoryMultiReporter::GetName(nsACString &name)
>+{
>+  name.Assign("dom+style");

I think you want AssignLiteral() here and elsewhere.

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js
>--- a/toolkit/components/aboutmemory/content/aboutMemory.js
>+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
>@@ -299,16 +299,21 @@ function getReportersByProcess(aMgr)
>     catch(e) {
>       debug("An error occurred when collecting results from the memory reporter " +
>             rOrig.path + ": " + e);
>     }
>   }
>   var e = aMgr.enumerateMultiReporters();
>   while (e.hasMoreElements()) {
>     var mrOrig = e.getNext().QueryInterface(Ci.nsIMemoryMultiReporter);
>+    // Ignore the "map" reporters in non-verbose mode.
>+    if (!gVerbose && mrOrig.name === "map") {
>+      continue;
>+    }

\o/

>@@ -771,31 +774,34 @@ function genProcessText(aProcess, aRepor
> {
>   var explicitTree = buildTree(aReporters, 'explicit');
>   var hasKnownHeapAllocated = fixUpExplicitTree(explicitTree, aReporters);
>   sortTreeAndInsertAggregateNodes(explicitTree._amount, explicitTree);
>   var explicitText = genTreeText(explicitTree, aProcess);
> 
>   // We only show these breakdown trees in verbose mode.
>   var mapTreeText = "";
>+  if (gVerbose) {
>+    kMapTreePaths.forEach(function(t) {
>       var tree = buildTree(aReporters, t);
> 
>       // |tree| will be null if we don't have any reporters for the given
>       // unsafePath.
>       if (tree) {
>         sortTreeAndInsertAggregateNodes(tree._amount, tree);
>         tree._hideKids = true;   // map trees are always initially collapsed
>         mapTreeText += genTreeText(tree, aProcess);
>       }
>+    });
>+  } else {
>+    // (Although we skip the "maps" multi-reporter in getReportersByProcess(),
>+    // we might get some maps reports from a child process, so we still must do
>+    // this.
>+    ignoreMapTrees(aReporters);

Bummer.  But one step at a time, I guess.

>+  }

>diff --git a/xpcom/base/MapsMemoryReporter.cpp b/xpcom/base/MapsMemoryReporter.cpp
>--- a/xpcom/base/MapsMemoryReporter.cpp
>+++ b/xpcom/base/MapsMemoryReporter.cpp
>@@ -143,16 +143,22 @@ struct CategoriesSeen {
> 
> class MapsReporter : public nsIMemoryMultiReporter
> {
> public:
>   MapsReporter();
> 
>   NS_DECL_ISUPPORTS
> 
>+  NS_IMETHOD GetName(nsACString &name)
>+  {
>+      name.Assign("map");

Perhaps 'smaps' would be a better name?  I may have fallen into the trap of
assuming generality when there's just one instance.
Attachment #594627 - Flags: review?(justin.lebar+bug) → review+
> >+  NS_IMETHOD GetName(nsACString &name)
> >+  {
> >+      name.Assign("map");
> 
> Perhaps 'smaps' would be a better name?  I may have fallen into the trap of
> assuming generality when there's just one instance.

In early versions of the patch I had "maps" but I went with "map" to match the paths used in aboutMemory.js ("map/vsize", etc).
I guess we'd need to modify that path, too.  In a follow-up bug.
I ended up renaming the paths to start with "smaps/" in this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9464928e08
https://hg.mozilla.org/mozilla-central/rev/4e9464928e08
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.