1173757 - Auto-scale-up IPC timeouts when running on Valgrind

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, task, P4)

Description

[Julian Seward [:jseward]]

I'm less than convinced this is a good idea, but it might well be
convenient sometimes.

Running Fx Desktop on Valgrind sometimes winds up with content
processes getting killed by parents who believe it has timed out.
I suspect FxOS suffers from the same, only worse.

The idea is to provide, from mfbt/MemoryChecking.h, a macro
MOZ_VALGRIND_SLOWDOWN_FACTOR (an integer) and use this to scale up
various IPC related timeouts.

MOZ_VALGRIND_SLOWDOWN_FACTOR will be a constant value 1 for non
--enable-valgrind builds, and it will also be 1 even for
--enable-valgrind builds that are not actually running on Valgrind.
Hence the behaviour of builds is only ever changed when actually
running on Valgrind, regardless of the build config.

When a value other than 1 is returned, a line will be printed in the
Valgrind output for confirmation purposes.  This line never appears
when running natively.

The returned factor is, at least for now, 50.  At least when running
without --track-origins=yes, Memcheck usually achieves a slowdown
much smaller than 50x.  But if it involves visiting a lot of new code
for the first time -- for example, starting a content process -- then
the apparent slowdown can be huge.  So 50 errs on the side of caution.

Comment 1

[Julian Seward [:jseward]]

Attachment: WIP Patch

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8620960 [details] [diff] [review]
WIP Patch

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

::: mfbt/MemoryChecking.cpp
@@ +10,5 @@
> +#if defined(MOZ_VALGRIND)
> +
> +#include "valgrind/valgrind.h"
> +
> +int get_valgrind_slowdown_factor()

Style nit: function name goes on the line following the return type.

Style nit: Call it |GetValgrindSlowdownFactor()|. Actually, |ValgrindSlowdownFactor()| is probably better.

Also, this function should be in the |mozilla| namespace.

@@ +13,5 @@
> +
> +int get_valgrind_slowdown_factor()
> +{
> +   if (!RUNNING_ON_VALGRIND)
> +      return 1;

Style nit: brace single-line blocks.

@@ +15,5 @@
> +{
> +   if (!RUNNING_ON_VALGRIND)
> +      return 1;
> +
> +   const int factor = 100;

Not 50?

@@ +17,5 @@
> +      return 1;
> +
> +   const int factor = 100;
> +   static mozilla::Atomic<int> ctr(0);
> +   if (++ctr == 1) {

The ++ is confusing. Just check if it's zero and set it to 1 in that case?

::: mfbt/MemoryChecking.h
@@ +79,1 @@
>  #elif defined(MOZ_MSAN)

Nit: why two blank lines? Ditto a couple of times below.

@@ +114,5 @@
>  
>  #define MOZ_MAKE_MEM_DEFINED(addr, size) \
>    VALGRIND_MAKE_MEM_DEFINED((addr), (size))
> +
> +int MFBT_API get_valgrind_slowdown_factor();

Nit: |int| after |MFBT_API|.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1173757-ipc-timeouts-2.diff

Rebased, and with fixes for most of the points in comment 2.

Comment 4

[Jens Stutte [:jstutte]]

Hi :mccr8, is this something we still would want to do?

Comment 5

[Andrew McCreight [:mccr8]]

That's a better question for Julian. I assume if he didn't figure out some other work around, he'd have moved forward with this, though.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 7

[Jens Stutte [:jstutte]]

Assuming that if we did not miss it for so long, we won't miss it anytime soon.

Comment 8

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.