1442603 - Background Hang Reporter: increase timeouts when running on Valgrind

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Julian Seward [:jseward]]

If we're running on Valgrind, we'll be making forward progress at a
rate of somewhere between 1/25th and 1/50th of normal.  This causes the
BHR to capture a lot of stacks, which slows us down even more.  As an
attempts to avoid the worst of this, scale up all presented timeouts by
a factor of ten, and add two seconds so as to impose a two second floor
on all timeouts.

Comment 1

[Julian Seward [:jseward]]

Possibly related to (viz, is possibly the cause of) bug 1415917.

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1442603-1.diff

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8955482 [details] [diff] [review]
bug1442603-1.diff

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

I'm fine with this

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1442603-3t.diff

Comment 5

[Nika Layzell [:nika] (ni? for response)]

IIRC you were also planning to add a nsThread which the processing happens on? I'd like to review the changes together.

Comment 6

[Julian Seward [:jseward]]

(In reply to Nika Layzell [:mystor] from comment #5)
Done, in bug 1443499.

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1442603-4.diff

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957572 [details] [diff] [review]
bug1442603-4.diff

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

::: toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp
@@ +689,5 @@
> +    if (aTimeoutMs !=  BackgroundHangMonitor::kNoTimeout) {
> +      aTimeoutMs *= scaleUp;
> +      aTimeoutMs += extraMs;
> +    }
> +    if (aMaxTimeoutMs !=  BackgroundHangMonitor::kNoTimeout) {

nit: there looks like there is some weird spacing in this condition and the condition above?

@@ +699,1 @@
>    if (!BackgroundHangManager::sDisabled && !mThread) {

nit: Please add a newline before this line.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e71240c073d
Background Hang Reporter: increase timeouts when running on Valgrind.  r=mystor.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4e71240c073d

Comment 11

[Julian Seward [:jseward]]

Comment on attachment 8957572 [details] [diff] [review]
bug1442603-4.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

 For automation runs on Valgrind, Valgrind slows down progress so much
 that the Background Hang Reporter kills the process unnecessarily,
 leading the test to fail.  This patch increases the BHR timeouts
 only for builds which are configured --enable-valgind and which are actually
 running on Valgrind.

User impact if declined: 

 Zero, because it requires (1) config with --enable-valgind and (2) actually
 running on Valgrind.

Fix Landed on Version:

 61 

Risk to taking this patch (and alternatives if risky): 

 Very low risk (no end-user effect)

String or UUID changes made by this patch: 

 None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8957572 [details] [diff] [review]
bug1442603-4.diff

Per bug 1443499 comment 9, taking for ESR60.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/843876c2ac8d