Closed Bug 1115061 Opened 7 years ago Closed 7 years ago

BackgroundHangMonitor is active even when Telemetry is disabled

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: froydnj, Assigned: poiru)

References

Details

(Keywords: perf, power)

Attachments

(1 file, 1 obsolete file)

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.
(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.
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.)
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8544757 - Flags: review?(nfroyd)
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!
Attachment #8544757 - Flags: review?(nfroyd) → review+
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
Attachment #8544757 - Flags: feedback-
(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?
Flags: needinfo?(nchen)
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.
Flags: needinfo?(nchen)
(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?
Flags: needinfo?(birunthan)
(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.
Attachment #8544757 - Attachment is obsolete: true
Flags: needinfo?(birunthan)
Attachment #8547210 - Flags: review?(nfroyd)
Attachment #8547210 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/c6467b1b36a9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.