|Summary:||chromium.py's KILL_TIMEOUT is too short for valgrind|
|Product:||WebKit||Reporter:||Tony Chang <tony>|
|Component:||Tools / Tests||Assignee:||Lei Zhang <thestig>|
|Severity:||Normal||CC:||abarth, dpranke, eric, mrobinson, ossy, thestig, timurrrr, webkit.review.bot, xan.lopez|
|Version:||528+ (Nightly build)|
Description Tony Chang 2011-09-13 14:21:50 PDT
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.
Comment 1 Eric Seidel (no email) 2011-09-13 14:29:16 PDT
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.
Comment 2 Lei Zhang 2011-09-13 14:33:33 PDT
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.
Comment 3 Eric Seidel (no email) 2011-09-13 14:40:02 PDT
Is this generally true when trying to use valgrind with run-webkit-tests on linux/mac? Will other ports want something like this?
Comment 4 Lei Zhang 2011-09-13 14:42:22 PDT
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.
Comment 5 Timur Iskhodzhanov 2011-09-14 01:11:36 PDT
> 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
Comment 6 Timur Iskhodzhanov 2011-09-14 01:13:27 PDT
Comment 7 Dirk Pranke 2011-09-14 15:15:53 PDT
Comment 8 Lei Zhang 2011-09-14 15:46:39 PDT
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.)
Comment 9 Lei Zhang 2011-09-22 13:39:08 PDT
Created attachment 108390 [details] [chromium] Make the layout test script's kill timeout proportional to --time-out-ms Ping? Would something like this work?
Comment 10 Dirk Pranke 2011-09-22 15:15:08 PDT
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?
Comment 11 Tony Chang 2011-09-22 15:20:21 PDT
Comment on attachment 108390 [details] [chromium] Make the layout test script's kill timeout proportional to --time-out-ms Needs a changelog :)
Comment 12 Dirk Pranke 2011-09-22 15:21:15 PDT
(In reply to comment #11) > (From update of attachment 108390 [details]) > Needs a changelog :) Whoops :(
Comment 13 Lei Zhang 2011-09-22 15:25:03 PDT
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*.
Comment 15 Tony Chang 2011-09-23 13:17:26 PDT
(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.
Comment 16 WebKit Review Bot 2011-09-23 15:46:30 PDT
Comment on attachment 108520 [details] Patch for landing Clearing flags on attachment: 108520 Committed r95875: <http://trac.webkit.org/changeset/95875>
Comment 17 WebKit Review Bot 2011-09-23 15:46:36 PDT
All reviewed patches have been landed. Closing bug.