WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68026
chromium.py's KILL_TIMEOUT is too short for valgrind
https://bugs.webkit.org/show_bug.cgi?id=68026
Summary
chromium.py's KILL_TIMEOUT is too short for valgrind
Tony Chang
Reported
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.
Attachments
[chromium] Make the layout test script's kill timeout proportional to --time-out-ms
(1.56 KB, patch)
2011-09-22 13:39 PDT
,
Lei Zhang
tony
: review-
Details
Formatted Diff
Diff
patch with changelog
(2.12 KB, patch)
2011-09-22 15:25 PDT
,
Lei Zhang
no flags
Details
Formatted Diff
Diff
Patch for landing
(2.33 KB, patch)
2011-09-23 13:16 PDT
,
Tony Chang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
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.
Lei Zhang
Comment 2
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.
Eric Seidel (no email)
Comment 3
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?
Lei Zhang
Comment 4
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.
Timur Iskhodzhanov
Comment 5
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
Timur Iskhodzhanov
Comment 6
2011-09-14 01:13:27 PDT
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
Comment 7
2011-09-14 15:15:53 PDT
(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
Comment 8
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.)
Lei Zhang
Comment 9
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?
Dirk Pranke
Comment 10
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?
Tony Chang
Comment 11
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 :)
Dirk Pranke
Comment 12
2011-09-22 15:21:15 PDT
(In reply to
comment #11
)
> (From update of
attachment 108390
[details]
) > Needs a changelog :)
Whoops :(
Lei Zhang
Comment 13
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*.
Tony Chang
Comment 14
2011-09-23 13:16:25 PDT
Created
attachment 108520
[details]
Patch for landing
Tony Chang
Comment 15
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.
WebKit Review Bot
Comment 16
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
>
WebKit Review Bot
Comment 17
2011-09-23 15:46:36 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug