903290 - Add a memory reporter for the observer service

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

(Bug 903285 is the same thing, but for the prefs service.)

In bug 901416, we saw a case where we registered tons of observers in the observer service.  This surely wasted memory, and it also slowed the device down.

We should add a memory reporter which counts the number of observers registered on the observer service.  Like in bug 903285, it would be helpful to split up the count between 

a) strong observers,
b) weak observers whose referent is still alive, and
c) weak observers whose referent is dead.

Also like in bug 903285, it would be super-awesome to point out observer topics which have more than N listeners, since (if we choose a reasonable N) these are likely leaks.

Comment 1

[Justin Lebar (not reading bugmail)]

If we want to also add a memory reporter which reports the amount of memory used by the observer service, that would be OK with me.  But I don't feel it's totally necessary.

Comment 2

[William Chen [:wchen]]

Attachment: Add a memory reporter for the observer service

This patch just reports referent counts, not the memory.

Comment 3

[William Chen [:wchen]]

Attachment: Add a memory reporter for the observer service

Looks like my last patch fails try because nsMemoryReporterManager uses the nsObserverService on Linux via nsMemoryInfoDumper::Initialize -> FifoWatcher::MaybeCreate -> FdWatcher::Init. This causes an assertion about recursive GetService. I fixed it by putting the registration in a runnable, let me know if there is a better place to register in unfortunate cases like this.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 794155 [details] [diff] [review]
Add a memory reporter for the observer service

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

I've reviewed this from a memory reporter angle, i.e. how you're reporting what you've measured.  But I don't know much about the observer service so I can't say if you've measured it appropriately (e.g. CountReferents()).  Perhaps jlebar could help with that?

It looks pretty good, and the flaws are flaws that are already present in other reporters that I'm in the process of fixing :)

::: xpcom/ds/nsObserverService.cpp
@@ +44,5 @@
>  #else
>    #define LOG(x)
>  #endif /* PR_LOGGING */
>  
> +#define OBSERVER_SERVICE_SUSPECT_REFERENT_COUNT 1000

Can this be a |static const int| member of ObserverServiceMemoryReporter?

@@ +48,5 @@
> +#define OBSERVER_SERVICE_SUSPECT_REFERENT_COUNT 1000
> +
> +namespace mozilla {
> +
> +class ObserverServiceMemoryReporter MOZ_FINAL : public nsIMemoryMultiReporter

Just call it |ObserverServiceReporter| -- we avoid having |Memory| in there in most other reporters (and if there any exceptions, they're on my radar, e.g. via bug 831193).

@@ +56,5 @@
> +        : mService(aService) {}
> +    NS_DECL_ISUPPORTS
> +    NS_DECL_NSIMEMORYMULTIREPORTER
> +protected:
> +    nsObserverService* mService;

I know some other reporters do this, but it's generally unsafe for a reporter to hold a raw pointer to a struct like this -- it's possible that the struct could be destroyed and the reporter doesn't realize and tries to measure it.

The best thing to do for now is to remove this member, and in CollectReports() instead get a handle to the observer service externally in some way.  Often singleton structs like services have a static pointer, but nsObserverService doesn't seem to, so there must be another way.

So it'll end up something like this:

  nsObserverService* os = <get the observer service>
  if (os) {
    <measure and report>
  }

It's still not perfect, and I have plans for a better way, but it'll do for now.

@@ +66,5 @@
> +
> +NS_IMETHODIMP
> +ObserverServiceMemoryReporter::GetName(nsACString& aName)
> +{
> +    aName.AssignLiteral("ObserverServiceMemoryReporter");

The name should have a "foo-bar-baz" form rather than "FooBarBaz", and match as closely as possible the paths reported to the callback.  "observer-service" would be best.

@@ +150,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +
> +    rv = cb->Callback(/* process */ EmptyCString(),
> +        NS_LITERAL_CSTRING("observer-service-referent/strong"),

Would this be better as "observer-service/referent/strong"?  And likewise for the others below.

::: xpcom/ds/nsObserverService.h
@@ +45,5 @@
>    ~nsObserverService(void);
>  
>    bool mShuttingDown;
>    nsTHashtable<nsObserverList> mObserverTopicTable;
> +  nsIMemoryMultiReporter* mReporter;

This should be nsCOMPtr<nsIMemoryMultiReporter>.  I know lots of existing examples don't do that, but I'm converting them at the moment (e.g. in bug 831193).

Comment 5

[Nicholas Nethercote [inactive]]

> I fixed it by putting the registration in a runnable,
> let me know if there is a better place to register in unfortunate cases like
> this.

That's come up before.  See AddPreferencesMemoryReporterRunnable in modules/libpref/src/Preferences.cpp.

Comment 6

[Justin Lebar (not reading bugmail)]

>+    if (aObserverList) {

We prefer early returns to big if statements.

This patch mixes two-space and four-space indentation.

>+        nsTArray<ObserverRef>& observers = aObserverList->mObservers;

Nit: const nsTArray?

>+          SuspectObserver suspect = { aObserverList->GetKey(), total };

Maybe this would be nicer if it used a constructor?

It looks good to me aside from these nits.

Comment 7

[William Chen [:wchen]]

Attachment: Add a memory reporter for the observer service. v2

Comments addressed

Comment 8

[William Chen [:wchen]]

Attachment: v1 diff v2

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 794893 [details] [diff] [review]
Add a memory reporter for the observer service. v2

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

Looks good.  Thanks.

What are typical numbers for these reporters?

::: xpcom/ds/nsObserverService.cpp
@@ +150,5 @@
> +        rv = cb->Callback(/* process */ EmptyCString(),
> +            suspectPath,
> +            nsIMemoryReporter::KIND_OTHER,
> +            nsIMemoryReporter::UNITS_COUNT,
> +            suspect.referentCount,

I often avoid callback repetition like this by defining a local REPORT macro.  See dom/base/nsWindowMemoryReporter.cpp for an example.

Comment 10

[William Chen [:wchen]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 794893 [details] [diff] [review]
> Add a memory reporter for the observer service. v2
> 
> Review of attachment 794893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  Thanks.
> 
> What are typical numbers for these reporters?
> 
Just after starting up an almost vanilla firefox desktop:
190 (100.0%) -- observer-service
└──190 (100.0%) -- referent
   ├──130 (68.42%) ── strong
   └───60 (31.58%) -- weak
       ├──58 (30.53%) ── alive
       └───2 (01.05%) ── dead

After quickly browsing a few web pages:
599 (100.0%) -- observer-service
└──599 (100.0%) -- referent
   ├──438 (73.12%) ── strong
   └──161 (26.88%) -- weak
      ├──151 (25.21%) ── alive
      └───10 (01.67%) ── dead

https://hg.mozilla.org/integration/mozilla-inbound/rev/f36d73856449

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/f36d73856449

Comment 12

[Guilherme Lima]

Just a question about this: the 'strong' number should go back down? Because while I'm browsing, I think I've never seen this number decrease. After hours, it's always something around 20000.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Guilherme Lima from comment #12)
> Just a question about this: the 'strong' number should go back down? Because
> while I'm browsing, I think I've never seen this number decrease. After
> hours, it's always something around 20000.

You were probably seeing the effects of bug 913310.

But yes, it would be expected that the numbers would go down if you e.g. closed a bunch of tabs.