Increase the KillProcess timeout when running under Valgrind

Increase the KillProcess timeout when running under Valgrind
BUG=17453
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89593

[Lei Zhang, 2011-06-17 18:32:26]

LGTM

Let's keep an eye on the tree after it lands and see what happens.

[Timur Iskhodzhanov, 2011-06-17 20:14:07]

Probably I need a "LG" from an OWNER.
Evan, can you have a look?  // looks like Will has problems with his MBP battery

I'll commit on the weekend and see the status

[Timur Iskhodzhanov, 2011-06-17 20:15:49]

Darin, can you please have a look instead of Evan and Will? :) // both are OOO

[darin (slow to review), 2011-06-17 20:54:42]

LGTM

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc
File base/process_util_posix.cc (right):

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#n...
base/process_util_posix.cc:30: #include
"base/third_party/dynamic_annotations/dynamic_annotations.h"
nit: it seems a bit unintuitive that RunningOnValgrind() would be buried in this
header file.  it might help if dynamic_annotations.h declared a namespace :)

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#n...
base/process_util_posix.cc:228: if (RunningOnValgrind()) {
OK...

In my experience 2x might not even be enough :-)

[Timur Iskhodzhanov, 2011-06-17 21:02:46]

Thanks!

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc
File base/process_util_posix.cc (right):

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#n...
base/process_util_posix.cc:30: #include
"base/third_party/dynamic_annotations/dynamic_annotations.h"
That's a good point.
However, currently dynamic_annotations are C headers (not C++) and maybe having
a namespace is an overkill (it mentions Valgrind after all)

On 2011/06/17 20:54:42, darin wrote:
> nit: it seems a bit unintuitive that RunningOnValgrind() would be buried in
this
> header file.  it might help if dynamic_annotations.h declared a namespace :)

http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#n...
base/process_util_posix.cc:228: if (RunningOnValgrind()) {
On 2011/06/17 20:54:42, darin wrote:
> OK...
> 
> In my experience 2x might not even be enough :-)
Totally agree - average Valgrind slowdown is ~20x.

But the idea right now is to increase the timout by just a bit and see how it
changes things.

I expect more leak reports to appear = good (because earlier the children
Valgrind processes were sometimes killed in the middle of the leak check)
but some tests may become slower or even flaky = bad.

That's why I don't want to increase the timeout by 20x immediately :)
After all, maybe 2x or 5x would be enough for 99% of tests.

[darin (slow to review), 2011-06-17 23:31:43]

On Fri, Jun 17, 2011 at 2:02 PM, <timurrrr@chromium.org> wrote:

> Thanks!
>
>
>
> http://codereview.chromium.**org/7204002/diff/2002/base/**
>
process_util_posix.cc<http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc>
> File base/process_util_posix.cc (right):
>
> http://codereview.chromium.**org/7204002/diff/2002/base/**
>
process_util_posix.cc#**newcode30<http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#newcode30>
> base/process_util_posix.cc:30: #include
> "base/third_party/dynamic_**annotations/dynamic_**annotations.h"
> That's a good point.
> However, currently dynamic_annotations are C headers (not C++) and maybe
> having a namespace is an overkill (it mentions Valgrind after all)
>
>
I see.  Convention is usually to prefix function names with Module_ or
something to help
consumers keep track of where various symbols are coming from.

Anyways, this is just food for thought.  In a large codebase like Chromium,
naming conventions
can really help.

-Darin


>
> On 2011/06/17 20:54:42, darin wrote:
>
>> nit: it seems a bit unintuitive that RunningOnValgrind() would be
>>
> buried in this
>
>> header file.  it might help if dynamic_annotations.h declared a
>>
> namespace :)
>
> http://codereview.chromium.**org/7204002/diff/2002/base/**
>
process_util_posix.cc#**newcode228<http://codereview.chromium.org/7204002/diff/2002/base/process_util_posix.cc#newcode228>
> base/process_util_posix.cc:**228: if (RunningOnValgrind()) {
> On 2011/06/17 20:54:42, darin wrote:
>
>> OK...
>>
>
>  In my experience 2x might not even be enough :-)
>>
> Totally agree - average Valgrind slowdown is ~20x.
>
> But the idea right now is to increase the timout by just a bit and see
> how it changes things.
>
> I expect more leak reports to appear = good (because earlier the
> children Valgrind processes were sometimes killed in the middle of the
> leak check)
> but some tests may become slower or even flaky = bad.
>
> That's why I don't want to increase the timeout by 20x immediately :)
> After all, maybe 2x or 5x would be enough for 99% of tests.
>
>
>
http://codereview.chromium.**org/7204002/<http://codereview.chromium.org/7204...
>