1348886 - [meta] Cut down on the number of xpcom-shutdown observers

Status: NEW

(Core :: General, enhancement)

Description

[:Gijs (he/him)]

It seems for every tab we create one or more shutdown observers (even if only about:blank is loaded). From a breakpoint in addobserver, I can see at least:

the event queue adds one:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/xpcom/threads/ThrottledEventQueue.cpp#239-240

(We seem to create one of these for each tab, but then also create additional ones through Promise.jsm's chrome promise worker.)

the event state manager adds one (for each tab, I think):

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/dom/events/EventStateManager.cpp#361

The URL classifier code seems to spawn one for every G_Alarm() invocation ( https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/url-classifier/content/moz/alarm.js#54 ) and also unnecessarily creates shortlived prefbranch instances which add their own shutdown observers ( https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#516-517 ). Generally, the url classifier code seems to be adding/removing various shutdown observers (also spotted profile teardown ones) somewhat willy-nilly.


nsBaseWidget adds one (for each chrome/toplevel/OS window):

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/widget/nsBaseWidget.cpp#187

etc.

There's probably more but these seem to be the most frequently encountered when just opening tabs/windows.

It also seems quite a number of these remove themselves when they are called for shutdown. My (very dated!) understanding was that we shouldn't be doing that, as it just needlessly increases the amount of work we do at shutdown. I could be wrong? :-)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Is there some runtime measurement that is driving this?  Or are you just trying to reduce the number of observers for its own sake?

Comment 2

[:Gijs (he/him)]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #1)
> Is there some runtime measurement that is driving this?  Or are you just
> trying to reduce the number of observers for its own sake?

See bug 1195386. The TL;DR here is that after e10s especially, a bunch of code that used to have 1 observer per process or per window ended up having 1 observer per tab (and in some cases then checking "is my window the window for which I'm getting this observer notification), which is obviously bad for users who have a lot of tabs. xpcom-shutdown simply tops the "how many observers for topic X do we have" list. Cutting this list down should be beneficial to shutdown times and memory usage, but I wouldn't be able to tell you by how much.

Comment 3

[:Gijs (he/him)]

Oh, one more note: my understanding is that for Quantum/Photon we're interested in cutting shutdown and window close time, for which this also seemed relevant, which is why I didn't ignore it (even if it's not e10s/per-tab-observer-specific).

Comment 4

[(no longer active)]

Do you have a profile where these are showing up?

Comment 5

[(no longer active)]

(I meant, besides bug 1195386, that is...)

Comment 6

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari (busy) from comment #4)
> Do you have a profile where these are showing up?

Nope. It's trivial to see them crop up in about:memory's "observer-service-suspect" category, but otherwise, no. To be fair, I also haven't tried at all.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs from comment #6)
> (In reply to :Ehsan Akhgari (busy) from comment #4)
> > Do you have a profile where these are showing up?
> 
> Nope. It's trivial to see them crop up in about:memory's
> "observer-service-suspect" category, but otherwise, no. To be fair, I also
> haven't tried at all.

And actually, thinking about this some more - how would I get a profile of the impact of shutdown observers? I can't use the profiler add-on, and last I tried profiling with an output file and environment variables was broken (the JSON files it generates are cut off in the middle and therefore invalid).

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to :Gijs from comment #0)
> It also seems quite a number of these remove themselves when they are called
> for shutdown. My (very dated!) understanding was that we shouldn't be doing
> that, as it just needlessly increases the amount of work we do at shutdown.
> I could be wrong? :-)

Yeah, they should be removed because they are redundant. nsObserverService will remove all observers on XPCOM shutdown.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Gijs, do you want to keep this meta bug open? (no activity and no deps)

Comment 10

[:Gijs (he/him)]

I think there's something to be gained here, I'll try and get something together later this week / early next week.