1115061 - BackgroundHangMonitor is active even when Telemetry is disabled

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

Bug summary says it all, really.  We should be making the calls to BackgroundHangMonitor's external methods no-ops when Telemetry's not on.

What this means in practice is that every call to BackgroundHangMonitor::NotifyActivity() causes a thread wakeup to record things that we're never going to use.  Which consumes power, unnecessary work, etc. etc.

Comment 1

[Vladan Djeric (:vladan)]

Dupe of bug 965392?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Vladan Djeric (:vladan) from comment #1)
> Dupe of bug 965392?

Related, I think, but not quite the same.  Even if we fixed bug 965392, we shouldn't be running BHM if we don't have telemetry turned on.

Comment 3

[Biru [:poiru]]

Attachment: Ignore calls to BackgroundHangMonitor::Notify{Activity,Wait} when Telemetry is disabled

Is this about what you had in mind?

(Note that this is not fully tested as I only have OS X handy at the moment. I will compile and test on Windows before landing.)

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8544757 [details] [diff] [review]
Ignore calls to BackgroundHangMonitor::Notify{Activity,Wait} when Telemetry is disabled

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

This works for me!

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8544757 [details] [diff] [review]
Ignore calls to BackgroundHangMonitor::Notify{Activity,Wait} when Telemetry is disabled

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

::: xpcom/threads/HangMonitor.cpp
@@ +93,5 @@
>      }
>    }
>  #endif
> +
> +  BackgroundHangMonitor::SetShouldHandleNotifications(timeout > 0);

I'm not sure this is what's intended.

timeout reflects the "hangmonitor.timeout" pref, which doesn't indicate whether or not telemetry is enabled. In fact, "hangmonitor.timeout" is always 0 these days [1].

What we want is to disable BackgroundHangMonitor when telemetry itself is disabled, which usually tests !Telemetry::CanRecord().

[1] http://mxr.mozilla.org/mozilla-central/search?string=hangmonitor.timeout

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jim Chen [:jchen] from comment #5)
> timeout reflects the "hangmonitor.timeout" pref, which doesn't indicate
> whether or not telemetry is enabled. In fact, "hangmonitor.timeout" is
> always 0 these days [1].

But if hangmonitor.timeout is zero, we set the timeout if telemetry is enabled, which is what we want.  (I'm not really sure it's worth trying to work with non-zero hangmonitor.timeout values...maybe we should just remove the preference entirely?)

Or are you objecting because we really should be checking telemetry enabled-ness directly?  What do you see that buying us, rather than using hangmonitor.timeout as a useful proxy?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Sorry, I should have been more clear.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> (In reply to Jim Chen [:jchen] from comment #5)
> > timeout reflects the "hangmonitor.timeout" pref, which doesn't indicate
> > whether or not telemetry is enabled. In fact, "hangmonitor.timeout" is
> > always 0 these days [1].
>
> But if hangmonitor.timeout is zero, we set the timeout if telemetry is
> enabled, which is what we want.

This only happens if REPORT_CHROME_HANGS is defined, and REPORT_CHROME_HANGS is only defined on Windows. So this patch effectively disables BHM on non-Windows platforms.

> Or are you objecting because we really should be checking telemetry
> enabled-ness directly?  What do you see that buying us, rather than using
> hangmonitor.timeout as a useful proxy?

I also think it's a bit confusing to use "hangmonitor.timeout" as a bool pref, because it doesn't control the timeout values for BHM, but merely turns it on or off.

Again, from my experience, Telemetry::CanRecord() is what other code uses to decide whether to record telemetry data, so I'd recommend using that in BackgroundHangMonitor.cpp, instead of keeping our own can-record variable.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Jim Chen [:jchen] from comment #7)
> Sorry, I should have been more clear.

Thank you for the explanation!

> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> > But if hangmonitor.timeout is zero, we set the timeout if telemetry is
> > enabled, which is what we want.
> 
> This only happens if REPORT_CHROME_HANGS is defined, and REPORT_CHROME_HANGS
> is only defined on Windows. So this patch effectively disables BHM on
> non-Windows platforms.

Ah.  That's not so good.  I didn't review closely enough. :(

> Again, from my experience, Telemetry::CanRecord() is what other code uses to
> decide whether to record telemetry data, so I'd recommend using that in
> BackgroundHangMonitor.cpp, instead of keeping our own can-record variable.

Fair enough.  Birunthan, can you update the patch?

Comment 9

[Biru [:poiru]]

Attachment: Ignore calls to BackgroundHangMonitor::Notify{Activity,Wait} when Telemetry is disabled

(In reply to Jim Chen [:jchen] from comment #7)
> Again, from my experience, Telemetry::CanRecord() is what other code uses to
> decide whether to record telemetry data, so I'd recommend using that in
> BackgroundHangMonitor.cpp, instead of keeping our own can-record variable.

Thanks for the clarification!

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> Fair enough.  Birunthan, can you update the patch?

Done.

Comment 10

[Biru [:poiru]]

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

Comment 11

[Carsten Book [:Tomcat]]

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