Bug 102389 – garden-o-matic should prefer efl/ over efl-wk1/ and efl-wk2/ when rebaselining

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Raphael Kubo da Costa (:rakuco)]

Looking at commits such as <http://trac.webkit.org/changeset/134723>, <http://trac.webkit.org/changeset/134639>, <http://trac.webkit.org/changeset/134625>, it is possible to see that people doing gardening ended up svn mv'ing some expectations which were in efl/ to efl-wk1/, possibly due to some unwanted garden-o-matic and webkitpy interaction.

Additionally, garden-o-matic created duplicate expectations in efl-wk1/ and efl-wk2/ in <http://trac.webkit.org/changeset/134517> instead of putting everything once in efl/.

This is causing some headache to people gardening the EFL port.

[Dirk Pranke]

Yeah, I've definitely seen this and wondered about it; it must be a bug. I'll try to look into it / fix it today.

[Dirk Pranke]

Okay, so the problem is that webkitpy.layout_tests.builders.all_port_names() returns only one efl entry, for the EFL WK1 bot. 

As a result, when the default baseline optimization algorithm (optimize_by_most_specfic_common_directory) runs, optimize [['LayoutTests/platform/efl-wk1', 'LayoutTests/platform/efl', 'LayoutTests']] results in 'efl-wk1', (the most specific directory for that list of lists). So, results in efl/ get moved down into efl-wk1.

It would be nice if we were smart enough to use efl instead in this case, but that's just not the way this algorithm is intended to work. 

I haven't tried seeing if the fallback algorithm (optimize_by_pushing_results_up) produces a better result (though I think it would). It's an open question if there's any cases where _optimize_by_most_specific outperforms pushing_resutls_up, but Adam spent a lot of time on most_specific so it's reasonable to think that there are.

In other words, I don't want to mess w/ the algorithms in this patch.

The alternative is to make sure we have both the efl-wk1 and efl-wk2 "ports" being considered, so that we only use efl-wk1 and efl-wk2 when they're really needed. I've chosen that approach instead.

I've filed bug 104761 for the fact that I have to "fake" the "virtual" efl-wk2 port in baselineoptimizer for this to work correctly; when I figure out the right way to do this I can clean this up.

[Dirk Pranke]

Created attachment 178948 [details]
Patch

[Eric Seidel (no email)]

I don't really understand this patch. :(

[Dirk Pranke]

Welcome to the magic of the baselineoptimizer! Good thing I waited until Adam left for a long vacation before looking at this bug :)

If you just look at the lines that changed, you'll have no idea what's going on. Here's a more detailed explanation. You can play along by running 'webkit-patch optimize-baselines --efl --suffixes png css1/basic/comments.html' (for example).

The "bug" is that the existing code thinks that, given the choice between putting a baseline in platform/efl and platform/efl-wk1, efl-wk1 is better. Here's why:

All of the computation happens in _find_optimal_placement (line 126). The first thing that routine does is re-order and re-group the test results so that we have a hash mapping unique test result to the list of ports that  share that result (port_names_by_result). In this case, efl has its own result (say, '6cee5b...', which you can see from git hash-object platform/efl/css1/basic/comments-expected.png), so port_names_by_result['6cee5b'] = ['efl'].

We then call _place_results_in_most_specific_directory() for that result. The idea here is that this routine pushes the results as far from the root of the baseline search path tree as it can without changing what result the ports would get. So, for example, mac-snowleopard and mac-lion 's most_specific_directory would be platform/mac-lion; mac-snowleopard and efl, on the other hand, only share the generic directory.

So, the problem is that we have a list of one port (['efl']), and the most specific directory for that port is 'efl-wk1', not 'efl', so we think we should move the baseline from efl to efl-wk1.

This is the result of two problems: (1) the whole "most_specific_directory" idea is in a sense flawed because it pushes things farther away from the root than it needs to, and (2) We aren't accounting for the EFL WK2 port.

I don't particularly want to mess with the algorithm in this patch (that would require a lot more testing and work). 

That means I have to make the BaselineOptimizer think that there are two ports. Unfortunately, 'efl-wk2' isn't a real port name given the way things are currently implemented, and so I can't change builders.all_port_names() to return it. "Fortunately", Adam probably stumbled on this or something like it before and worked around it by adding this "VIRTUAL_PORTS" mapping into the baseline optimizer (cf. the comments on lines 41 and 42). 

It's up to whoever reviews this to decide if the quick-and-dirty fix is okay, or if we should wait for either a new algorithm or a better way to get the full list of ports to optimize (both of which will take more work to implement and review, and potentially destabilize other things).

[Tony Chang]

Comment on attachment 178948 [details]
Patch

FWIW, I get similar problems when trying to rebaseline Apple Mac.  I don't remember the exact problem, but I wanted render tree dumps to just go in mac, but they ended up in a more specific directory instead (maybe mac-lion)?

Actually, can we delete mac-snowleopard?  There are no bots on the waterfall for Apple SnowLeopard.

[Tony Chang]

Hmm, my review comment got lost, let me try again.

[Tony Chang]

Comment on attachment 178948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178948&action=review

> Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:70
> +    # FIXME: Unfortunately, builders.all_port_names() does not actually give you a list of
> +    # all of the ports needed to get coverage on the builders, because we don't yet have a 1:1
> +    # mapping of port names and bot configurations. For example, we only have one 'efl' port name,
> +    # but two efl ports, WK1 and WK2. We either need to modify determine_full_port_name() in the
> +    # port classes to treat efl-wk1 and efl-wk2 as real port names, or provide a different entry point
> +    # in builders to return all of the ports. This might also explain why we have the 'mac-future' and
> +    # 'qt-unknown' hacks above.

Nit: I don't think this comment is helpful. I would write something like, "FIXME: Account for the efl-wk2 port, which isn't returned by builders.all_port_names().  https://bugs.webkit.org/show_bug.cgi?id=104761" and dump extra information into bug 104761.  You could also include this text in the ChangeLog (I guess most of it is already there).

We may want to rename VIRTUAL_PORTS to something more descriptive in a followup patch.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 178948 [details])
> FWIW, I get similar problems when trying to rebaseline Apple Mac.  I don't remember the exact problem, but I wanted render tree dumps to just go in mac, but they ended up in a more specific directory instead (maybe mac-lion)?
>

This wouldn't surprise me; I think any of the -wk2 ports might have problems. Or, it could be you were seeing something slightly different.
 
> Actually, can we delete mac-snowleopard?  There are no bots on the waterfall for Apple SnowLeopard.

Probably.

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 178948 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178948&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:70
> > +    # FIXME: Unfortunately, builders.all_port_names() does not actually give you a list of
> > +    # all of the ports needed to get coverage on the builders, because we don't yet have a 1:1
> > +    # mapping of port names and bot configurations. For example, we only have one 'efl' port name,
> > +    # but two efl ports, WK1 and WK2. We either need to modify determine_full_port_name() in the
> > +    # port classes to treat efl-wk1 and efl-wk2 as real port names, or provide a different entry point
> > +    # in builders to return all of the ports. This might also explain why we have the 'mac-future' and
> > +    # 'qt-unknown' hacks above.
> 
> Nit: I don't think this comment is helpful. I would write something like, "FIXME: Account for the efl-wk2 port, which isn't returned by builders.all_port_names().  https://bugs.webkit.org/show_bug.cgi?id=104761" and dump extra information into bug 104761.  You could also include this text in the ChangeLog (I guess most of it is already there).
> 

Yeah, I agree. I wrote things in the opposite order (comment, then changelog, then bug). I will tighten this up.

> We may want to rename VIRTUAL_PORTS to something more descriptive in a followup patch.

We should get rid of VIRTUAL_PORTS altogether; it indicates that there are bugs elsewhere in the design.

I will probably work on fixing all_port_names() and getting rid of this shortly.

[Dirk Pranke]

Committed r137485: <http://trac.webkit.org/changeset/137485>