102389 2012-11-15 07:50:24 -0800 garden-o-matic should prefer efl/ over efl-wk1/ and efl-wk2/ when rebaselining 2012-12-12 11:06:35 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rakuco dpranke abarth dglazkov dpranke eric laszlo.gombos ojan tmpsantos tony oldest_to_newest 768069 0 rakuco 2012-11-15 07:50:24 -0800 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. 768325 1 dpranke 2012-11-15 11:12:24 -0800 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. 789123 2 dpranke 2012-12-11 19:34:55 -0800 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. 789124 3 178948 dpranke 2012-12-11 19:36:31 -0800 Created attachment 178948 Patch 789152 4 eric 2012-12-11 20:53:10 -0800 I don't really understand this patch. :( 789164 5 dpranke 2012-12-11 21:21:19 -0800 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). 789664 6 178948 tony 2012-12-12 10:17:13 -0800 Comment on attachment 178948 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. 789666 7 tony 2012-12-12 10:19:19 -0800 Hmm, my review comment got lost, let me try again. 789669 8 178948 tony 2012-12-12 10:21:48 -0800 Comment on attachment 178948 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. 789687 9 dpranke 2012-12-12 10:35:46 -0800 (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. 789688 10 dpranke 2012-12-12 10:37:01 -0800 (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. 789714 11 dpranke 2012-12-12 11:06:35 -0800 Committed r137485: <http://trac.webkit.org/changeset/137485> 178948 2012-12-11 19:36:31 -0800 2012-12-12 10:21:48 -0800 Patch bug-102389-20121211193353.patch text/plain 3540 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogMTM3NDA0CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZDQyNWVlNmViOTRkOTA4NjYzYTE2ZTVmOWYzZjUzYTBh YWQxMzVhMi4uMWVkY2IwNDIxZGFmZTQzOTUzMTA4MjI1NmJkMDMwNDFmNTY1OTU5MyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIx IEBACiAyMDEyLTEyLTExICBEaXJrIFByYW5rZSAgPGRwcmFua2VAY2hyb21pdW0ub3JnPgogCisg ICAgICAgIGdhcmRlbi1vLW1hdGljIHNob3VsZCBwcmVmZXIgZWZsLyBvdmVyIGVmbC13azEvIGFu ZCBlZmwtd2syLyB3aGVuIHJlYmFzZWxpbmluZworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyMzg5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVGhlIGNvZGUgd2Fzbid0IGF3YXJlIG9mIGJvdGggdGhlIGVm bC13azEgYW5kIGVmbC13azIgdmFyaWFudHMsCisgICAgICAgIGJlY2F1c2UgYWxsX3BvcnRfbmFt ZXMoKSBieSBpdHNlbGYgZG9lc24ndCB0ZWxsIHlvdSBhYm91dCB0aGUKKyAgICAgICAgdmFyaWFu dHMuIFdvcmsgYXJvdW5kIHRoaXMgZm9yIG5vdywgYW5kIGZpbGVkIGJ1ZyAxMDQ3NjEgdG8KKyAg ICAgICAgYWRkcmVzcyB0aGUgdW5kZXJseWluZyBwcm9ibGVtLgorCisgICAgICAgICogU2NyaXB0 cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvYmFzZWxpbmVvcHRpbWl6ZXIucHk6CisgICAgICAg ICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvYmFzZWxpbmVvcHRpbWl6ZXJfdW5p dHRlc3QucHk6CisgICAgICAgIChCYXNlbGluZU9wdGltaXplclRlc3QudGVzdF9lZmwpOgorCisy MDEyLTEyLTExICBEaXJrIFByYW5rZSAgPGRwcmFua2VAY2hyb21pdW0ub3JnPgorCiAgICAgICAg IFJFR1JFU1NJT04ocjEzMzM4MCk6IG5ldyB0ZXN0cyB3aXRob3V0IGV4cGVjdGVkIGZpbGUgcmVw b3J0ZWQgYXMgZmFpbGluZyB0ZXN0cyBvbiB0aGUgYm90cwogICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAxNDY5CiAKZGlmZiAtLWdpdCBhL1Rvb2xzL1Nj cmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L2Jhc2VsaW5lb3B0aW1pemVyLnB5IGIvVG9v bHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvYmFzZWxpbmVvcHRpbWl6ZXIucHkK aW5kZXggZDJkNTNhNTY4NGRiYjhkZGZhMTg1NjBkMjFiODgyMGYxYWYyMTc3NS4uMjIwYjhiN2I1 ZWIyNTYyMWU0MjllNDMzNGVkY2U3NWQ2YThjZTlhZSAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0 cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvYmFzZWxpbmVvcHRpbWl6ZXIucHkKKysrIGIvVG9v bHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvYmFzZWxpbmVvcHRpbWl6ZXIucHkK QEAgLTYwLDYgKzYwLDE1IEBAIF9WSVJUVUFMX1BPUlRTID0gewogICAgICdtYWMtZnV0dXJlJzog WydMYXlvdXRUZXN0cy9wbGF0Zm9ybS9tYWMtZnV0dXJlJywgJ0xheW91dFRlc3RzL3BsYXRmb3Jt L21hYycsICdMYXlvdXRUZXN0cyddLAogICAgICd3aW4tZnV0dXJlJzogWydMYXlvdXRUZXN0cy9w bGF0Zm9ybS93aW4tZnV0dXJlJywgJ0xheW91dFRlc3RzL3BsYXRmb3JtL3dpbicsICdMYXlvdXRU ZXN0cyddLAogICAgICdxdC11bmtub3duJzogWydMYXlvdXRUZXN0cy9wbGF0Zm9ybS9xdC11bmtu b3duJywgJ0xheW91dFRlc3RzL3BsYXRmb3JtL3F0JywgJ0xheW91dFRlc3RzJ10sCisKKyAgICAj IEZJWE1FOiBVbmZvcnR1bmF0ZWx5LCBidWlsZGVycy5hbGxfcG9ydF9uYW1lcygpIGRvZXMgbm90 IGFjdHVhbGx5IGdpdmUgeW91IGEgbGlzdCBvZgorICAgICMgYWxsIG9mIHRoZSBwb3J0cyBuZWVk ZWQgdG8gZ2V0IGNvdmVyYWdlIG9uIHRoZSBidWlsZGVycywgYmVjYXVzZSB3ZSBkb24ndCB5ZXQg aGF2ZSBhIDE6MQorICAgICMgbWFwcGluZyBvZiBwb3J0IG5hbWVzIGFuZCBib3QgY29uZmlndXJh dGlvbnMuIEZvciBleGFtcGxlLCB3ZSBvbmx5IGhhdmUgb25lICdlZmwnIHBvcnQgbmFtZSwKKyAg ICAjIGJ1dCB0d28gZWZsIHBvcnRzLCBXSzEgYW5kIFdLMi4gV2UgZWl0aGVyIG5lZWQgdG8gbW9k aWZ5IGRldGVybWluZV9mdWxsX3BvcnRfbmFtZSgpIGluIHRoZQorICAgICMgcG9ydCBjbGFzc2Vz IHRvIHRyZWF0IGVmbC13azEgYW5kIGVmbC13azIgYXMgcmVhbCBwb3J0IG5hbWVzLCBvciBwcm92 aWRlIGEgZGlmZmVyZW50IGVudHJ5IHBvaW50CisgICAgIyBpbiBidWlsZGVycyB0byByZXR1cm4g YWxsIG9mIHRoZSBwb3J0cy4gVGhpcyBtaWdodCBhbHNvIGV4cGxhaW4gd2h5IHdlIGhhdmUgdGhl ICdtYWMtZnV0dXJlJyBhbmQKKyAgICAjICdxdC11bmtub3duJyBoYWNrcyBhYm92ZS4KKyAgICAn ZWZsLXdrMic6IFsnTGF5b3V0VGVzdHMvcGxhdGZvcm0vZWZsLXdrMicsICdMYXlvdXRUZXN0cy9w bGF0Zm9ybS9lZmwnLCAnTGF5b3V0VGVzdHMnXSwKIH0KIAogCmRpZmYgLS1naXQgYS9Ub29scy9T Y3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9iYXNlbGluZW9wdGltaXplcl91bml0dGVz dC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L2Jhc2VsaW5lb3B0 aW1pemVyX3VuaXR0ZXN0LnB5CmluZGV4IGE1ZmQwNjU2ODY3OTBjODc3Y2M5NWE2ODU3MjNkMDQ2 Nzg0OWU3OWMuLjE2ZDg2NWI4OTUyM2QwZGVjMjEzMTFhMmEzMTYzZTVhOTM4MjU4MTMgMTAwNjQ0 Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L2Jhc2VsaW5lb3B0 aW1pemVyX3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2No ZWNrb3V0L2Jhc2VsaW5lb3B0aW1pemVyX3VuaXR0ZXN0LnB5CkBAIC04Miw2ICs4MiwxNCBAQCBj bGFzcyBCYXNlbGluZU9wdGltaXplclRlc3QodW5pdHRlc3QuVGVzdENhc2UpOgogICAgICAgICAg ICAgJ0xheW91dFRlc3RzL3BsYXRmb3JtL2Nocm9taXVtLXdpbic6ICc0NjJkMDNiOWMwMjVkYjFi MDM5MmQ3NDUzMzEwZGJlZTVmOWE5ZTc0JywKICAgICAgICAgfSkKIAorICAgIGRlZiB0ZXN0X2Vm bChzZWxmKToKKyAgICAgICAgc2VsZi5fYXNzZXJ0T3B0aW1pemF0aW9uKHsKKyAgICAgICAgICAg ICdMYXlvdXRUZXN0cy9wbGF0Zm9ybS9lZmwnOiAnNDYyZDAzYjljMDI1ZGIxYjAzOTJkNzQ1MzMx MGRiZWU1ZjlhOWU3NCcsCisgICAgICAgIH0sIHsKKyAgICAgICAgICAgICdMYXlvdXRUZXN0cy9w bGF0Zm9ybS9lZmwnOiAnNDYyZDAzYjljMDI1ZGIxYjAzOTJkNzQ1MzMxMGRiZWU1ZjlhOWU3NCcs CisgICAgICAgIH0pCisKKwogICAgIGRlZiB0ZXN0X25vX2FkZF9tYWNfZnV0dXJlKHNlbGYpOgog ICAgICAgICBzZWxmLl9hc3NlcnRPcHRpbWl6YXRpb24oewogICAgICAgICAgICAgJ0xheW91dFRl c3RzL3BsYXRmb3JtL21hYyc6ICcyOWExNzE1YTY0NzBkNWRkOTQ4NmExNDJmNjA5NzA4ZGU4NGNk YWM4JywK