Make rebaselining not use gigabytes of memory.

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 350 matching lines @@
             fallback_path = port.baseline_search_path()
             if fallback_path not in builders_to_fallback_paths.values():
                 builders_to_fallback_paths[builder] = fallback_path
         return builders_to_fallback_paths.keys()
 
     def _rebaseline_commands(self, test_prefix_list, options):
         path_to_webkit_patch = self._tool.path()
         cwd = self._tool.scm().checkout_root
         copy_baseline_commands = []
         rebaseline_commands = []
+        lines_to_remove = {}
         port = self._tool.port_factory.get()
 
         for test_prefix in test_prefix_list:
             for test in port.tests([test_prefix]):
                 for builder in self._builders_to_fetch_from(test_prefix_list[test_prefix]):
                     actual_failures_suffixes = self._suffixes_for_actual_failures(test, builder, test_prefix_list[test_prefix][builder])
                     if not actual_failures_suffixes:
+                        # If we're not going to rebaseline the test because it's passing on this
+                        # builder, we still want to remove the line from TestExpectations.
+                        if test not in lines_to_remove:
+                            lines_to_remove[test] = []
+                        lines_to_remove[test].append(builder)
                         continue
 
                     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
+        return copy_baseline_commands, rebaseline_commands, lines_to_remove
 
     def _serial_commands(self, command_results):
         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:
@@ skipping 31 matching lines @@
                 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]))
         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.

[Dirk Pranke, 2014/06/01 21:25:33]

I think this is you fixing this FIXME, yes?

[ojan, 2014/06/02 01:41:15]

Mostly. I updated it.

+        tests = lines_to_remove.keys()
+        to_remove = []
+
+        for port_name in self._tool.port_factory.all_port_names():
+            port = self._tool.port_factory.get(port_name)
+            generic_expectations = TestExpectations(port, tests=tests, include_overrides=False)

[eseidel, 2014/06/01 04:03:42]

Why don't we just always load TestExpectations with model_full_expectations=True
and then have methods to return just the expectations which apply to your port?

[ojan, 2014/06/01 04:26:33]

That might work. I think that should be a separate patch though. Even if we did
that, we'd still want this patch since this is the patch that makes us not do it
once per test. Doing the model_full_expectations would just save doing it once
per port. That's certainly worth doing, but it less valuable than making this
not create O(number of tests) TestExpectations.

[Dirk Pranke, 2014/06/01 21:25:33]

I think Eric's idea is a reasonable suggestion, but I also would want to do that
as a separate change. 

Unfortunately, the test_expectations.py file is easily (IMO) the largest ball of
mud in webkitpy. There's at least two significant large-scale refactors that it
already needs in order to start being somewhat reasonable. This would be a
third. My fear would be that if we made this change without also attempting to
clean up the rest of the file, it would just make a bad thing worse.

[ojan, 2014/06/02 01:41:15]

+1

+            full_expectations = TestExpectations(port, tests=tests, include_overrides=True)
+            for test in tests:
+                if self._port_skips_test(port, test, generic_expectations, full_expectations):
+                    for test_configuration in port.all_test_configurations():
+                        if test_configuration.version == port.test_configuration().version:
+                            to_remove.append((test, test_configuration))

[Dirk Pranke, 2014/06/01 21:25:33]

It seems like lines 454-458 could use a comment explaining what they are doing;
I actually had to stop and think about this for a few minutes to see what's
going on and why it is written the way it is (it's lost to my memory at the
moment, at least).

I think the idea is to remove lines in the file that are skipped in some other
file (and hence are redundant), but I'm not positive.

[ojan, 2014/06/02 01:41:15]

Yeah...I'm not 100% sure what this was for either. I believe the idea is to
remove lines for tests that are skipped since we won't get a baseline off the
bot. Actually, with the fix in this patch to remove lines that are passing on
the bot, I wonder if that obviates the need for this code entirely. I'm not
really sure. Like you, I don't fully remember what this code does. 

I added the best comment I could think of and add a FIXME to look into deleting
this code entirely.

+
         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)
                 for test_configuration in port.all_test_configurations():
                     if test_configuration.version == port.test_configuration().version:
-                        expectationsString = expectations.remove_configuration_from_test(test, test_configuration)
-                self._tool.filesystem.write_text_file(path, expectationsString)
+                        to_remove.append((test, test_configuration))
 
-            for port_name in self._tool.port_factory.all_port_names():
-                port = self._tool.port_factory.get(port_name)
-                generic_expectations = TestExpectations(port, tests=[test], include_overrides=False)
-                if self._port_skips_test(port, test, generic_expectations):
-                    for test_configuration in port.all_test_configurations():
-                        if test_configuration.version == port.test_configuration().version:
-                            expectationsString = generic_expectations.remove_configuration_from_test(test, test_configuration)
-                    generic_path = port.path_to_generic_test_expectations_file()
-                    self._tool.filesystem.write_text_file(generic_path, expectationsString)
+        port = self._tool.port_factory.get()
+        expectations = TestExpectations(port, include_overrides=False)
+        expectationsString = expectations.remove_configurations(to_remove)
+        path = port.path_to_generic_test_expectations_file()
+        self._tool.filesystem.write_text_file(path, expectationsString)
 
-    def _port_skips_test(self, port, test, generic_expectations):
+    def _port_skips_test(self, port, test, generic_expectations, full_expectations):
         fs = port.host.filesystem
         if port.default_smoke_test_only():
             smoke_test_filename = fs.join(port.layout_tests_dir(), 'SmokeTests')
             if fs.exists(smoke_test_filename) and test not in fs.read_text_file(smoke_test_filename):
                 return True
 
-        full_expectations = TestExpectations(port, tests=[test], include_overrides=True)
         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):
+    def _run_in_parallel_and_update_scm(self, commands, extra_lines_to_remove={}):

[Dirk Pranke, 2014/06/01 21:25:33]

Is extra_lines_to_remove used in this routine? I don't see it ...

[ojan, 2014/06/02 01:41:15]

Whoops. Sorry, this was a leftover from an earlier iteration.

         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, files_to_delete, lines_to_remove = self._serial_commands(command_results)
+
         if files_to_delete:
             self._tool.scm().delete_list(files_to_delete)
         if files_to_add:
             self._tool.scm().add_list(files_to_add)
-        if lines_to_remove:
-            self._update_expectations_files(lines_to_remove)
+        return 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)
+        copy_baseline_commands, rebaseline_commands, extra_lines_to_remove = self._rebaseline_commands(test_prefix_list, options)
+        lines_to_remove = {}
+
         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)
+            lines_to_remove = self._run_in_parallel_and_update_scm(rebaseline_commands)
+
+        for test in extra_lines_to_remove:
+            if test in lines_to_remove:
+                lines_to_remove[test] = lines_to_remove[test] + extra_lines_to_remove[test]
+            else:
+                lines_to_remove[test] = extra_lines_to_remove[test]
+
+        if lines_to_remove:
+            self._update_expectations_files(lines_to_remove)
+
         if options.optimize:
             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)
 
 
@@ skipping 350 matching lines @@
 
         try:
             old_branch_name = tool.scm().current_branch()
             tool.scm().delete_branch(self.AUTO_REBASELINE_BRANCH_NAME)
             tool.scm().create_clean_branch(self.AUTO_REBASELINE_BRANCH_NAME)
 
             # If the tests are passing everywhere, then this list will be empty. We don't need
             # to rebaseline, but we'll still need to update TestExpectations.
             if test_prefix_list:
                 self._rebaseline(options, test_prefix_list)
-            # If a test is not failing on the bot, we don't try to rebaseline it, but we still
-            # want to remove the NeedsRebaseline line.
-            self._update_expectations_files(lines_to_remove)
 
             tool.scm().commit_locally_with_message(self.commit_message(author, revision, bugs))
 
             # FIXME: It would be nice if we could dcommit the patch without uploading, but still
             # go through all the precommit hooks. For rebaselines with lots of files, uploading
             # takes a long time and sometimes fails, but we don't want to commit if, e.g. the
             # tree is closed.
             did_finish = self._run_git_cl_command(options, ['upload', '-f'])
 
             if did_finish:
@@ skipping 26 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)