Bug 103268 – Make it possible to run performance tests on Chromium Android

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peter Beverloo]

Make it possible to run performance tests on Chromium Android

[Peter Beverloo]

Created attachment 176019 [details]
Patch

[Peter Beverloo]

I'm not happy about having to tell Port what kind of tests are being run, but the ChromiumAndroidPort uses this to forward the local requests to an external HTTP server. The forwarding is done in Android's platform_support, as shown in this patch:

https://codereview.chromium.org/11416182

That patch is required for this one to work, but I'd like to be sure this approach is correct before landing it. Neither is blocked on the other.

[Dirk Pranke]

Comment on attachment 176019 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
> +            if self._port.running_perf_tests():

can you determine the directory from the command, rather than relying on a running_perf_tests() setting? As your fixme notes, it should be possible to run tests outside of LayoutTests/ and you might need a more generic way of handling this (through a directory map or something).

> Tools/Scripts/webkitpy/performance_tests/perftest.py:93
> +                return False

It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:66
> +        self._port.set_running_perf_tests(True)

see comment above; I'm hoping this isn't actually needed.

[Peter Beverloo]

Comment on attachment 176019 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
>> +            if self._port.running_perf_tests():
> 
> can you determine the directory from the command, rather than relying on a running_perf_tests() setting? As your fixme notes, it should be possible to run tests outside of LayoutTests/ and you might need a more generic way of handling this (through a directory map or something).

We could check the path itself:

if 'PerformanceTests' in command:
    ..perf..
else:
    ..layout..

This still wouldn't address the FIXME. When the command lies outside of the LayoutTests directory it simply can't load the page, resulting in an empty DOM.

[Ryosuke Niwa]

Comment on attachment 176019 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:134
>  DEVICE_LAYOUT_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit/LayoutTests/'
> +DEVICE_PERF_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit/PerformanceTests/'

This is crazy. You should be able to dynamically resolve the path by replacing the test path's root by DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit'. Also, you should be using filesystem.join instead.
r- because of this.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:266
> +    def fail_perf_tests_on_stderr(self):
> +        """Chromium for Android can have warnings in the stderr output for not yet
> +        implemented features. There is no reason to fail because of them."""
> +        return False
> +

Please fix all those warnings before turning on performance tests.
Since webkit-perf.appspot.com is completely broken now, there isn't much point in rushing to land this patch.
r- because of this as well.

>> Tools/Scripts/webkitpy/performance_tests/perftest.py:93
>> +                return False
> 
> It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.

By design, a performance test fails if we see any line we don't recognize. That includes any line in stdout and sterr.

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 176019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176019&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
> >> +            if self._port.running_perf_tests():
> > 
> > can you determine the directory from the command, rather than relying on a running_perf_tests() setting? As your fixme notes, it should be possible to run tests outside of LayoutTests/ and you might need a more generic way of handling this (through a directory map or something).
> 
> We could check the path itself:
> 
> if 'PerformanceTests' in command:
>     ..perf..
> else:
>     ..layout..
> 

Right, that's what I had in mind.

> This still wouldn't address the FIXME. When the command lies outside of the LayoutTests directory it simply can't load the page, resulting in an empty DOM.

Yeah, I know, and I'm not asking to fix that now, but, generally speaking, we should try to avoid making assumptions like "all layout tests are under LayoutTests"; that hasn't always been true.

The idea is that the Port class provides all the hooks needed to implement arbitrary lists of tests from anywhere, as long as they're all used consistently in a single Port implementation.

[Dirk Pranke]

(In reply to comment #5)
> > It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.
> 
> By design, a performance test fails if we see any line we don't recognize. That includes any line in stdout and sterr.

Thanks for clarifying. This seems a little fragile and not consistent with how we do layout tests (where stdout fails but stderr generates warnings). Any downside to ignoring stderr output?

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #5)
> > > It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.
> > 
> > By design, a performance test fails if we see any line we don't recognize. That includes any line in stdout and sterr.
> 
> Thanks for clarifying. This seems a little fragile and not consistent with how we do layout tests (where stdout fails but stderr generates warnings). Any downside to ignoring stderr output?

That's because layout tests have expected results. Performance tests don't. If we ignore any lines in stdout, then tests signal a failure on its own. If we ignore stderr, we might miss serious correctness failure.

[Peter Beverloo]

Comment on attachment 176019 [details]
Patch

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

Thank you for the review so far! Please see my replies.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:134
>> +DEVICE_PERF_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit/PerformanceTests/'
> 
> This is crazy. You should be able to dynamically resolve the path by replacing the test path's root by DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit'. Also, you should be using filesystem.join instead.
> r- because of this.

We need to manually forward the requests on the Chromium side. We cannot do this based on the "LayoutTests" or "PerformanceTests" prefix as there also are files in LayoutTests/ that are being pushed to the device (which would end up getting redirected if we'd implement your proposal). See the comment above these lines.

See https://codereview.chromium.org/11416182/ for the PerformanceTests-specific addition.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:266
>> +
> 
> Please fix all those warnings before turning on performance tests.
> Since webkit-perf.appspot.com is completely broken now, there isn't much point in rushing to land this patch.
> r- because of this as well.

There are warnings and there will be warnings. Fixing them all is out of scope here.

>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
>>>> +            if self._port.running_perf_tests():
>>> 
>>> can you determine the directory from the command, rather than relying on a running_perf_tests() setting? As your fixme notes, it should be possible to run tests outside of LayoutTests/ and you might need a more generic way of handling this (through a directory map or something).
>> 
>> We could check the path itself:
>> 
>> if 'PerformanceTests' in command:
>>     ..perf..
>> else:
>>     ..layout..
>> 
>> This still wouldn't address the FIXME. When the command lies outside of the LayoutTests directory it simply can't load the page, resulting in an empty DOM.
> 
> Right, that's what I had in mind.

Ok, I'll upload a new patch with that addition and won't mark it as review until rniwa's concerns have been addressed. This also allowed me to remove some more code from perftestsrunner.py.

>>> Tools/Scripts/webkitpy/performance_tests/perftest.py:93
>>> +                return False
>> 
>> It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.
> 
> By design, a performance test fails if we see any line we don't recognize. That includes any line in stdout and sterr.

The stderr output is separate from stdout. The runner fails on *any* output in stderr. This makes the tests much too fragile on external factors, as the occasional warning will show up for Android. It's not worth failing all performance tests on completely unrelated warnings.

This is the current warning showing up:
[WARNING:proxy_service.cc(885)] PAC support disabled because there is no system implementation

I understand that this is not desirable for all ports, hence why I made it an option. They are still being logged, too.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:66
>> +        self._port.set_running_perf_tests(True)
> 
> see comment above; I'm hoping this isn't actually needed.

Removed.

[Peter Beverloo]

Created attachment 176031 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 176019 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:134
>>> +DEVICE_PERF_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit/PerformanceTests/'
>> 
>> This is crazy. You should be able to dynamically resolve the path by replacing the test path's root by DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit'. Also, you should be using filesystem.join instead.
>> r- because of this.
> 
> We need to manually forward the requests on the Chromium side. We cannot do this based on the "LayoutTests" or "PerformanceTests" prefix as there also are files in LayoutTests/ that are being pushed to the device (which would end up getting redirected if we'd implement your proposal). See the comment above these lines.
> 
> See https://codereview.chromium.org/11416182/ for the PerformanceTests-specific addition.

I understand that. What I'm suggesting is to replace the postfix part (rootDirectory) of your path instead of hard-coding the full path to LayoutTests or PerformanceTests here.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:266
>>> +
>> 
>> Please fix all those warnings before turning on performance tests.
>> Since webkit-perf.appspot.com is completely broken now, there isn't much point in rushing to land this patch.
>> r- because of this as well.
> 
> There are warnings and there will be warnings. Fixing them all is out of scope here.

Then supporting perf. tests on Android port is out of scope.

[Ryosuke Niwa]

If there are specific warnings that are expected to persist on Android port, then we should hard-code those specific warnings in perf_test.py and ignore them manually. Ignoring all stderr is completely out of question.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > > It seems like logging to stderr shouldn't cause a test to fail on any platform, but I don't really know if that's by design or not. We should confirm w/ rniwa.
> > > 
> > > By design, a performance test fails if we see any line we don't recognize. That includes any line in stdout and sterr.
> > 
> > Thanks for clarifying. This seems a little fragile and not consistent with how we do layout tests (where stdout fails but stderr generates warnings). Any downside to ignoring stderr output?
> 
> That's because layout tests have expected results. Performance tests don't. If we ignore any lines in stdout, then tests signal a failure on its own. If we ignore stderr, we might miss serious correctness failure.

I'm not sure I follow you here. I get the part about stdout (I think), but why are we more sensitive to stderr output in perf tests than in layout tests? Wouldn't we be likely to generate the same sort of messages in either case?

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #8)
> > 
> > That's because layout tests have expected results. Performance tests don't. If we ignore any lines in stdout, then tests signal a failure on its own. If we ignore stderr, we might miss serious correctness failure.
> 
> I'm not sure I follow you here. I get the part about stdout (I think), but why are we more sensitive to stderr output in perf tests than in layout tests? Wouldn't we be likely to generate the same sort of messages in either case?

Yes but perf. tests' output is not saved anywhere. The actual result is not compared with anything. So unless we fail the test, we can't tell if tests are passing (no stderr messages) or not.

If there are specific stderr messages we want to ignore, we should be hard-coding that in perf_test.py. However, it's hard to believe that there is no way to remove those superflous stderr messages in the first place.

[Dirk Pranke]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #8)
> > > 
> > > That's because layout tests have expected results. Performance tests don't. If we ignore any lines in stdout, then tests signal a failure on its own. If we ignore stderr, we might miss serious correctness failure.
> > 
> > I'm not sure I follow you here. I get the part about stdout (I think), but why are we more sensitive to stderr output in perf tests than in layout tests? Wouldn't we be likely to generate the same sort of messages in either case?
> 
> Yes but perf. tests' output is not saved anywhere. The actual result is not compared with anything. So unless we fail the test, we can't tell if tests are passing (no stderr messages) or not.
> 

Without having really run the perf tests much or looked at the output, it feels to me like you're trying to be more careful about failures in the perf tests than we are in the layout tests (since we don't have the reference output, as you say). I wonder if this additional level of caution is justified or is just causing more unnecessary work.

That said, I tend to agree that we shouldn't be generating stderr warnings for any tests, period.

Perhaps we can get around this at the moment by filtering out the stderr messages in the android-specific driver run_test method for now and add a fixme to clean up the code later. I tend to agree with Peter that we shouldn't gate running the perf tests at all on android on this.

[Ryosuke Niwa]

(In reply to comment #15)
> (In reply to comment #14)
> > 
> > Yes but perf. tests' output is not saved anywhere. The actual result is not compared with anything. So unless we fail the test, we can't tell if tests are passing (no stderr messages) or not.
> > 
> 
> Without having really run the perf tests much or looked at the output, it feels to me like you're trying to be more careful about failures in the perf tests than we are in the layout tests (since we don't have the reference output, as you say). I wonder if this additional level of caution is justified or is just causing more unnecessary work.

I don't think there is any additional level of caution here. Tests with stderr are reported on the results page for layout tests.

> Perhaps we can get around this at the moment by filtering out the stderr messages in the android-specific driver run_test method for now and add a fixme to clean up the code later. I tend to agree with Peter that we shouldn't gate running the perf tests at all on android on this.

That would be a better solution. This is a design issue with Android DRT that needs to be fixed, and I would not want to see us adding specific code to work around that in perf_test.py or perf_test_runner.py.

[Dirk Pranke]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > 
> > > Yes but perf. tests' output is not saved anywhere. The actual result is not compared with anything. So unless we fail the test, we can't tell if tests are passing (no stderr messages) or not.
> > > 
> > 
> > Without having really run the perf tests much or looked at the output, it feels to me like you're trying to be more careful about failures in the perf tests than we are in the layout tests (since we don't have the reference output, as you say). I wonder if this additional level of caution is justified or is just causing more unnecessary work.
> 
> I don't think there is any additional level of caution here. Tests with stderr are reported on the results page for layout tests.
> 

Yeah, but that's basically ignored and doesn't cause a build step to either fail or generate warnings; if you don't have actual failures or flakiness, we don't generate a results.html at all.

[Adam Barth]

Comment on attachment 176031 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:293
> +        if port.requires_http_server():
> +            port.start_http_server()

This leaks the HTTP server, but that's easy to fix.  Also, you need to pass requires_http to check_build.

[Adam Barth]

> Please fix all those warnings before turning on performance tests.

That seems like an unreasonable precondition for this pathc.

[Ryosuke Niwa]

(In reply to comment #19)
> > Please fix all those warnings before turning on performance tests.
> 
> That seems like an unreasonable precondition for this pathc.

It’s not. We wouldn’t know if a test passed or not if we ignore all stderr. I’m fine with hard-coding specific lines to ignore but ignoring the entirely of stderr is never okay.

[Adam Barth]

> It’s not. We wouldn’t know if a test passed or not if we ignore all stderr.

What does it mean for a perf test to pass or fail?  I just want to know how long it took to run.

> Then supporting perf. tests on Android port is out of scope.

I would like to run perf tests on Android locally.  It's clearly in scope for the WebKit project, regardless of the status of dashboard.

Requiring Peter to fix all warning messages everyone on chromium-android before being able to run the perf test is silly.  There's got to be a solution here that doesn't involve boiling the warning ocean.

[Adam Barth]

Comment on attachment 176031 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:91
>              _log.error('error: %s\n%s' % (self.test_name(), output.error))

We're already logging the error here.

[Ryosuke Niwa]

Comment on attachment 176031 [details]
Patch

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

>> Tools/Scripts/webkitpy/performance_tests/perftest.py:91
>>              _log.error('error: %s\n%s' % (self.test_name(), output.error))
> 
> We're already logging the error here.

Right.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:93
> +            if not self._port.fail_perf_tests_on_stderr():
> +                return False

That’s why we should never add the line below. If you’re only interested in running perf. tests locally then it shouldn’t matter whether run-perf-tests exits non-zero exit code or not.
I don’t object to this patch, if we removed these two lines so that run-perf-tests continues to fail on stderr.

[Adam Barth]

Ok.  I'll post a version that doesn't mess with stderr.  Thanks!

[Adam Barth]

Created attachment 176335 [details]
not yet tested

[Ryosuke Niwa]

(In reply to comment #24)
> Ok.  I'll post a version that doesn't mess with stderr.  Thanks!

Thanks.

[Ryosuke Niwa]

Comment on attachment 176335 [details]
not yet tested

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:677
> +            if 'PerformanceTests' in command:
> +                command = DEVICE_PERF_TESTS_DIR + self._port.relative_test_filename(command.split('PerformanceTests')[1])
> +            else:
> +                command = DEVICE_LAYOUT_TESTS_DIR + self._port.relative_test_filename(command)

As I commented on the previous patch, we should be able to swap-out the root directory part of command by DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit'.

[Adam Barth]

Yes.

[Adam Barth]

Created attachment 176344 [details]
Patch

[Adam Barth]

@rniwa, there's still an issue with stderr, but we can discuss that separately.

[Ryosuke Niwa]

Comment on attachment 176344 [details]
Patch

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

> Tools/ChangeLog:11
> +        on the  Chromium port for Android. There are a few things I had to do

"Â"

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:674
> +            relative_test_filename = fs.relpath(command, fs.dirname(self._port.layout_tests_dir()))

This doesn’t work for performance tests, does it? (since it starts in PerformanceTests).

[Adam Barth]

Created attachment 176346 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 176346 [details]
Patch for landing

Clearing flags on attachment: 176346

Committed r135930: <http://trac.webkit.org/changeset/135930>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

I posted a followup patch dealing with the stderr issue in bug 103462.

[Peter Beverloo]

Thank you for landing the patch, Adam!