Bug 173116 - webkitpy: Reduce polling in ServerProcess
Summary: webkitpy: Reduce polling in ServerProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-08 15:01 PDT by Jonathan Bedard
Modified: 2017-06-10 12:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.26 KB, patch)
2017-06-08 15:18 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2017-06-09 13:34 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.02 KB, patch)
2017-06-09 16:10 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (9.76 KB, patch)
2017-06-10 12:11 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 2017-06-08 15:01:40 PDT
Every time 'has_crashed' is called, a ServerProcess will poll it's process to determine if a crash has occurred. This issue with this approach is that on some platforms, polling may be an expensive feature. The ServerProcess checks its crash every time a line is accessed from stdout or stderr, even if the data for that line is already cached in Python.  Additionally, the driver checks if the ServerProcess has crashed before reading the next line from either standard error or standard out.

We should reduce the amount of polling ServerProcess does so that it is only polling when polling is actually required.
Comment 1 Jonathan Bedard 2017-06-08 15:18:39 PDT
Created attachment 312351 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-06-08 16:27:33 PDT
<rdar://problem/32661857>
Comment 3 Daniel Bates 2017-06-09 12:55:40 PDT
Comment on attachment 312351 [details]
Patch

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

Jonathan Bedard and I spoke about this issue in-person today (06/09). I am worried that this change may cause a test that crashed to be misclassified among other issues (see remarks below). I suggest that we investigate whether we need to speed up ServerProcess._read() or if it is sufficient to speed up the calling code. Briefly looking, Driver._read_block() is one of the callers. It explicitly calls ServerProcess.has_crashed() (which calls ServerProcess.poll()) twice and implicitly calls ServerProcess.has_crashed() via ServerProcess.{read_stdout_line, read_stderr_line, read_either_stdout_or_stderr_line}(). We should look to see if we can achieve the speed up we need by removing one or both of the calls to ServerProcess.has_crashed() from Driver._read_block().

> Tools/Scripts/webkitpy/port/server_process.py:148
> +            if not self._crashed and poll_result:
> +                _log.debug('Failed to poll process {}.'.format(self.process_name()))
> +                self._crashed = True
> +                self._handle_possible_interrupt()

Could this logic cause side effects for callers that causally call poll()?

> Tools/Scripts/webkitpy/port/server_process.py:331
> +            self.poll()
> +            if self.has_crashed():
> +                return None
> +

It seems weird that we may return buffered output from a process that may have already crashed. How does this effect the processing logic in callers? Could this cause a test that crashed to be misclassified?

> Tools/Scripts/webkitpy/port/server_process.py:346
> +        if self._proc and self._proc.poll() is None:

Notice that self._proc is always non-None when we get to this line.
Comment 4 Jonathan Bedard 2017-06-09 13:34:18 PDT
Created attachment 312483 [details]
Patch
Comment 5 Jonathan Bedard 2017-06-09 13:56:37 PDT
I did some profiling and code investigation.

It is necessary to speed up both Driver._read_block() and ServerProcess._read().  If we only speed up one, we will only cut longer test's execution time in half.  In the particular case I was working with (quicklook/word.html), without any optimizations, the test took 1:52, with only the optimization in Driver._read_block(), the test took 53 seconds.  With both optimizations, the test takes less than a second.

Further more, at least for layout tests, we actually test the ServerProcess after all data is read (on line 208 in driver.py).  This means that there should be no change in crash classification.

The other major usage of server_process is in webkitpy/port/image_diff.py.  ImageDiffer already polls the process before a line is read.  This means that the change in ServerProcess._read() will not effect calls here.  Also note that ImageDiff is run on the Mac when running iOS tests, so does not require an optimization.

To address some of Dan's points in person today (06/09), I moved this optimization out of has_crashed(), as the optimization in Driver._read_block() and ServerProcess._read() give us everything we need.
Comment 6 Ryosuke Niwa 2017-06-09 15:02:05 PDT
Comment on attachment 312483 [details]
Patch

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

> Tools/ChangeLog:13
> +        (Driver._read_block): Rely on output of the ServerProcess to detect a crash or a timeout.

This comment is inaccurate. As we discussed on IRC, we're also relying on ServerProcess._read to poll the process.

> Tools/ChangeLog:17
> +

Please add tests for both changes.
Comment 7 Jonathan Bedard 2017-06-09 16:10:42 PDT
Created attachment 312506 [details]
Patch
Comment 8 Jonathan Bedard 2017-06-09 16:14:31 PDT
*** Bug 88280 has been marked as a duplicate of this bug. ***
Comment 9 Ryosuke Niwa 2017-06-10 00:20:09 PDT
Comment on attachment 312506 [details]
Patch

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

r=me with one more test case.

> Tools/Scripts/webkitpy/port/driver_unittest.py:147
> +    def test_read_block_crashed_process(self):

We should also add another test for making sure that we don't poll the process on every line since that's what we're fixing here.
Comment 10 Daniel Bates 2017-06-10 06:46:33 PDT
Comment on attachment 312506 [details]
Patch

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

> Tools/Scripts/webkitpy/port/driver.py:555
> +        while 1:

while True
Comment 11 Jonathan Bedard 2017-06-10 12:11:50 PDT
Created attachment 312575 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2017-06-10 12:51:17 PDT
Comment on attachment 312575 [details]
Patch for landing

Clearing flags on attachment: 312575

Committed r218055: <http://trac.webkit.org/changeset/218055>
Comment 13 WebKit Commit Bot 2017-06-10 12:51:18 PDT
All reviewed patches have been landed.  Closing bug.