This message is sent surprisingly often. Since it runs pref observers, I don't think it's safe to make it SystemGroup. Some options: - Figure out why it's sent so often and try to reduce the frequency. - If there is a specific pref that changes a lot, use a different IPDL message to update it and try to label that message. - Give it idle priority, which means it doesn't have to be labeled. This does mean that prefs will take longer to take effect in the content process. But I think that should be safe.

I looked at this some today, and I can't imagine why this is getting called so often. SendPreferenceUpdate is only called from ContentParent::Observe for the nsPref:changed notification: http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2764 nsPref:changed (NS_PREFBRANCH_PREFCHANGE_TOPIC) is only ever activated from within libpref: http://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp#1925 and NotifyObserver is only called for callbacks registered with RegisterPriorityCallback: http://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp#1946 which is basically any and all preferences that get passed in to Add*VarCache(). So some cached variable(s) are getting changed thousands and thousands of times according to telemetry? That seems...very very weird, unless there's some addon that's flipping preferences *constantly*. I think we're going to have to collect some telemetry on what prefs we're getting notified for, and then try and figure out why those prefs are changing so frequently. WDYT?

That sounds reasonable. However, it might be good to do some local testing. It might be obvious that there are a couple prefs that are being constantly flipped.

OK, so I missed the NS_PREFBRANCH_PREFCHANGE_TOPIC observation in nsPrefBranch::NotifyObserver: http://dxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#786 which gets called when you're watching a pref branch and somebody sets it via the preferences API. So that part helps explain why we would be seeing these messages more. For comment 2, I did a local build, modifying ContentChild::RecvPreferenceUpdate to dump the name of the received pref, and started the browser. After a couple of minutes, we have about 200 preference updates--but notice many of those are coming because every content process is getting an update for several preferences. The preferences are all over the place, so it doesn't seem likely that we'll be able to split out separate messages. I don't know whether the content processes *really* need to be informed about app.update.lastUpdateTime.* and similar preferences; those seem like things only the parent process needs to know about.

It seems like we could probably make good progress by excluding pref notifications with "lastUpdateTime" in the name. We would have to make sure that's okay to do, but it sounds safe.

(In reply to Nathan Froyd [:froydnj] from comment #3) > The preferences are all over the place, so it doesn't seem likely that we'll > be able to split out separate messages. I don't know whether the content > processes *really* need to be informed about app.update.lastUpdateTime.* and > similar preferences; those seem like things only the parent process needs to > know about. I see two ways to address this, which would probably help remove Msg_PreferenceUpdate from the most-common runnables list. They wouldn't be a complete solution, but they would at least assist in deferring the issue. This is assuming that the evidence in comment 3 actually carries over to the nightly population, and there aren't a lot of addons setting lastUpdated-style preferences. 1) Add new nsIPrefBranch APIs like setSystemIntPref etc., only usable in the parent process, that would *not* broadcast changes to the child process. We'd then modify a number of places to use this new API instead. This would have the advantage of letting module experts take a look at whether the child process actually cared about the value and vetting our strategy. I'm not super excited about adding even more pref APIs, but I think you could make a reasonable case for these. 2) Add a blacklist in ContentParent of pref branches that we wouldn't broadcast to the children.

Did we make a decision here?

We did not. I think billm preferred the blacklist approach, which is probably the simplest option to start with. I can implement a (short) blacklist and we can see what the telemetry numbers look like?

I think that makes sense.

Telemetry has indicated that preference update messages are one of the most common unlabeled runnables. Starting the browser on a clean profile and letting it just sit for a few minutes shows many prefs being broadcast to all content processes that won't really have any effect on said content processes--preference changes having to do when services were last checked for updates and things like that. To cut down on preference update traffic, this patch introduces a simple blacklist against which all preference updates are compared. This should cut down on preference update traffic and reduce the number of unlabeled runnables to worry about. I looked at my list and chose the ones that seemed safest to ignore. I have a try push running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca57256aa0caa192e51a35c468e4b0e50f0d0b79 that will hopefully validate the approach...though I should probably run tests on OS X and Windows just to make sure nothing weird is happening there.

Comment on attachment 8893966 [details] [diff] [review] blacklist some prefs from being broadcast to child processes Review of attachment 8893966 [details] [diff] [review]: ----------------------------------------------------------------- The prefs seem reasonable to me. If the try run looks good, let's land it. ::: dom/ipc/ContentParent.cpp @@ +2755,5 @@ > + }; > +#undef BLACKLIST_ENTRY > + > + for (const auto& entry : sContentPrefBranchBlacklist) { > + if (NS_strncmp(entry.mPrefBranch, aData, entry.mLen) == 0) { It seems like it might be easier to do this testing after the ASCII conversions. Then you could use normal string literals and strcmp, relying on null termination. But this seems fine too.

try ran into some weird mochitest problems. I'm doing more try runs to sort things out.

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/36b4f9c79e0e blacklist some prefs from being broadcast to child processes; r=billm

It seems like the pref runnable is still pretty high (#11 in the list of unlabeled). I guess the next thing to do would be to add a histogram to record which prefs are being sent down.

I guess this patch virtually had no effect due to bug 1426989.