Make rebaselining not use gigabytes of memory.

Make rebaselining not use gigabytes of memory.

The TestExpectations objects we were creating in a loop would consume memory
and never return it. I'm not sure why the memory wasn't getting freed, but reparsing
TestExpectations over and over again is really slow and by far the longest pole
in rebaseline performance. So, fix both issues by not doing this.

Instead, grab all the test_configurations that we want to remove up front and then
remove them all in one go from TestExpectations. This makes the work and memory
cost be O(number of ports) instead of O(number of tests * number of ports).

Also, fix a bug where we would not remove lines for passing tests that were being
rebaselined. I had previously fixed this incorrectly in auto-rebaseline. Instead, fix
it correctly in AbstractParallelRebaselineCommand so that the rebaseline-expectations,
rebaseline-json and rebaseline commands also get the fix.

BUG=379484
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175250

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

I don't feel like I know this code very well, but I"m happy to rslgtm if that's
what you're looking for.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:452: generic_expectations =
TestExpectations(port, tests=tests, include_overrides=False)
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]

I'm fine waiting for Dirk's review. No rush.

eae can patch this locally in the meantime to make his rebaselines not take
days.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:452: generic_expectations =
TestExpectations(port, tests=tests, include_overrides=False)
On 2014/06/01 04:03:42, eseidel wrote:
> 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?

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]

lgtm with a few nits and comments ...

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:1033: for
removal in removals:
Nit: I'd probably change this to 'for test, test_configuration in removals:'

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:446: # We should be able to
update everything in memory, once, and then write the file out a single time.
I think this is you fixing this FIXME, yes?

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:452: generic_expectations =
TestExpectations(port, tests=tests, include_overrides=False)
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.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:458: to_remove.append((test,
test_configuration))
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.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:483: def
_run_in_parallel_and_update_scm(self, commands, extra_lines_to_remove={}):
Is extra_lines_to_remove used in this routine? I don't see it ...

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

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:1033: for
removal in removals:
On 2014/06/01 21:25:33, Dirk Pranke wrote:
> Nit: I'd probably change this to 'for test, test_configuration in removals:'

Indeed. It's clearly been a while since I've written python. :)

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:446: # We should be able to
update everything in memory, once, and then write the file out a single time.
On 2014/06/01 21:25:33, Dirk Pranke wrote:
> I think this is you fixing this FIXME, yes?

Mostly. I updated it.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:452: generic_expectations =
TestExpectations(port, tests=tests, include_overrides=False)
On 2014/06/01 21:25:33, Dirk Pranke wrote:
> 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.

+1

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:458: to_remove.append((test,
test_configuration))
On 2014/06/01 21:25:33, Dirk Pranke wrote:
> 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.

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.

https://codereview.chromium.org/302003009/diff/20001/Tools/Scripts/webkitpy/t...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:483: def
_run_in_parallel_and_update_scm(self, commands, extra_lines_to_remove={}):
On 2014/06/01 21:25:33, Dirk Pranke wrote:
> Is extra_lines_to_remove used in this routine? I don't see it ...

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

[ojan, 2014-06-02 01:41:19]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-06-02 01:41:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ojan@chromium.org/302003009/40001

[commit-bot: I haz the power, 2014-06-02 02:53:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9780)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10093)

[commit-bot: I haz the power, 2014-06-02 03:31:47]

Change committed as 175250