Bug 173116 – webkitpy: Reduce polling in ServerProcess

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

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 312351 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/32661857>

[Daniel Bates]

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.

[Jonathan Bedard]

Created attachment 312483 [details]
Patch

[Jonathan Bedard]

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.

[Ryosuke Niwa]

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.

[Jonathan Bedard]

Created attachment 312506 [details]
Patch

[Jonathan Bedard]

*** Bug 88280 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

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.

[Daniel Bates]

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

[Jonathan Bedard]

Created attachment 312575 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312575 [details]
Patch for landing

Clearing flags on attachment: 312575

Committed r218055: <http://trac.webkit.org/changeset/218055>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.