Closed Bug 903285 Opened 11 years ago Closed 11 years ago

Add a memory reporter for the prefs service

Categories

(Toolkit :: about:memory, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

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

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

We could add a proper memory reporter that counts the size of the prefs service's observers data structure, but what I'm actually more interested in is a memory reporter which reports

a) The number of strong pref observers,
b) The number of weak pref observers whose referent is alive, and
c) The number of weak pref observers whose referent is dead.

A memory reporter like this would make it obvious if we're leaking tons of pref observers in a long-running B2G test.  I've never seen this leak in practice, but I have seen a similar leak: In bug 901416, we leaked a ton of strong observer-service observers.

Even if 10,000 (or whatever) pref observers wouldn't take up a lot of space, the pref service code has a number of O(n) operations, so a device could end up slowing down due to a ton of pref observers.
Maybe it would make sense to report any pref which has more than N observers, kind of like we do for long strings.  This would make it trivial to debug most leaks here.
Someone asked me online for more details here.

The prefs service has two layers of observers: nsPrefBranch.{h,cpp} and prefapi.{h,cpp}.  nsPrefBranch contains a hashtable of observers and also registers those observers with prefapi by way of the PREF_RegisterCallback function.  prefapi then keeps a linked list of listeners.

For the purposes of writing a counting memory reporter, you'd only need to look at one of these layers.  Either one would work, I think.

You'd iterate over the layer's hashtable/linked list of pref observers and count how many are strong/weak+alive/weak+dead.  Then you'd write a nsIMemoryMultiReporter which reports the results.

To tell whether a nsWeakPtr is alive or dead, you can do

  nsWeakPtr weakPtr = ...;
  nsCOMPtr<nsISupports> strongPtr = do_QueryReferent(weakPtr);
  if (strongPtr) {
    // It's weak+alive.
  } else {
    // It's weak+dead.
  }

To write an nIMemoryMultiReporter, see nsIMemoryReporter.idl for documentation, and ContentParent.cpp for a relatively simple reporter you can crib off.

Hopefully it's clear how to write code which will specifically call out prefs with many observers (you'll probably want to use a hashtable; see [1]), but if it's not, I'd say just write a patch which counts strong/weak+alive/weak+dead and then we can talk about how to do this.

[1] https://developer.mozilla.org/en-US/docs/XPCOM_hashtable_guide
We already have a memory reporter for preferences, albeit one that measures bytes of memory rather than counts of things.  It was added in bug 765244.  Looking at that bug would be a good start -- the existing nsIMemoryReporter could be converted into an nsIMemoryMultiReporter in order to report the extra counts.
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee: nobody → wchen
Attachment #802168 - Flags: review?(n.nethercote)
Comment on attachment 802168 [details] [diff] [review]
Add a memory reporter for the pref service.

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

::: modules/libpref/src/Preferences.cpp
@@ +307,5 @@
> +    nsCString& suspect = referentCount.suspectPreferences[i];
> +    uint32_t totalReferentCount = 0;
> +    referentCount.prefCounter.Get(suspect, &totalReferentCount);
> +
> +    nsPrintfCString suspectPath("preference-service-suspect/"

Here you start the path with "preferences-service-suspect/" but below you start with "preference-service/".  Should the prefix be the same?  Or do you explicitly want the suspect ones to be counted separately?
Attachment #802168 - Flags: review?(n.nethercote) → review+
> Here you start the path with "preferences-service-suspect/" but below you
> start with "preference-service/".  Should the prefix be the same?  Or do you
> explicitly want the suspect ones to be counted separately?

Oh, I see you've done exactly what you already did with the observer service.  Ok, should be good as is.
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 802168 [details] [diff] [review]
>
> Here you start the path with "preferences-service-suspect/" but below you
> start with "preference-service/".  Should the prefix be the same?  Or do you
> explicitly want the suspect ones to be counted separately?

I use a different path because I don't want suspect references to be double counted in the totals for preference-service/ and having a different path makes it easier to spot the suspect references in about:memory (it might also be nice to have a "suspicious" reporter kind in the memory reporter to make it even easier to spot suspicious reports).

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