chromium.py's KILL_TIMEOUT is too short for valgrind

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tony Chang]

In WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium.py, there's a 3 second timeout when closing the DRT process pipe where we wait for the DRT process to shutdown.  When running DRT in valgrind, this takes longer than 3 seconds and we kill the process before getting memory leak information.

Maybe we can somehow share the --time-out-ms flag to increase this value?  I'm not sure if there's some other way to detect that we're running under valgrind to increase this timeout.  Maybe thestig knows.

[Eric Seidel (no email)]

I mean, we could also add back the TOOL_WRAPPER environment if valgrind really needs it. :)

But yes, it seems like a reasonable heuristic that we could just double that value when time_out_ms is larger than default.

[Lei Zhang]

I already fixed the script on the Chromium side to use --wrapper. Just need to fix this bit here.

Can we just use --time-out-ms as the timer value here? The script on the Chromium side is already passing --time-out-ms=200000.

Doubling the timeout value is not enough. I set to 180 seconds locally just to be on the safe side , but I can see what's a good, lower value if needed.

[Eric Seidel (no email)]

Is this generally true when trying to use valgrind with run-webkit-tests on linux/mac?  Will other ports want something like this?

[Lei Zhang]

I don't know if other ports run Valgrind with --time-out-ms=$large_number, but I imagine they would have to given Valgrind's slowness.

[Timur Iskhodzhanov]

> I'm not sure if there's some other way to detect
> that we're running under valgrind to increase this timeout.
Use RUNNING_ON_VALGRIND ?
http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/valgrind/valgrind.h?revision=72632&view=markup

[Timur Iskhodzhanov]

Also, you can add RunningOnValgrind() from http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/dynamic_annotations/dynamic_annotations.c?revision=84975&view=markup
to JavaScriptCore/wtf/DynamicAnnotations.{h,cpp}

[Dirk Pranke]

(In reply to comment #6)
> Also, you can add RunningOnValgrind() from http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/dynamic_annotations/dynamic_annotations.c?revision=84975&view=markup
> to JavaScriptCore/wtf/DynamicAnnotations.{h,cpp}

Unfortunately I don't see how we could access that from the python code.

Do we have any idea how long it does take for DRT to shut down, so we know how long we should be scaling up to?

It seems reasonable to scale up the 3 second timeout based on how much --time-out-ms is scaled up over the default.

[Lei Zhang]

If the current 3 second timeout is meant for a machine like a Z600, then 15 seconds should be fine for Valgrind. (I ran a bunch of tests and see ~8 seconds nromally and doubled it to be safe.)

[Lei Zhang]

Created attachment 108390 [details]
[chromium] Make the layout test script's kill timeout proportional to --time-out-ms

Ping? Would something like this work?

[Dirk Pranke]

Comment on attachment 108390 [details]
[chromium] Make the layout test script's kill timeout proportional to --time-out-ms

Hi, sorry for the delay in getting back to you (I've been backlogged with other things). This looks fine; do you need me to CQ+ it or land it for you?

[Tony Chang]

Comment on attachment 108390 [details]
[chromium] Make the layout test script's kill timeout proportional to --time-out-ms

Needs a changelog :)

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 108390 [details])
> Needs a changelog :)

Whoops :(

[Lei Zhang]

Created attachment 108411 [details]
patch with changelog

Ha, my Chromium checkout checked out Tools/Scripts, but not Tools, thus I couldn't find the ChangeLog and when *shrug*.

[Tony Chang]

Created attachment 108520 [details]
Patch for landing

[Tony Chang]

(In reply to comment #14)
> Created an attachment (id=108520) [details]
> Patch for landing

Fixed the svn root and added the bug link to the changelog.

[WebKit Review Bot]

Comment on attachment 108520 [details]
Patch for landing

Clearing flags on attachment: 108520

Committed r95875: <http://trac.webkit.org/changeset/95875>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.