716467 - Don't call g_settings_new every time we look up system proxy settings

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Chris Coulson]

I've noticed for the last few days that whilst running Thunderbird Daily, dbus-daemon will use a lot of CPU for a few minutes every now and again and power consumption goes through the roof. When I look at traffic on the session bus, I see thousands of these messages from Thunderbird:

method call sender=:1.527 -> dest=org.freedesktop.DBus serial=34074 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=AddMatch
   string "type='signal',interface='ca.desrt.dconf.Writer',path='/ca/desrt/dconf/Writer/user',arg0path='/system/proxy/'"

What is happening is that each time we look for the system proxy settings, we are calling g_settings_new(). When doing this, the dconf backend sets up a watch on these settings to be notified of change events.

Of course, there might actually be a Thunderbird bug here, but we can probably improve this situation in nsUnixSystemProxySettings by not creating a new GSettings instance (and not setting up a new watch) each time we look up the proxy settings.

Comment 1

[Chris Coulson]

Attachment: Don't call g_settings_new each time we look up system proxy settings

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 586912 [details] [diff] [review]
Don't call g_settings_new each time we look up system proxy settings

Thanks for this.  I was surprised to see how often GetProxyForURI is called,
and it is disturbing that this causes so much CPU use.
The approach in this patch makes sense.

Would mProxySettings be a better name than mGSettingsKeys?

Those who have "mode" set to "manual" are still going to see the same systems,
but fixing that is more involved, and that is a smaller portion of users.  If you fix that also, then it might be a good idea to make nsGSettingsCollection hold a strong reference to mSettings, so that nsUnixSystemProxySettings doesn't need to keep its reference to mGettings.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> it might be a good idea to make
> nsGSettingsCollection hold a strong reference to mSettings, so that
> nsUnixSystemProxySettings doesn't need to keep its reference to mGettings.

I meant the nsGSettingsService not mSettings, but keeping a reference to that would be unnecessary anyway I assume.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> Those who have "mode" set to "manual" are still going to see the same
> systems,

I should just go to bed, but to clarify, I mean "the same symptoms".

Comment 5

[Nicholas Miell]

This bug results in four unnecessary D-Bus method calls per HTTP request instead of two during the entire lifetime of the application (one at startup, one at shutdown).

Comment 6

[Chris Coulson]

Attachment: Don't call g_settings_new each time we look up system proxy settings (v2)

Hi,

I decided to fix the case with manual proxy settings too when addressing your other comment. I'm not sure if this is the best approach though.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 599787 [details] [diff] [review]
Don't call g_settings_new each time we look up system proxy settings (v2)

Looks fine to me, thanks.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62c8bf0d48bb

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/62c8bf0d48bb

Comment 10

[Alex Keybl [:akeybl]]

Tracking for Firefox 11 and 12 since this is a regression in 11. Based upon the perf symptoms detailed here and the lack of dupes, we likely will not want to uplift this fix to Beta prior to release based upon the risk/reward. Please send email to [e-mail] if you disagree. We'd be willing to take this fix for Aurora 12 in the near future, however. Please nominate when ready.

Comment 11

[Alex Keybl [:akeybl]]

Including Mark to see if we've gotten any feedback that would push us to uplift this fix to Beta 12.

Comment 12

[Chris Coulson]

Note, this actually only affects builds that are built with --enable-gio, so it doesn't affect mozilla.org builds. It only affects Linux distributions who are building with gio support and running on GNOME 3, and we're carrying this as a distro patch in Ubuntu already.

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Chris Coulson from comment #12)
> Note, this actually only affects builds that are built with --enable-gio, so
> it doesn't affect mozilla.org builds. It only affects Linux distributions
> who are building with gio support and running on GNOME 3, and we're carrying
> this as a distro patch in Ubuntu already.

Thanks Chris. No need to track on our end in that case.