Make rebaselining not use gigabytes of memory.

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

Index: Tools/Scripts/webkitpy/tool/commands/rebaseline.py
diff --git a/Tools/Scripts/webkitpy/tool/commands/rebaseline.py b/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
index 852c136a6cd6c8cbc7e441271d8b85563a2a9d06..f10d6ae92c3cd39aabf3f048cfb0c0dfec096ad9 100644
--- a/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
+++ b/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
@@ -368,6 +368,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
         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:
@@ -375,6 +376,11 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                 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)
@@ -385,7 +391,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                         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()
@@ -438,38 +444,43 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
         # 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'):
@@ -477,12 +488,12 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                 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()):
@@ -490,11 +501,23 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             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))
 
@@ -865,9 +888,6 @@ class AutoRebaseline(AbstractParallelRebaselineCommand):
             # 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))