970358 - Disable BackgroundHangMonitor on release/beta

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

For now, we should be getting enough data from Nightly/Aurora channels, and the hang monitor has non-trivial power consumption in certain idle cases, so it makes sense to disable it on Release and Beta.

Comment 1

[Benjamin Smedberg]

This affects 28 and up (from bug 909974) right?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like you could try RELEASE_BUILD defines on this so we don't have to track & disable manually every release -- see https://wiki.mozilla.org/Platform/Channel-specific_build_defines

Comment 3

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

Attachment: Disable BackgroundHangMonitor on release builds (v1)

This patch disables BHM when RELEASE_BUILD is defined (for release/beta). Feel free to suggest other names for the macro :)  It was easier to put ifdef in Telemetry.cpp than to isolate ThreadHangStatsIterator, so I just changed Telemetry.cpp. I only see Telemetry ever using ThreadHangStatsIterator, so I think this is okay.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8373657 [details] [diff] [review]
Disable BackgroundHangMonitor on release builds (v1)

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

r=me with the change(s) below.

::: xpcom/threads/BackgroundHangMonitor.h
@@ +18,5 @@
>  };
>  
> +#ifdef RELEASE_BUILD
> +#define MOZ_DISABLE_BACKGROUND_HANG_MONITOR
> +#endif

Please invert this:

#ifndef RELEASE_BUILD
#define MOZ_ENABLE_BACKGROUND_HANG_MONITOR
#endif

and change all the other conditionals correspondingly.

Comment 5

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

Attachment: Disable BackgroundHangMonitor on release builds (v1.1)

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 8373657 [details] [diff] [review]
> Disable BackgroundHangMonitor on release builds (v1)
> 
> ::: xpcom/threads/BackgroundHangMonitor.h
> @@ +18,5 @@
> >  };
> >  
> > +#ifdef RELEASE_BUILD
> > +#define MOZ_DISABLE_BACKGROUND_HANG_MONITOR
> > +#endif
> 
> Please invert this:
> 
> #ifndef RELEASE_BUILD
> #define MOZ_ENABLE_BACKGROUND_HANG_MONITOR
> #endif
> 
> and change all the other conditionals correspondingly.

Okay, done

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

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

Comment on attachment 8374607 [details] [diff] [review]
Disable BackgroundHangMonitor on release builds (v1.1)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 909974

User impact if declined: Higher power consumption in certain idle cases when using beta/release builds

Testing completed (on m-c, etc.): Locally; m-c

Risk to taking this patch (and alternatives if risky): None; patch simply disables background hang reporting

String or IDL/UUID changes made by this patch: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d17893fc481a
https://hg.mozilla.org/releases/mozilla-beta/rev/3c2d1430b3d1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3c2d1430b3d1