[Chromium-Android] Run ref tests together to avoid expensive driver restarts
authorwangxianzhu@chromium.org <wangxianzhu@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2012 02:45:46 +0000 (02:45 +0000)
committerwangxianzhu@chromium.org <wangxianzhu@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2012 02:45:46 +0000 (02:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=91533

Reviewed by Dirk Pranke.

Though DriverProxy maintains two drivers to support pixel tests and non-pixel tests,
chromium-android uses another way because it can't support multiple drivers.
It restarts the driver when pixel-test mode changes (e.g. when running a ref test after
a normal test in --no-pixel-tests mode). However restarting driver is expensive on
Android (several seconds each time). To reduce the cost, a command line option
'--shard-ref-tests' is added to group ref tests in dedicated shards.
The option is by default enabled on Android.

Will remove the option once DRT supports switching pixel test mode during one run.
(https://bugs.webkit.org/show_bug.cgi?id=91538, https://bugs.webkit.org/show_bug.cgi?id=91539)

* Scripts/webkitpy/layout_tests/controllers/manager.py:
(Manager._shard_tests):
(Manager._shard_in_two):
(Manager._shard_by_directory):
(Manager._run_tests):
* Scripts/webkitpy/layout_tests/controllers/worker.py:
(Worker._update_test_input):
* Scripts/webkitpy/layout_tests/port/chromium_android.py:
(ChromiumAndroidPort.__init__):
* Scripts/webkitpy/layout_tests/run_webkit_tests.py:
(parse_args):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@122914 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
Tools/Scripts/webkitpy/layout_tests/controllers/worker.py
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py

index 77857a0..a9a625f 100644 (file)
@@ -1,3 +1,33 @@
+2012-07-17  Xianzhu Wang  <wangxianzhu@chromium.org>
+
+        [Chromium-Android] Run ref tests together to avoid expensive driver restarts
+        https://bugs.webkit.org/show_bug.cgi?id=91533
+
+        Reviewed by Dirk Pranke.
+
+        Though DriverProxy maintains two drivers to support pixel tests and non-pixel tests,
+        chromium-android uses another way because it can't support multiple drivers.
+        It restarts the driver when pixel-test mode changes (e.g. when running a ref test after
+        a normal test in --no-pixel-tests mode). However restarting driver is expensive on
+        Android (several seconds each time). To reduce the cost, a command line option
+        '--shard-ref-tests' is added to group ref tests in dedicated shards.
+        The option is by default enabled on Android.
+
+        Will remove the option once DRT supports switching pixel test mode during one run.
+        (https://bugs.webkit.org/show_bug.cgi?id=91538, https://bugs.webkit.org/show_bug.cgi?id=91539)
+
+        * Scripts/webkitpy/layout_tests/controllers/manager.py:
+        (Manager._shard_tests):
+        (Manager._shard_in_two):
+        (Manager._shard_by_directory):
+        (Manager._run_tests):
+        * Scripts/webkitpy/layout_tests/controllers/worker.py:
+        (Worker._update_test_input):
+        * Scripts/webkitpy/layout_tests/port/chromium_android.py:
+        (ChromiumAndroidPort.__init__):
+        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
+        (parse_args):
+
 2012-07-17  Don Olmstead  <don.olmstead@am.sony.com>
 
         NRWT The time before server_process kills DRT should be variable
index 325763c..0d07d3a 100644 (file)
@@ -550,7 +550,13 @@ class Manager(object):
     def _test_is_slow(self, test_file):
         return self._expectations.has_modifier(test_file, test_expectations.SLOW)
 
-    def _shard_tests(self, test_files, num_workers, fully_parallel):
+    def _is_ref_test(self, test_input):
+        if test_input.reference_files is None:
+            # Lazy initialization.
+            test_input.reference_files = self._port.reference_files(test_input.test_name)
+        return bool(test_input.reference_files)
+
+    def _shard_tests(self, test_files, num_workers, fully_parallel, shard_ref_tests):
         """Groups tests into batches.
         This helps ensure that tests that depend on each other (aka bad tests!)
         continue to run together as most cross-tests dependencies tend to
@@ -564,30 +570,40 @@ class Manager(object):
         # own class or module. Consider grouping it with the chunking logic
         # in prepare_lists as well.
         if num_workers == 1:
-            return self._shard_in_two(test_files)
+            return self._shard_in_two(test_files, shard_ref_tests)
         elif fully_parallel:
             return self._shard_every_file(test_files)
-        return self._shard_by_directory(test_files, num_workers)
+        return self._shard_by_directory(test_files, num_workers, shard_ref_tests)
 
-    def _shard_in_two(self, test_files):
+    def _shard_in_two(self, test_files, shard_ref_tests):
         """Returns two lists of shards, one with all the tests requiring a lock and one with the rest.
 
         This is used when there's only one worker, to minimize the per-shard overhead."""
         locked_inputs = []
+        locked_ref_test_inputs = []
         unlocked_inputs = []
+        unlocked_ref_test_inputs = []
         for test_file in test_files:
             test_input = self._get_test_input_for_file(test_file)
             if self._test_requires_lock(test_file):
-                locked_inputs.append(test_input)
+                if shard_ref_tests and self._is_ref_test(test_input):
+                    locked_ref_test_inputs.append(test_input)
+                else:
+                    locked_inputs.append(test_input)
             else:
-                unlocked_inputs.append(test_input)
+                if shard_ref_tests and self._is_ref_test(test_input):
+                    unlocked_ref_test_inputs.append(test_input)
+                else:
+                    unlocked_inputs.append(test_input)
+        locked_inputs.extend(locked_ref_test_inputs)
+        unlocked_inputs.extend(unlocked_ref_test_inputs)
 
         locked_shards = []
         unlocked_shards = []
         if locked_inputs:
             locked_shards = [TestShard('locked_tests', locked_inputs)]
         if unlocked_inputs:
-            unlocked_shards = [TestShard('unlocked_tests', unlocked_inputs)]
+            unlocked_shards.append(TestShard('unlocked_tests', unlocked_inputs))
 
         return locked_shards, unlocked_shards
 
@@ -610,7 +626,7 @@ class Manager(object):
 
         return locked_shards, unlocked_shards
 
-    def _shard_by_directory(self, test_files, num_workers):
+    def _shard_by_directory(self, test_files, num_workers, shard_ref_tests):
         """Returns two lists of shards, each shard containing all the files in a directory.
 
         This is the default mode, and gets as much parallelism as we can while
@@ -618,13 +634,18 @@ class Manager(object):
         locked_shards = []
         unlocked_shards = []
         tests_by_dir = {}
+        ref_tests_by_dir = {}
         # FIXME: Given that the tests are already sorted by directory,
         # we can probably rewrite this to be clearer and faster.
         for test_file in test_files:
             directory = self._get_dir_for_test_file(test_file)
             test_input = self._get_test_input_for_file(test_file)
-            tests_by_dir.setdefault(directory, [])
-            tests_by_dir[directory].append(test_input)
+            if shard_ref_tests and self._is_ref_test(test_input):
+                ref_tests_by_dir.setdefault(directory, [])
+                ref_tests_by_dir[directory].append(test_input)
+            else:
+                tests_by_dir.setdefault(directory, [])
+                tests_by_dir[directory].append(test_input)
 
         for directory, test_inputs in tests_by_dir.iteritems():
             shard = TestShard(directory, test_inputs)
@@ -633,6 +654,14 @@ class Manager(object):
             else:
                 unlocked_shards.append(shard)
 
+        for directory, test_inputs in ref_tests_by_dir.iteritems():
+            # '~' to place the ref tests after other tests after sorted.
+            shard = TestShard('~ref:' + directory, test_inputs)
+            if self._test_requires_lock(directory):
+                locked_shards.append(shard)
+            else:
+                unlocked_shards.append(shard)
+
         # Sort the shards by directory name.
         locked_shards.sort(key=lambda shard: shard.name)
         unlocked_shards.sort(key=lambda shard: shard.name)
@@ -714,7 +743,7 @@ class Manager(object):
         interrupted = False
 
         self._printer.write_update('Sharding tests ...')
-        locked_shards, unlocked_shards = self._shard_tests(file_list, int(self._options.child_processes), self._options.fully_parallel)
+        locked_shards, unlocked_shards = self._shard_tests(file_list, int(self._options.child_processes), self._options.fully_parallel, self._options.shard_ref_tests)
 
         # FIXME: We don't have a good way to coordinate the workers so that
         # they don't try to run the shards that need a lock if we don't actually
index 7be6bd0..ae20a8a 100644 (file)
@@ -59,9 +59,16 @@ from webkitpy.common.host_mock import MockHost
 
 
 class ManagerWrapper(Manager):
+    def __init__(self, ref_tests, **kwargs):
+        Manager.__init__(self, **kwargs)
+        self._ref_tests = ref_tests
+
     def _get_test_input_for_file(self, test_file):
         return test_file
 
+    def _is_ref_test(self, test_input):
+        return test_input in self._ref_tests
+
 
 class ShardingTests(unittest.TestCase):
     test_list = [
@@ -77,14 +84,21 @@ class ShardingTests(unittest.TestCase):
         "perf/object-keys.html",
     ]
 
-    def get_shards(self, num_workers, fully_parallel, test_list=None, max_locked_shards=None):
+    ref_tests = [
+        "http/tests/security/view-source-no-refresh.html",
+        "http/tests/websocket/tests/websocket-protocol-ignored.html",
+        "ietestcenter/Javascript/11.1.5_4-4-c-1.html",
+        "dom/html/level2/html/HTMLAnchorElement06.html",
+    ]
+
+    def get_shards(self, num_workers, fully_parallel, shard_ref_tests=False, test_list=None, max_locked_shards=None):
         test_list = test_list or self.test_list
         host = MockHost()
         port = host.port_factory.get(port_name='test')
         port._filesystem = MockFileSystem()
         options = MockOptions(max_locked_shards=max_locked_shards)
-        self.manager = ManagerWrapper(port=port, options=options, printer=Mock())
-        return self.manager._shard_tests(test_list, num_workers, fully_parallel)
+        self.manager = ManagerWrapper(self.ref_tests, port=port, options=options, printer=Mock())
+        return self.manager._shard_tests(test_list, num_workers, fully_parallel, shard_ref_tests)
 
     def test_shard_by_dir(self):
         locked, unlocked = self.get_shards(num_workers=2, fully_parallel=False)
@@ -110,6 +124,31 @@ class ShardingTests(unittest.TestCase):
              TestShard('ietestcenter/Javascript',
                        ['ietestcenter/Javascript/11.1.5_4-4-c-1.html'])])
 
+    def test_shard_by_dir_sharding_ref_tests(self):
+        locked, unlocked = self.get_shards(num_workers=2, fully_parallel=False, shard_ref_tests=True)
+
+        # Note that although there are tests in multiple dirs that need locks,
+        # they are crammed into a single shard in order to reduce the # of
+        # workers hitting the server at once.
+        self.assertEquals(locked,
+            [TestShard('locked_shard_1',
+              ['http/tests/websocket/tests/unicode.htm',
+               'http/tests/xmlhttprequest/supported-xml-content-types.html',
+               'perf/object-keys.html',
+               'http/tests/security/view-source-no-refresh.html',
+               'http/tests/websocket/tests/websocket-protocol-ignored.html'])])
+        self.assertEquals(unlocked,
+            [TestShard('animations',
+                       ['animations/keyframes.html']),
+             TestShard('dom/html/level2/html',
+                       ['dom/html/level2/html/HTMLAnchorElement03.html']),
+             TestShard('fast/css',
+                       ['fast/css/display-none-inline-style-change-crash.html']),
+             TestShard('~ref:dom/html/level2/html',
+                       ['dom/html/level2/html/HTMLAnchorElement06.html']),
+             TestShard('~ref:ietestcenter/Javascript',
+                       ['ietestcenter/Javascript/11.1.5_4-4-c-1.html'])])
+
     def test_shard_every_file(self):
         locked, unlocked = self.get_shards(num_workers=2, fully_parallel=True)
         self.assertEquals(locked,
@@ -142,6 +181,23 @@ class ShardingTests(unittest.TestCase):
                         'ietestcenter/Javascript/11.1.5_4-4-c-1.html',
                         'dom/html/level2/html/HTMLAnchorElement06.html'])])
 
+    def test_shard_in_two_sharding_ref_tests(self):
+        locked, unlocked = self.get_shards(num_workers=1, fully_parallel=False, shard_ref_tests=True)
+        self.assertEquals(locked,
+            [TestShard('locked_tests',
+                       ['http/tests/websocket/tests/unicode.htm',
+                        'http/tests/xmlhttprequest/supported-xml-content-types.html',
+                        'perf/object-keys.html',
+                        'http/tests/security/view-source-no-refresh.html',
+                        'http/tests/websocket/tests/websocket-protocol-ignored.html'])])
+        self.assertEquals(unlocked,
+            [TestShard('unlocked_tests',
+                       ['animations/keyframes.html',
+                        'fast/css/display-none-inline-style-change-crash.html',
+                        'dom/html/level2/html/HTMLAnchorElement03.html',
+                        'ietestcenter/Javascript/11.1.5_4-4-c-1.html',
+                        'dom/html/level2/html/HTMLAnchorElement06.html'])])
+
     def test_shard_in_two_has_no_locked_shards(self):
         locked, unlocked = self.get_shards(num_workers=1, fully_parallel=False,
              test_list=['animations/keyframe.html'])
index 837aea8..378826c 100644 (file)
@@ -83,7 +83,9 @@ class Worker(object):
         self._caller.post('finished_test_list', test_list_name, len(test_inputs), elapsed_time)
 
     def _update_test_input(self, test_input):
-        test_input.reference_files = self._port.reference_files(test_input.test_name)
+        if test_input.reference_files is None:
+            # Lazy initialization.
+            test_input.reference_files = self._port.reference_files(test_input.test_name)
         if test_input.reference_files:
             test_input.should_run_pixel_test = True
         elif self._options.pixel_tests:
index ae430d2..126b318 100644 (file)
@@ -162,6 +162,9 @@ class ChromiumAndroidPort(chromium.ChromiumPort):
         # The Chromium port for Android always uses the hardware GPU path.
         self._options.enable_hardware_gpu = True
 
+        # Shard ref tests so that they run together to avoid repeatedly driver restarts.
+        self._options.shard_ref_tests = True
+
         self._operating_system = 'android'
         self._version = 'icecreamsandwich'
         self._original_governor = None
index 1663416..60db587 100755 (executable)
@@ -420,6 +420,11 @@ def parse_args(args=None):
             help="Don't re-try any tests that produce unexpected results."),
         optparse.make_option("--max-locked-shards", type="int",
             help="Set the maximum number of locked shards"),
+        # For chromium-android to reduce the cost of restarting the driver.
+        # FIXME: Remove the option once per-test arg is supported:
+        # https://bugs.webkit.org/show_bug.cgi?id=91539.
+        optparse.make_option("--shard-ref-tests", action="store_true",
+            help="Run ref tests in dedicated shard(s). Enabled on Android by default."),
     ]))
 
     option_group_definitions.append(("Miscellaneous Options", [