Optimize baselines in parallel.

Tools/Scripts/webkitpy/tool/commands/rebaseline.py

 # Copyright (c) 2010 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 54 matching lines @@
     platform_options = factory.platform_options(use_globs=True)
 
     results_directory_option = optparse.make_option("--results-directory", help="Local results directory to use")
 
     suffixes_option = optparse.make_option("--suffixes", default=','.join(BASELINE_SUFFIX_LIST), action="store",
         help="Comma-separated-list of file types to rebaseline")
 
     def __init__(self, options=None):
         super(AbstractRebaseliningCommand, self).__init__(options=options)
         self._baseline_suffix_list = BASELINE_SUFFIX_LIST
+        self._scm_changes = {'add': [], 'delete': [], 'remove-lines': []}
+
+    def _add_to_scm(self, path):

[Dirk Pranke, 2014/05/30 23:35:11]

I would probably use different names, like "remember_to_add" or something;
"add_to_scm" sounds like it's actually doing the add, and it isn't.

[ojan, 2014/05/31 00:02:56]

I went with add_to_scm_later.

+        self._scm_changes['add'].append(path)
+
+    def _delete_from_scm(self, path):
+        self._scm_changes['delete'].append(path)
 
 
 class BaseInternalRebaselineCommand(AbstractRebaseliningCommand):
     def __init__(self):
         super(BaseInternalRebaselineCommand, self).__init__(options=[
             self.results_directory_option,
             self.suffixes_option,
             optparse.make_option("--builder", help="Builder to pull new baselines from"),
             optparse.make_option("--test", help="Test to rebaseline"),
             ])
-        self._scm_changes = {'add': [], 'remove-lines': []}
-
-    def _add_to_scm(self, path):
-        self._scm_changes['add'].append(path)
 
     def _baseline_directory(self, builder_name):
         port = self._tool.port_factory.get_from_builder_name(builder_name)
         override_dir = builders.rebaseline_override_dir(builder_name)
         if override_dir:
             return self._tool.filesystem.join(port.layout_tests_dir(), 'platform', override_dir)
         return port.baseline_version_dir()
 
     def _test_root(self, test_name):
         return self._tool.filesystem.splitext(test_name)[0]
@@ skipping 125 matching lines @@
         print json.dumps(self._scm_changes)
 
 
 class OptimizeBaselines(AbstractRebaseliningCommand):
     name = "optimize-baselines"
     help_text = "Reshuffles the baselines for the given tests to use as litte space on disk as possible."
     show_in_main_help = True
     argument_names = "TEST_NAMES"
 
     def __init__(self):
-        super(OptimizeBaselines, self).__init__(options=[self.suffixes_option] + self.platform_options)
+        super(OptimizeBaselines, self).__init__(options=[
+            self.suffixes_option,
+            optparse.make_option('--no-modify-scm', action='store_true', default=False, help='Dump SCM commands as JSON instead of '),
+            ] + self.platform_options)
 
     def _optimize_baseline(self, optimizer, test_name):
         for suffix in self._baseline_suffix_list:
             baseline_name = _baseline_name(self._tool.filesystem, test_name, suffix)
-            if not optimizer.optimize(baseline_name):
+            succeeded, files_to_delete, files_to_add = optimizer.optimize(baseline_name)
+            if not succeeded:
                 print "Heuristics failed to optimize %s" % baseline_name
+            return files_to_delete, files_to_add
 
     def execute(self, options, args, tool):
         self._baseline_suffix_list = options.suffixes.split(',')
         port_names = tool.port_factory.all_port_names(options.platform)
         if not port_names:
             print "No port names match '%s'" % options.platform
             return
 
-        optimizer = BaselineOptimizer(tool, port_names)
+        optimizer = BaselineOptimizer(tool, port_names, skip_scm_commands=options.no_modify_scm)
         port = tool.port_factory.get(port_names[0])
         for test_name in port.tests(args):
             _log.info("Optimizing %s" % test_name)
-            self._optimize_baseline(optimizer, test_name)
+            files_to_delete, files_to_add = self._optimize_baseline(optimizer, test_name)
+            for path in files_to_delete:
+                self._delete_from_scm(path)
+            for path in files_to_add:
+                self._add_to_scm(path)
+
+        print json.dumps(self._scm_changes)
 
 
 class AnalyzeBaselines(AbstractRebaseliningCommand):
     name = "analyze-baselines"
     help_text = "Analyzes the baselines for the given tests and prints results that are identical."
     show_in_main_help = True
     argument_names = "TEST_NAMES"
 
     def __init__(self):
         super(AnalyzeBaselines, self).__init__(options=[
@@ skipping 17 matching lines @@
             elif options.missing:
                 self._write("%s: (no baselines found)" % baseline_name)
 
     def execute(self, options, args, tool):
         self._baseline_suffix_list = options.suffixes.split(',')
         port_names = tool.port_factory.all_port_names(options.platform)
         if not port_names:
             print "No port names match '%s'" % options.platform
             return
 
-        self._baseline_optimizer = self._optimizer_class(tool, port_names)
+        self._baseline_optimizer = self._optimizer_class(tool, port_names, skip_scm_commands=False)
         self._port = tool.port_factory.get(port_names[0])
         for test_name in self._port.tests(args):
             self._analyze_baseline(options, test_name)
 
 
 class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
     # not overriding execute() - pylint: disable=W0223
 
     def __init__(self, options=None):
         super(AbstractParallelRebaselineCommand, self).__init__(options=options)
@@ skipping 64 matching lines @@
                     suffixes = ','.join(actual_failures_suffixes)
                     cmd_line = ['--suffixes', suffixes, '--builder', builder, '--test', test]
                     if options.results_directory:
                         cmd_line.extend(['--results-directory', options.results_directory])
                     if options.verbose:
                         cmd_line.append('--verbose')
                     copy_baseline_commands.append(tuple([[path_to_webkit_patch, 'copy-existing-baselines-internal'] + cmd_line, cwd]))
                     rebaseline_commands.append(tuple([[path_to_webkit_patch, 'rebaseline-test-internal'] + cmd_line, cwd]))
         return copy_baseline_commands, rebaseline_commands
 
     def _files_to_add(self, command_results):

[Dirk Pranke, 2014/05/30 23:35:11]

since we're now returning the files to delete as well as the files to add, can
you change this name to something else as well?

[ojan, 2014/05/31 00:02:56]

I called it _serial_commands.

         files_to_add = set()
+        files_to_delete = set()
         lines_to_remove = {}
         for output in [result[1].split('\n') for result in command_results]:
             file_added = False
             for line in output:
                 try:
                     if line:
                         parsed_line = json.loads(line)
                         if 'add' in parsed_line:
                             files_to_add.update(parsed_line['add'])
+                        if 'delete' in parsed_line:
+                            files_to_delete.update(parsed_line['delete'])
                         if 'remove-lines' in parsed_line:
                             for line_to_remove in parsed_line['remove-lines']:
                                 test = line_to_remove['test']
                                 builder = line_to_remove['builder']
                                 if test not in lines_to_remove:
                                     lines_to_remove[test] = []
                                 lines_to_remove[test].append(builder)
                         file_added = True
                 except ValueError:
                     _log.debug('"%s" is not a JSON object, ignoring' % line)
 
             if not file_added:
                 _log.debug('Could not add file based off output "%s"' % output)
 
-        return list(files_to_add), lines_to_remove
+        return list(files_to_add), list(files_to_delete), lines_to_remove
 
     def _optimize_baselines(self, test_prefix_list, verbose=False):
-        # We don't run this in parallel because modifying the SCM in parallel is unreliable.
+        optimize_commands = []
         for test in test_prefix_list:
             all_suffixes = set()
             for builder in self._builders_to_fetch_from(test_prefix_list[test]):
                 all_suffixes.update(self._suffixes_for_actual_failures(test, builder, test_prefix_list[test][builder]))
+
             # FIXME: We should propagate the platform options as well.
-            self._run_webkit_patch(['optimize-baselines', '--suffixes', ','.join(all_suffixes), test], verbose)
+            cmd_line = ['--no-modify-scm', '--suffixes', ','.join(all_suffixes), test]
+            if verbose:
+                cmd_line.append('--verbose')
+
+            path_to_webkit_patch = self._tool.path()
+            cwd = self._tool.scm().checkout_root
+            optimize_commands.append(tuple([[path_to_webkit_patch, 'optimize-baselines'] + cmd_line, cwd]))
+
+            # TODO: add test that shows 'delete' getting populated.

[Dirk Pranke, 2014/05/30 23:35:11]

can we actually write this test now, rather than later? it seems unlikely that
someone will come back and write it later (unless you just wanted to do so in a
separate patch you're already working on?)

otherwise, if we have code coverage of everything and pylint isn't complaining,
I'd probably just omit the TODO.

[ojan, 2014/05/31 00:02:56]

Whoops. I meant to do this. I use TODO instead of FIXME when I intend to do it
before I upload for review. :)

All I really wanted when I wrote that was the BaselineOptimizer tests I already
added.

+        return optimize_commands
 
     def _update_expectations_files(self, lines_to_remove):
         # FIXME: This routine is way too expensive. We're creating N ports and N TestExpectations
         # objects and (re-)writing the actual expectations file N times, for each test we update.
         # We should be able to update everything in memory, once, and then write the file out a single time.
         for test in lines_to_remove:
             for builder in lines_to_remove[test]:
                 port = self._tool.port_factory.get_from_builder_name(builder)
                 path = port.path_to_generic_test_expectations_file()
                 expectations = TestExpectations(port, include_overrides=False)
@@ skipping 23 matching lines @@
         return (SKIP in full_expectations.get_expectations(test) and
                 SKIP not in generic_expectations.get_expectations(test))
 
     def _run_in_parallel_and_update_scm(self, commands):
         command_results = self._tool.executive.run_in_parallel(commands)
         log_output = '\n'.join(result[2] for result in command_results).replace('\n\n', '\n')
         for line in log_output.split('\n'):
             if line:
                 print >> sys.stderr, line  # FIXME: Figure out how to log properly.
 
-        files_to_add, lines_to_remove = self._files_to_add(command_results)
+        files_to_add, files_to_delete, lines_to_remove = self._files_to_add(command_results)

[Dirk Pranke, 2014/05/30 23:35:11]

see note on the naming of "self._files_to_add()", above.

[ojan, 2014/05/31 00:02:56]

Done.

+        if files_to_delete:
+            self._tool.scm().delete_list(files_to_delete)
         if files_to_add:
-            self._tool.scm().add_list(list(files_to_add))
+            self._tool.scm().add_list(files_to_add)
         if lines_to_remove:
             self._update_expectations_files(lines_to_remove)
 
     def _rebaseline(self, options, test_prefix_list):
         for test, builders_to_check in sorted(test_prefix_list.items()):
             _log.info("Rebaselining %s" % test)
             for builder, suffixes in sorted(builders_to_check.items()):
                 _log.debug("  %s: %s" % (builder, ",".join(suffixes)))
 
         copy_baseline_commands, rebaseline_commands = self._rebaseline_commands(test_prefix_list, options)
         if copy_baseline_commands:
             self._run_in_parallel_and_update_scm(copy_baseline_commands)
         if rebaseline_commands:
             self._run_in_parallel_and_update_scm(rebaseline_commands)
-
         if options.optimize:
-            self._optimize_baselines(test_prefix_list, options.verbose)
+            self._run_in_parallel_and_update_scm(self._optimize_baselines(test_prefix_list, options.verbose))
 
     def _suffixes_for_actual_failures(self, test, builder_name, existing_suffixes):
         actual_results = self.builder_data()[builder_name].actual_results(test)
         if not actual_results:
             return set()
         return set(existing_suffixes) & TestExpectations.suffixes_for_actual_expectations_string(actual_results)
 
 
 class RebaselineJson(AbstractParallelRebaselineCommand):
     name = "rebaseline-json"
@@ skipping 397 matching lines @@
                 if options.verbose:
                     rebaseline_command.append('--verbose')
                 # Use call instead of run_command so that stdout doesn't get swallowed.
                 tool.executive.call(rebaseline_command)
             except:
                 traceback.print_exc(file=sys.stderr)
                 # Sometimes git crashes and leaves us on a detached head.
                 tool.scm().checkout_branch(old_branch_name)
 
             time.sleep(self.SLEEP_TIME_IN_SECONDS)