Bug 161507 – Fix --no-sample-on-timeout command line argument

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Provide command line arguments to enable/disable sampling on timeout and pick between spindump and sample to reduce memory usage on bots.

[Jonathan Bedard]

Created attachment 287674 [details]
Patch

[Jonathan Bedard]

Created attachment 287676 [details]
Patch

[Dean Johnson]

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)".

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 287697 [details]
Patch

[Alexey Proskuryakov]

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.

[Jonathan Bedard]

Created attachment 287767 [details]
Patch

[Jonathan Bedard]

Created attachment 287770 [details]
Patch

[Alexey Proskuryakov]

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.

[Jonathan Bedard]

Created attachment 287772 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287772 [details]
Patch

Rejecting attachment 287772 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 287772 [details]
Patch

Clearing flags on attachment: 287772

Committed r205351: <http://trac.webkit.org/changeset/205351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

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)

[Jonathan Bedard]

(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.