Bug 161507 - Fix --no-sample-on-timeout command line argument
Summary: Fix --no-sample-on-timeout command line argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-01 14:01 PDT by Jonathan Bedard
Modified: 2016-09-02 11:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.12 KB, patch)
2016-09-01 14:09 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (7.11 KB, patch)
2016-09-01 14:10 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (7.11 KB, patch)
2016-09-01 16:05 PDT, Jonathan Bedard
lforschler: review+
lforschler: commit-queue?
Details | Formatted Diff | Diff
Patch (9.14 KB, patch)
2016-09-02 09:47 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (5.37 KB, patch)
2016-09-02 10:17 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2016-09-02 10:33 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-09-01 14:01:54 PDT
Provide command line arguments to enable/disable sampling on timeout and pick between spindump and sample to reduce memory usage on bots.
Comment 1 Jonathan Bedard 2016-09-01 14:09:08 PDT
Created attachment 287674 [details]
Patch
Comment 2 Jonathan Bedard 2016-09-01 14:10:24 PDT
Created attachment 287676 [details]
Patch
Comment 3 Dean Johnson 2016-09-01 15:51:37 PDT
Comment on attachment 287676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287676&action=review

Does this also fix the issue where spindump is dumping binary information into logs as well? All this seems to do is allow for forcing sample instead of spindump.

LGTM other than the changes mentioned above.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:145
> +        optparse.make_option("--sample-on-timeout", action="store_true",
> +            dest="sample_on_timeout", help="Don't run sample on timeout (OS X only)"),

The description for this command doesn't seem to follow the command name.

I think it should read "Run sample on timeouts (OS X only)".
Comment 4 Jonathan Bedard 2016-09-01 15:59:38 PDT
(In reply to comment #3)
> Comment on attachment 287676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287676&action=review
> 
> Does this also fix the issue where spindump is dumping binary information
> into logs as well? All this seems to do is allow for forcing sample instead
> of spindump.
>
> ...

No, it does not fix this issue, but this issue is also not a trivial fix.  The binary information can be used to mutate the spindump with command line tools after it is generated, there isn't a flag to simply disable the binary output.  Because of this, an the fact that the binary output is only a third of the spindump and we aren't yet compressing results, it seems that the binary data is a fix that should be delayed, if done at all.
Comment 5 Jonathan Bedard 2016-09-01 16:05:47 PDT
Created attachment 287697 [details]
Patch
Comment 6 Alexey Proskuryakov 2016-09-01 20:19:52 PDT
Comment on attachment 287697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287697&action=review

> Tools/ChangeLog:8
> +        This patch disables spindump by default to conserve memory on bots.  It does the same with the â-sample-on-timeout flag.

I think that we want this enabled by default going forward, and it's not clear if a temporary workaround is needed. Lucas should make the call.
Comment 7 Jonathan Bedard 2016-09-02 09:47:28 PDT
Created attachment 287767 [details]
Patch
Comment 8 Jonathan Bedard 2016-09-02 10:17:12 PDT
Created attachment 287770 [details]
Patch
Comment 9 Alexey Proskuryakov 2016-09-02 10:21:18 PDT
Comment on attachment 287770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287770&action=review

> Tools/ChangeLog:3
> +        Provide additional command line arguments for spindump/sample

The title is no longer accurate.
Comment 10 Jonathan Bedard 2016-09-02 10:33:41 PDT
Created attachment 287772 [details]
Patch
Comment 11 WebKit Commit Bot 2016-09-02 10:34:52 PDT
Comment on attachment 287772 [details]
Patch

Rejecting attachment 287772 [details] from commit-queue.

jbedard@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 12 WebKit Commit Bot 2016-09-02 11:01:12 PDT
Comment on attachment 287772 [details]
Patch

Clearing flags on attachment: 287772

Committed r205351: <http://trac.webkit.org/changeset/205351>
Comment 13 WebKit Commit Bot 2016-09-02 11:01:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Lucas Forschler 2016-09-02 11:28:16 PDT
Comment on attachment 287697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287697&action=review

>> Tools/ChangeLog:8
>> +        This patch disables spindump by default to conserve memory on bots.  It does the same with the â-sample-on-timeout flag.
> 
> I think that we want this enabled by default going forward, and it's not clear if a temporary workaround is needed. Lucas should make the call.

The spin dumps take a significant amount of disk space on build-safari after they are uploaded. I think for now we need to either stop generating them... or at least stop collecting them the test results upload archive. Having a flag with default disabled is a good starting point. This will allow us to turn it back on when we are ready. (meaning that we have a storage solution for results in place)
Comment 15 Jonathan Bedard 2016-09-02 11:32:55 PDT
(In reply to comment #14)
> ...
>
> The spin dumps take a significant amount of disk space on build-safari after
> they are uploaded. I think for now we need to either stop generating them...
> or at least stop collecting them the test results upload archive. Having a
> flag with default disabled is a good starting point. This will allow us to
> turn it back on when we are ready. (meaning that we have a storage solution
> for results in place)

As it turns out, all of our bots are passing --no-sample-on-timeout in the first place.  The memory issues where caused by spindumping on timeout (I believe) and spindumping on  crashes is a relatively rare occurrence, it shouldn't cause that much of a memory issue.

The patch (which has no landed) should stop generating spindumps on timeout (previously, the --no-sample-on-timeout was broken in a number of places, this patch fixes those).  This should fix our memory issues in the short term.