Bug 91533

Summary: [Chromium-Android] Run ref tests together to avoid expensive driver restarts
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Tools / TestsAssignee: Xianzhu Wang <wangxianzhu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, ojan, peter, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Patch for preview
none
Patch
none
Patch
none
Patch
none
Rebased for landing none

Description Xianzhu Wang 2012-07-17 12:06:20 PDT
Driver restarts on Android is expensive (several seconds each time). Switching between normal tests and ref tests requires a driver restart to switch between non-pixel-test mode and pixel-test mode. To reduce the time to run layout tests on Android, we can group all ref tests together.
Comment 1 Dirk Pranke 2012-07-17 12:08:17 PDT
I don't think this is a good idea. reftests are getting increasingly scattered through all the directories.

The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT.
Comment 2 Xianzhu Wang 2012-07-17 12:15:58 PDT
(In reply to comment #1)
> I don't think this is a good idea. reftests are getting increasingly scattered through all the directories.
> 
> The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT.

It'll be great to switch pixel test on/off in one DRT.

However, the issue is significant on Android and I've already created a patch. Can we make it an intermediate solution before we have the pixel-test per-test argument?
Comment 3 Dirk Pranke 2012-07-17 12:19:51 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > I don't think this is a good idea. reftests are getting increasingly scattered through all the directories.
> > 
> > The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT.
> 
> It'll be great to switch pixel test on/off in one DRT.
> 
> However, the issue is significant on Android and I've already created a patch. Can we make it an intermediate solution before we have the pixel-test per-test argument?

if you post the patch I'll be happy to look at it, but I can't guarantee I'll like it :).
Comment 4 Xianzhu Wang 2012-07-17 12:19:59 PDT
Created attachment 152804 [details]
Patch for preview
Comment 5 Dirk Pranke 2012-07-17 12:25:31 PDT
Comment on attachment 152804 [details]
Patch for preview

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

I guess I can live with this as long as it's optional. Can you file a bug to remove this and also file a bug to explicitly track adding per-test arg support to DRT and NRWT, and add a couple of FIXMEs in the code to note the hack?

> Tools/ChangeLog:8
> +        Added a command line option '--shard-ref-tests' to group ref tests in dedicated shards.

Please add a comment here about why we want to group the  tests; you can almost guess form the subject line above but some comment about DriverProxy would probably be helpful.
Comment 6 Tony Chang 2012-07-17 13:46:56 PDT
Out of curiosity, how much is the slowdown?

Maybe it would make more sense to have a DriverPool so we can reuse DRT instances.
Comment 7 Xianzhu Wang 2012-07-17 13:59:37 PDT
Created attachment 152823 [details]
Patch
Comment 8 Xianzhu Wang 2012-07-17 14:05:26 PDT
Created attachment 152825 [details]
Patch
Comment 9 Xianzhu Wang 2012-07-17 14:16:49 PDT
(In reply to comment #8)
> Created an attachment (id=152825) [details]
> Patch

(In reply to comment #5)
> (From update of attachment 152804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152804&action=review
> 
> I guess I can live with this as long as it's optional. Can you file a bug to remove this and also file a bug to explicitly track adding per-test arg support to DRT and NRWT, and add a couple of FIXMEs in the code to note the hack?
>

Filed bug 91538 and bug 91539.
Added FIXME in run_webkit_tests.py parse_args().

> > Tools/ChangeLog:8
> > +        Added a command line option '--shard-ref-tests' to group ref tests in dedicated shards.
> 
> Please add a comment here about why we want to group the  tests; you can almost guess form the subject line above but some comment about DriverProxy would probably be helpful.

Done.

(In reply to comment #6)
> Out of curiosity, how much is the slowdown?
> 
> Maybe it would make more sense to have a DriverPool so we can reuse DRT instances.

Our stat shows that the switching between pixel-test and non-pixel-test occurs about 300 times (total 20~30 minutes) during a full NRWT run.

Actually DriverProxy already pools two drivers (one pixel-test and the other non-pixel test) but we don't use it because we don't support multiple native_test activities on Android. Only one driver/DRT instance is allowed on one device. (We will support multiple devices to run layout tests in parallel in the future.)
Comment 10 Dirk Pranke 2012-07-17 15:52:33 PDT
Comment on attachment 152825 [details]
Patch

close ... please add a test for this in run_webkit_tests_integrationtest.py so we can be sure the code is exercised by test-webkitpy.
Comment 11 Xianzhu Wang 2012-07-17 17:33:57 PDT
Created attachment 152883 [details]
Patch
Comment 12 Xianzhu Wang 2012-07-17 17:34:44 PDT
(In reply to comment #10)
> (From update of attachment 152825 [details])
> close ... please add a test for this in run_webkit_tests_integrationtest.py so we can be sure the code is exercised by test-webkitpy.

Done.
Comment 13 Dirk Pranke 2012-07-17 17:38:54 PDT
Comment on attachment 152883 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:62
> +    def __init__(self, ref_tests, **kwargs):

oh, I meant add an integration test in run_webkit_tests_integrationtest.py that tested that --shard-ref-tests actually ran properly (I don't care as much that the tests are actually sharded in that case, just that we don't error out).

This is fine, too, though.
Comment 14 Xianzhu Wang 2012-07-17 17:43:26 PDT
(In reply to comment #13)
> (From update of attachment 152883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152883&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:62
> > +    def __init__(self, ref_tests, **kwargs):
> 
> oh, I meant add an integration test in run_webkit_tests_integrationtest.py that tested that --shard-ref-tests actually ran properly (I don't care as much that the tests are actually sharded in that case, just that we don't error out).
> 

Sorry for misunderstanding that :)

> This is fine, too, though.

Thanks!
Comment 15 Xianzhu Wang 2012-07-17 17:51:59 PDT
Created attachment 152889 [details]
Rebased for landing
Comment 16 WebKit Review Bot 2012-07-17 19:45:59 PDT
Comment on attachment 152889 [details]
Rebased for landing

Clearing flags on attachment: 152889

Committed r122914: <http://trac.webkit.org/changeset/122914>
Comment 17 WebKit Review Bot 2012-07-17 19:46:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Ryosuke Niwa 2012-07-18 00:16:49 PDT
This caused python tests to fail on some bots:
http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/1026/steps/webkitpy-test/logs/stdio