garden-o-matic should rebaseline baselines in parallel
Created attachment 143951 [details] Patch
copying from the email thread ... now that I've added a "rebaselineall" entry point to garden-o-matic, the next obvious step to speed things up on the back end is to fetch baselines in parallel. Using a test case of fetching 1 file that needs baselines on all 8 chromium configurations, over my fast network connection to the bots and my beefy mac pro, it takes about 20 seconds to rebaseline the test. This breaks down to about 12 seconds to fetch the baselines, 4 to add them to repo (git add), and 4 to optimize the baselines. Fetching the baselines in parallel scales more or less linearly, so that drops to 1.5s, and changing git add to accept a batch seems to give an O(n) -> O(1) speedup, at least for small n, so there's lots of room for improvement. I haven't looked at speeding up optimize yet. I've posted my work-in-progress for review. I had to modify rebaseline-test to not add the file to git, because it turns out git doesn't support concurrent adds (the add command will fail if there's another add running).
another option in the design would be to add a webkit-patch rebaseline-batch command and push the logic lower; this would potentially be useful if we wanted some other front end or tool to be able to rebaseline a bunch of tests at once other than garden-o-matic. We should think about rebaselining from results.html here, for example ...
Comment on attachment 143951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143951&action=review This looks much cleaner than I expected it to. Tests + cleaning up the patch (e.g. removing prints) is all this needs. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:163 > + def add(self, path=None, return_exit_code=False, paths=None): This change is fine, but you'll need to modify svn.py as well. Instead of adding a new parameter you could change paths to path and have it take a string or a list of string. I'm on the fence as to whether that's better though. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:201 > + # optimizing baselines is fast and involves only local file operations, so we don't bother running this in parallel. I expect this command could benefit from the same optimization as above, i.e. splitting SCM commands from file-system commands and then running all the adds/deletes as a single operation for each. Totally a separate patch though. Just commenting that this could be a FIXME instead.
Or adding --verbose logging for the times instead of the prints would be fine too.
Comment on attachment 143951 [details] Patch Sure, I will clean up everything, this was just to demonstrate the approach. It's a bit unfortunate that we have to jump through some hoops on the back end to re-implement the dataflow we had in garden-o-matic through the async command chaining.
(In reply to comment #6) > (From update of attachment 143951 [details]) > Sure, I will clean up everything, this was just to demonstrate the approach. > > It's a bit unfortunate that we have to jump through some hoops on the back end to re-implement the dataflow we had in garden-o-matic through the async command chaining. From a purist perspective, I think it'd be better for everything to be behind a single webkit-patch command that just did the right thing. It'd be more future-proof as the tools change. But we can always make that change later. This is a good incremental step in that direction either way.
(In reply to comment #7) > From a purist perspective, I think it'd be better for everything to be behind a single webkit-patch command that just did the right thing. It'd be more future-proof as the tools change. But we can always make that change later. This is a good incremental step in that direction either way. Yeah, I've thought that, too ... adding a webkit-patch rebaseline-all or something that takes a json description of what to do. That would be a simple cut&paste at some point.
Created attachment 144152 [details] clean up, add tests
Comment on attachment 144152 [details] clean up, add tests View in context: https://bugs.webkit.org/attachment.cgi?id=144152&action=review Nice test. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:209 > +def run_command(cmd_line_and_cwd): > + (cmd_line, cwd) = cmd_line_and_cwd > + host = Host() > + output = host.executive.run_command(cmd_line, cwd=cwd) dead code?
REview comments: https://bugs.webkit.org/show_bug.cgi?id=87528#c8
Comment on attachment 144152 [details] clean up, add tests View in context: https://bugs.webkit.org/attachment.cgi?id=144152&action=review Please see my other review comments, linked above. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:175 > def rebaselineall(self): I would encourage you to split this function into helpers for code readability.
Comment on attachment 144152 [details] clean up, add tests View in context: https://bugs.webkit.org/attachment.cgi?id=144152&action=review >> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:175 >> def rebaselineall(self): > > I would encourage you to split this function into helpers for code readability. Will do. >> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:209 >> + output = host.executive.run_command(cmd_line, cwd=cwd) > > dead code? Yeah, will remove.
(In reply to comment #11) > REview comments: https://bugs.webkit.org/show_bug.cgi?id=87528#c8 Thanks for the review! Responding here. >> Tools/Scripts/webkitpy/common/system/executive.py:464 >> +def _run_command_thunk(cmd_line_and_cwd): >> + (cmd_line, cwd) = cmd_line_and_cwd >> + proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) >> + stdout, stderr = proc.communicate() >> + return (proc.returncode, stdout, stderr) > This could easily have been a class method instead of a module method. Unfortunately, no, it couldn't. In order for it to work properly with the multiprocessing module, it needs to be a module method. I will add a comment to that end. >> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:67 >> + self._defer_scm_adds = False > It doesn't feel like it's really "defer"ing anything. It's just printing instead of doing. Fair point; it's deferred in the larger workflow, but that doesn't make much sense here. I'll change the name to something better. >> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:126 >> + print "scm-add:%s" % path > Do you have any testing of this printing and parsing of the resulting values? Spaces in file names unicode? No. Will add some. >> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:194 >> + files_to_add = set() >> + for output in [result[1] for result in command_results]: >> + files_to_add.update(line.replace('scm-add:', '') for line in output.splitlines() if line.startswith('scm-add:')) >> + scm.add_list(list(files_to_add)) > Yeah, this should be tested. You'll likely have to make it a separate function. The whole idea of serliazing/deserializing the add list is split across two classes currently. Well, it is tested as part of the existing unit tests, but I take your point. I'll add some more tests. >> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:209 >> +def run_command(cmd_line_and_cwd): >> + (cmd_line, cwd) = cmd_line_and_cwd >> + host = Host() >> + output = host.executive.run_command(cmd_line, cwd=cwd) > :( You really don't want to instantiate a new host per command. Yeah, this was a proof of concept. In the latest patch, this is all pushed into Executive and is very lightweight.
Comment on attachment 144152 [details] clean up, add tests View in context: https://bugs.webkit.org/attachment.cgi?id=144152&action=review > Tools/ChangeLog:5 > + Also, this could use a short, high-level description of what the patch is doing.
(In reply to comment #8) > (From update of attachment 144150 [details] [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144150&action=review > > > Tools/Scripts/webkitpy/common/system/executive.py:464 > > +def _run_command_thunk(cmd_line_and_cwd): > > + (cmd_line, cwd) = cmd_line_and_cwd > > + proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) > > + stdout, stderr = proc.communicate() > > + return (proc.returncode, stdout, stderr) > > This could easily have been a class method instead of a module method. Sorry, I should have said "static". @staticmethod def _run_command_thunk(command_line_and_cwd): pass MyClass._run_command_thunk should then do exactly what you want, no?
Created attachment 144160 [details] Patch
Comment on attachment 144160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144160&action=review I'll leave the r+ for eseidel since he had more substantive review comments. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:217 > +def run_command(cmd_line_and_cwd): > + (cmd_line, cwd) = cmd_line_and_cwd > + host = Host() > + output = host.executive.run_command(cmd_line, cwd=cwd) ?
Comment on attachment 144160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144160&action=review > Tools/Scripts/webkitpy/common/system/executive_unittest.py:220 > + def test_run_in_parallel(self): Doesn't this require a multi-processor machine to pass? :) Do we need to skip it if the machine has fewer than 2 processors? I'm not sure this is the betst way to test this. Another way might be to have 2 process, the first of which required the second to complete before it could complete? That has the risk of hanging on failure, but wouln't have risk of being flaky? > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:77 > + optparse.make_option("--print-scm-changes", action="store_true", help="Print modifcations to the scm (as a json dict) rather than actually modifying the scm"), Nice. > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:214 > +def run_command(cmd_line_and_cwd): I think you said this was dead?
(In reply to comment #19) > (From update of attachment 144160 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144160&action=review > > > Tools/Scripts/webkitpy/common/system/executive_unittest.py:220 > > + def test_run_in_parallel(self): > > Doesn't this require a multi-processor machine to pass? :) Do we need to skip it if the machine has fewer than 2 processors? Confused. Multiple processes still run in parallel on a single-core, no?
Comment on attachment 144160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144160&action=review >>> Tools/Scripts/webkitpy/common/system/executive_unittest.py:220 >>> + def test_run_in_parallel(self): >> >> Doesn't this require a multi-processor machine to pass? :) Do we need to skip it if the machine has fewer than 2 processors? >> >> I'm not sure this is the betst way to test this. Another way might be to have 2 process, the first of which required the second to complete before it could complete? That has the risk of hanging on failure, but wouln't have risk of being flaky? > > Confused. Multiple processes still run in parallel on a single-core, no? Yes, sorry. Sleep will of course yield, so this seems safe enough.
(In reply to comment #19) > (From update of attachment 144160 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144160&action=review > > > Tools/Scripts/webkitpy/common/system/executive_unittest.py:220 > > + def test_run_in_parallel(self): > > Doesn't this require a multi-processor machine to pass? :) Do we need to skip it if the machine has fewer than 2 processors? > > I'm not sure this is the betst way to test this. Another way might be to have 2 process, the first of which required the second to complete before it could complete? That has the risk of hanging on failure, but wouln't have risk of being flaky? > multiprocess.Poll() defaults to one task per CPU, but you can override it with the processes= keyword, as I'm doing. Thus this should work fine even on a single-proc CPU, since the tasks are just sleeping and waking up. > > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:214 > > +def run_command(cmd_line_and_cwd): > > I think you said this was dead? Haven't removed it yet :)
Created attachment 144185 [details] more review feedback, more tests, more cleanup
Created attachment 144187 [details] minor cleanup
Okay, this should cover all of the review feedback. The testing is still pretty light, but given that we're now using stock marshalling of json data to pass information between the commands, I'm not sure how much more is merited. Thoughts?
Comment on attachment 144187 [details] minor cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=144187&action=review > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:181 > + self._scm_changes = {'add': [], 'rm': []} Nit: s/rm/delete/ since that's what SCM calls these.
Committed r118836: <http://trac.webkit.org/changeset/118836>