i.e., if we have builders for Mac Lion Release and Mac Lion Debug, we should only fetch from the Release bot when rebaselining. The reason is that we only check in one kind of baseline file, and if the two bots produce different results, one of them has to lose. Seems like it should be the Debug bot. I suspect making this change would give us a 2x speed up in rebaselining :). Also, it's not clear if we have *just* a debug bot anywhere, so we could probably simplify this to just "we should only rebaseline release bots"
Seems fine to me. The release bots in general should be more likely to be caught up to tip of tree.
Wait...do we currently grab baselines off both the release and debug bots for the same rebaseline? Sigh. Well...at least it's an easy perf improvement.
We should try to make the code that handles this robust though in case there is in the future a bot that only has a debug bot, e.g. for a while, I think we only had a debug mt lion bot.
Probably the easiest thing to do is to run a filter over the list of bots we're going to try to rebaseline to remove redundant ones.
Created attachment 143112 [details] Patch
Work in progress ... thoughts? Should I finish removing dead tests, write new tests, and land-this as-is, and then optimize the backend in a separate patch? Or should I optimize the back-end to at least de-dup release and debug in this patch? Or take a different approach altogether?
Comment on attachment 143112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:95 > + var tests = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.testName; })); > + var builders = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.builderName; })); > + var suffixes = base.uniquifyArray(base.flattenArray( Doesn't this lose the association between tests, builders, and suffixes? For example, what if I want to rebaseline PNG on Mac but TEXT on Win? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:104 > + net.post(config.kLocalServerURL + '/rebaselineall?' + $.param({ > + 'builders': builders.join(','), > + 'tests': tests.join(','), > + 'suffixes': suffixes.join(',') By the way, another option here is to send the information in the POST body. That way we can send all the builder/test/suffix triples and we won't lose the association between them. The server can sort out what to do with them.
I think losing the association is problematic because it limits what we can do in the future in the UI. It seems better to send the whole list of triples up to the server and then have it process them. For the first patch, we might want to have the server run webkit-patch a bunch of times in a loop. Once we've changed the network protocol, we can optimize the server in a subsequent patch.
(In reply to comment #7) > (From update of attachment 143112 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:95 > > + var tests = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.testName; })); > > + var builders = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.builderName; })); > > + var suffixes = base.uniquifyArray(base.flattenArray( > > Doesn't this lose the association between tests, builders, and suffixes? For example, what if I want to rebaseline PNG on Mac but TEXT on Win? > Yes; this was effectively being lost already on the client side, but you are absolutely correct that it's not as flexible as one could hope for. I was erring on the side of simplicity to start with. I like the post-the-triples idea. I'll implement that and upload another patch.
Comment on attachment 143112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review I agree that this is the right architectural direction (i.e. having more of the logic on the python side). > Tools/ChangeLog:15 > + (.): FYI: I usually delete this lines since they bloat the changelog description. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:104 >> + 'suffixes': suffixes.join(',') > > By the way, another option here is to send the information in the POST body. That way we can send all the builder/test/suffix triples and we won't lose the association between them. The server can sort out what to do with them. I think this is the right thing to do. Lets send this information in the POST body as JSON. > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:181 > + argument_names = "TEST_NAMES..." > + > + def __init__(self): > + options = [ > + optparse.make_option('--builders', action='store', default=None, > + help='List of buildbots to pull from'), > + optparse.make_option('--platform-to-move-to', action='store', default=None, > + help='New platform to move existing baselines to'), > + optparse.make_option('--suffixes', action='store', default=None, > + help='List of output types to rebaseline (as file extension suffixes)'), As per the above, this would take JSON as it's only argument and --no-optimize as it's only flag. I suppose we could make a command that does the current behavior as well, but I don't really see a use-case for this command outside of supporting garden-o-matic directly.
Lol...sorry for the barrage of redundant comments. :)
Created attachment 143146 [details] post a block of json instead, add tests
Comment on attachment 143146 [details] post a block of json instead, add tests I love it when deleting code gets you more functionality
Comment on attachment 143146 [details] post a block of json instead, add tests View in context: https://bugs.webkit.org/attachment.cgi?id=143146&action=review Looks great. One nit below. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:100 > + net.post(config.kLocalServerURL + '/rebaselineall', JSON.stringify(tests), function() { callback() }); function() { callback() } <--- You can just pass callback directly without needing to create a thunk.
Comment on attachment 143146 [details] post a block of json instead, add tests Looks great!
Committed r118234: <http://trac.webkit.org/changeset/118234>
Created attachment 143666 [details] Patch
Comment on attachment 143666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143666&action=review If this doesn't crash in your test case, please add more tests that would trigger the crash. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:160 > + builders_to_ports = {} This variable appears to be unused. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:170 > + fallback_path = port.baseline_search_path() Doesn't this crash? port is local to the loop above.
Created attachment 143908 [details] fix w/ review feedback
Comment on attachment 143908 [details] fix w/ review feedback Oh, much nicer. Thanks.
Comment on attachment 143908 [details] fix w/ review feedback Yay! LGTM as well.
Committed r118447: <http://trac.webkit.org/changeset/118447>