<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Thanks Daniel. <div class=""><br class=""></div><div class="">I have made all your suggested changes, and committed in r218080. For now reruns are off by default. I will start introducing the flag to some of the bots.</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 23, 2014, at 1:33 PM, Daniel Dunbar <<a href="mailto:daniel@zuster.org" class="">daniel@zuster.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Comments on the main patch inline, haven't looked at the tests yet:<div class=""><br class=""></div><div class="">--</div><div class=""><br class=""></div><div class=""><div class="">> From 08612457b3c39c29eba5aa8ada79aeeb1e1d21cd Mon Sep 17 00:00:00 2001</div><div class="">
> From: Chris Matthews <<a href="mailto:cmatthews5@apple.com" class="">cmatthews5@apple.com</a>></div><div class="">> Date: Mon, 21 Jul 2014 14:53:23 -0700</div><div class="">> Subject: [PATCH] Add rerun flag: when passed, rerun benchmarks which the</div>
<div class="">> server says changed</div><div class="">> </div><div class="">> ---</div><div class="">> lnt/tests/nt.py | 319 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-</div><div class="">> 1 file changed, 315 insertions(+), 4 deletions(-)</div>
<div class="">> </div><div class="">> diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py</div><div class="">> index 7e292bc..4e34fb6 100644</div><div class="">> --- a/lnt/tests/nt.py</div><div class="">> +++ b/lnt/tests/nt.py</div><div class="">> @@ -7,18 +7,27 @@ import subprocess</div>
<div class="">> import sys</div><div class="">> import time</div><div class="">> import traceback</div><div class="">> -</div><div class="">> from datetime import datetime</div><div class="">> +from optparse import OptionParser, OptionGroup</div><div class="">> +</div>
<div class="">> </div><div class="">> import lnt.testing</div><div class="">> import lnt.testing.util.compilers</div><div class="">> import lnt.util.ImportData as ImportData</div><div class="">> </div><div class="">> -from lnt.testing.util.commands import note, warning, error, fatal, resolve_command_path</div>
<div class="">> +from lnt.testing.util.commands import note, warning, fatal</div><div class="">> from lnt.testing.util.commands import capture, mkdir_p, which</div><div class="">> +from lnt.testing.util.commands import resolve_command_path</div>
<div class="">> +</div><div class="">> from lnt.testing.util.rcs import get_source_version</div><div class="">> +</div><div class="">> from lnt.testing.util.misc import timestamp</div><div class="">> </div><div class="">> +from lnt.server.reporting.analysis import UNCHANGED_PASS, UNCHANGED_FAIL</div>
<div class="">> +from lnt.server.reporting.analysis import REGRESSED, IMPROVED</div><div class="">> +from lnt.util import ImportData</div><div class="">> +import builtintest</div><div class="">> +</div><div class="">> ###</div><div class="">> </div><div class="">> class TestModule(object):</div>
<div class="">> @@ -1033,5 +1042,61 @@ def run_test(nick_prefix, iteration, config):</div><div class="">> </div><div class="">> ###</div><div class="">> </div><div class="">> -import builtintest</div><div class="">> -from optparse import OptionParser, OptionGroup</div>
<div class="">> +def _construct_report_path(basedir, only_test, test_style, file_type="csv"):</div><div class="">> + """Get the full path to report files in the sandbox.</div><div class="">> + """</div>
<div class="">> + report_path = os.path.join(basedir)</div><div class="">> + if only_test is not None:</div><div class="">> + report_path = os.path.join(report_path, only_test)</div><div class="">> + report_path = os.path.join(report_path, ('report.%s.' % test_style) + file_type)</div>
<div class="">> + return report_path</div><div class="">> +</div><div class="">> +</div><div class="">> +def rerun_test(config, name, num_times):</div><div class="">> + """Take the test at name, and rerun it num_times with the previous settings</div>
<div class="">> + stored in config.</div><div class="">> +</div><div class="">> + """</div><div class="">> + # Extend the old log file.</div><div class="">> + logfile = open(config.test_log_path(None), 'a')</div><div class="">
> +</div><div class="">> + relative_test_path = os.path.dirname(name)</div><div class="">> + test_name = os.path.basename(name)</div><div class="">> +</div><div class="">> + single_test_report = _construct_report_path(config.report_dir,</div>
<div class="">> + config.only_test,</div><div class="">> + config.test_style)</div><div class="">> +</div><div class="">> + assert os.path.exists(single_test_report), "Previous report not there?" + \</div>
<div class="">> + single_test_report</div><div class="">> +</div><div class="">> + test_full_path = os.path.join(</div><div class="">> + config.report_dir, relative_test_path)</div><div class="">> +</div><div class="">> + assert os.path.exists(test_full_path), "Previous test directory not there?" + \</div>
<div class="">> + test_full_path</div><div class="">> +</div><div class="">> + results = []</div><div class="">> + for i in xrange(0, num_times):</div><div class="">> + _prepare_testsuite_for_rerun(test_name, test_full_path, config)</div>
<div class="">> + result_path = _execute_test_again(config,</div><div class="">> + test_name,</div><div class="">> + test_full_path,</div><div class="">> + logfile)</div>
<div class="">> + results.extend(load_nt_report_file(result_path, config))</div><div class="">> +</div><div class="">> + # Check we got an exec and status from each run.</div><div class="">> + assert len(results) >= num_times, "Did not get all the runs?"</div>
<div class="">> +</div><div class="">> + logfile.close()</div><div class="">> + return results</div><div class="">> +</div><div class="">> +</div><div class="">> +def _prepare_testsuite_for_rerun(test_name, test_full_path, config):</div><div class="">> + """Rerun step 1: wipe out old files. To prepare the test suite,</div>
<div class="">> + we must delete the correct files to convince make to rerun</div><div class="">> + just this test. For executoin time runs, these are the</div><div class="">> + previous execution time runs, then any reports that would hav</div>
<div class="">> + been created by them.</div><div class="">> +</div><div class="">> + """</div><div class=""><br class=""></div><div class="">Sic (execution, have)</div><div class=""><br class=""></div><div class="">> + output = os.path.join(test_full_path, "Output/")</div>
<div class="">> + # The execution timing file.</div><div class="">> +</div><div class="">> + try:</div><div class="">> + os.remove(output + test_name + ".out-" + config.test_style)</div><div class="">> + except OSError:</div>
<div class="">> + # If that does not work, try the mangled name.</div><div class="">> + os.remove(output + test_name.replace("_", ".") + ".out-" + config.test_style)</div><div class=""><br class=""></div><div class="">
I don't like the ad hoc handling of mangling here, and this won't properly work</div><div class="">with tests that had a mix of actual '_' and '.' characters. I think we should</div><div class="">find a principled way to solve this, one way would be to have the initial run</div>
<div class="">leave behind a mapping of original names to mangled names that could be inverted</div><div class="">on demand.</div><div class=""><br class=""></div><div class="">> +</div><div class="">> + # Remove the report files.</div><div class="">> + os.remove(_construct_report_path(config.report_dir,</div>
<div class="">> + config.only_test,</div><div class="">> + config.test_style))</div><div class="">> +</div><div class="">> + os.remove(_construct_report_path(config.report_dir,</div>
<div class="">> + config.only_test,</div><div class="">> + config.test_style,</div><div class="">> + "txt"))</div><div class="">> +</div>
<div class="">> + os.remove(_construct_report_path(config.report_dir,</div><div class="">> + config.only_test,</div><div class="">> + config.test_style,</div><div class="">> + "raw.out"))</div>
<div class=""><br class=""></div><div class="">I'm tempted to make this code more resilient and simpler by just removing the</div><div class="">entire contents of the Output directory. Do you see any good reason not to do</div><div class="">that? If we don't do that, I'd like to remove *.$(TEST).* from in the test dir.</div>
<div class=""><br class=""></div><div class="">> +</div><div class="">> +</div><div class="">> +def _execute_test_again(config, test_name, test_path, logfile):</div><div class="">> + """Rerun step 2: rerun the execution benchmark of interest. Now that the</div>
<div class="">> + test suite is ready, rerun the make command to gather new</div><div class="">> + data. Modify the old make arguments to just be the execution</div><div class="">> + target and all the reports that are dependent on that target.</div>
<div class="">> +</div><div class="">> + """</div><div class="">> + # Grab old make invocation.</div><div class="">> + mk_vars, _ = config.compute_run_make_variables()</div><div class="">> + to_exec = list(mk_vars)</div><div class="">
> + to_exec = ['make','-k'] + to_exec</div><div class="">> + to_exec.extend('%s=%s' % (k,v) for k,v in mk_vars.items())</div><div class="">> + if config.only_test is not None:</div><div class="">> + to_exec.extend(['-C', config.only_test])</div>
<div class="">> +</div><div class="">> + to_exec.append("Output/" + test_name + ".out-" + config.test_style)</div><div class="">> + to_exec.append("Output/" + test_name + ".diff-" + config.test_style)</div>
<div class="">> + to_exec.append("Output/" + test_name + ".exec-" + config.test_style)</div><div class="">> + to_exec.append("Output/" + test_name + ".exec.report." + config.test_style)</div>
<div class="">> + to_exec.append("Output/" + test_name + "." + config.test_style + ".report.txt")</div><div class="">> + to_exec.append("report." + config.test_style + ".csv")</div>
<div class="">> + to_exec.append("report")</div><div class=""><br class=""></div><div class="">You don't need to provide all of these targets. The "master" target for driving</div><div class="">each test is $(TEST-NAME).$(TEST).report.txt, and we should only build that</div>
<div class="">target. If you build "report" or "report.$(TEST).csv", you may end up rerunning</div><div class="">many other tests (in directories that have multiple tests, or that have</div><div class="">subdirectories).</div><div class="">
<br class=""></div><div class="">> +</div><div class="">> + result_loc = _construct_report_path(config.report_dir,</div><div class="">> + config.only_test,</div><div class="">> + config.test_style)</div>
<div class="">> + </div><div class=""><br class=""></div><div class="">Instead of building the report.$(TEST).csv, I'd like to just build the one</div><div class="">individual report file, then manually call GenerateReport.pl on the one input</div><div class="">file we need so we can get an output we can pass to load_nt_report_file()</div>
<div class=""><br class=""></div><div class="">> + assert not os.path.exists(result_loc)</div><div class="">> + execute_command(logfile, config.build_dir(None), to_exec, test_path)</div><div class="">> + assert os.path.exists(result_loc), "Missing {}".format(result_loc)</div>
<div class="">> + return result_loc</div><div class=""><br class=""></div><div class="">I think a more sensible API is for this method to return the report data, not</div><div class="">the path to the report. Also, I would sink the call to</div><div class="">prepare_testsuite_for_rerun into this method.</div>
<div class=""><br class=""></div><div class="">> +</div><div class="">> +</div><div class="">> +# When set to true, all benchmarks will be rerun.</div><div class="">> +DEBUG_RERUN = False</div><div class=""><br class=""></div><div class="">What is this here for?</div><div class=""><br class=""></div><div class="">> +NUMBER_OF_RERUNS = 4</div>
<div class="">> +</div><div class="">> +SERVER_FAIL = u'FAIL'</div><div class="">> +SERVER_PASS = u'PASS'</div><div class="">> +</div><div class="">> +# Local test reults names have these suffexes</div><div class="">> +# Test will have the perf suffex if it passed</div>
<div class="">> +# if it failed it will have a status suffex.</div><div class=""><br class=""></div><div class="">Sic (suffix)</div><div class=""><br class=""></div><div class="">> +LOCAL_COMPILE_PERF = "compile"</div><div class="">> +LOCAL_COMPILE_STATUS = "compile.status"</div>
<div class="">> +LOCAL_EXEC_PERF = "exec"</div><div class="">> +LOCAL_EXEC_STATUS = "exec.status"</div><div class="">> +</div><div class="">> +# Server results have both status and performance in each entry</div><div class="">> +SERVER_COMPILE_RESULT = "compile_time"</div>
<div class="">> +SERVER_EXEC_RESULT = "execution_time"</div><div class="">> +</div><div class="">> +</div><div class="">> +class PastRunData:</div><div class=""><br class=""></div><div class="">This should inherit from object to be modern.</div><div class=""><br class=""></div><div class="">
> + """To decide if we need to rerun, we must know</div><div class="">> + what happened on each test in the first runs.</div><div class="">> + Because the server retruns data in a different format than</div>
<div class="">> + the local results, this class contains a per-test per-run</div><div class="">> + aggregate of the two reports."""</div><div class=""><br class=""></div><div class="">Sic (reruns)</div><div class=""><br class=""></div><div class="">> + def __init__(self, name):</div>
<div class="">> + <a href="http://self.name/" class="">self.name</a> = name</div><div class="">> + self.compile_status = None</div><div class="">> + self.compile_time = None</div><div class="">> + self.execution_status = None</div>
<div class="">> + self.execution_time = None</div><div class="">> + self.valid = False</div><div class="">> +</div><div class="">> + def _check(self):</div><div class="">> + """Make sure this run data is complete."""</div>
<div class="">> + assert <a href="http://self.name/" class="">self.name</a> is not None</div><div class="">> + msg = "Malformed test: %s" % (repr(self))</div><div class="">> + assert self.compile_status is not None, msg</div>
<div class="">> + assert self.execution_status is not None, msg</div><div class="">> + assert self.compile_time is not None, msg</div><div class="">> + assert self.execution_time is not None, msg</div><div class="">> + self.valid = True</div>
<div class="">> +</div><div class="">> + def _is_rerunable(self):</div><div class="">> + """Decide if we should rerun this test. Rerun all execute tests when in</div><div class="">> + debug, re-run if test compiled correctly, execuited correctly,</div>
<div class="">> + but performance changed. Don't rerun if compile failed, or execute</div><div class="">> + failed, sucessful execute did not change in performance (as decided by</div><div class="">> + the server).</div>
<div class="">> + """</div><div class=""><br class=""></div><div class="">Sic (executed). This doc comment just duplicates the comments in the code, I</div><div class="">would just make this describe the API (should we rerun this test) not the logic.</div>
<div class=""><br class=""></div><div class="">> + assert self.valid</div><div class="">> + # Don't rerun if compile failed.</div><div class="">> + if self.compile_status == SERVER_FAIL:</div><div class="">> + return False</div>
<div class="">> +</div><div class="">> + # Rerun everything in debug.</div><div class="">> + if DEBUG_RERUN:</div><div class="">> + return True</div><div class="">> +</div><div class="">> + # Don't rerun on correctness failure or test pass.</div>
<div class="">> + if self.execution_status == UNCHANGED_FAIL or \</div><div class="">> + self.execution_status == UNCHANGED_PASS or \</div><div class="">> + self.execution_status == SERVER_FAIL:</div><div class="">> + return False</div>
<div class="">> +</div><div class="">> + # Do rerun on regression or improvement.</div><div class="">> + if self.execution_status == REGRESSED or \</div><div class="">> + self.execution_status == IMPROVED:</div><div class="">> + return True</div>
<div class="">> +</div><div class="">> + assert False, "Malformed run data: " \</div><div class="">> + "you should not get here. " + str(self)</div><div class="">> +</div><div class="">> + def __repr__(self):</div>
<div class="">> + template = "<{}: CS {}, CT {}, ES {}, ET {}>"</div><div class="">> + return template.format(<a href="http://self.name/" class="">self.name</a>,</div><div class="">> + repr(self.compile_status),</div>
<div class="">> + repr(self.compile_time),</div><div class="">> + repr(self.execution_status),</div><div class="">> + repr(self.execution_time))</div>
<div class="">> +</div><div class="">> +</div><div class="">> +def _process_reruns(config, server_reply, local_results):</div><div class="">> + """Rerun each benchmark which the server reported "changed", N more</div>
<div class="">> + times. By default N=4 to make 5 samples total. 5 samples is the</div><div class="">> + smallest thing that has an okay median. For now, only rerun the</div><div class="">> + exec time tests, and don't rerun test which failed or whoes</div>
<div class="">> + compile failed.</div><div class="">> +</div><div class="">> + """</div><div class=""><br class=""></div><div class="">This comment again duplicates ones elsewhere, better to delegate.</div><div class=""><br class=""></div><div class="">> + # To know which benchmarks to run, we need to check the local results</div>
<div class="">> + # to see which did not fail, then check the server results to see</div><div class="">> + # which regressed or improved. The server will return extra results</div><div class="">> + # for things that we did not run this time, it may also omit performance</div>
<div class="">> + # results.</div><div class=""><br class=""></div><div class="">More duplication of logic explanation.</div><div class=""><br class=""></div><div class="">> + server_results = server_reply['test_results'][0]['results']</div><div class="">> +</div>
<div class="">> + # Holds the combined local and server results.</div><div class="">> + cololated_results = dict()</div><div class=""><br class=""></div><div class="">Sic (collated)</div><div class=""><br class=""></div><div class="">> +</div><div class="">> + for b in local_results.tests:</div>
<div class="">> + # format: suite.test/path/and/name.type<.type></div><div class="">> + fields = b.name.split('.')</div><div class="">> + test_name = fields[1]</div><div class="">> + test_type = '.'.join(fields[2:])\</div>
<div class="">> +</div><div class="">> + updating_entry = cololated_results.get(test_name,</div><div class="">> + PastRunData(test_name))</div><div class="">> + if test_type == LOCAL_COMPILE_PERF:</div>
<div class="">> + updating_entry.compile_time = b.data</div><div class="">> + elif test_type == LOCAL_COMPILE_STATUS:</div><div class="">> + updating_entry.compile_status = SERVER_FAIL</div><div class="">> + elif test_type == LOCAL_EXEC_PERF:</div>
<div class="">> + updating_entry.execution_time = b.data</div><div class="">> + elif test_type == LOCAL_EXEC_STATUS:</div><div class="">> + updating_entry.execution_status = SERVER_FAIL</div><div class="">> + else:</div>
<div class="">> + assert False, "Unexpected local test type."</div><div class="">> +</div><div class="">> + cololated_results[test_name] = updating_entry</div><div class="">> +</div><div class="">> + # Now add on top the server results to any entry we already have.</div>
<div class="">> + for full_name, results_status, perf_status in server_results:</div><div class="">> + test_name, test_type = full_name.split(".")</div><div class="">> +</div><div class="">> + new_entry = cololated_results.get(test_name, None)</div>
<div class="">> + # Some tests will come from the server, which we did not run locally.</div><div class="">> + # Drop them.</div><div class="">> + if new_entry is None:</div><div class="">> + continue</div><div class="">> + # Set these, if they were not set with fails above.</div>
<div class="">> + if SERVER_COMPILE_RESULT in test_type:</div><div class="">> + if new_entry.compile_status is None:</div><div class="">> + new_entry.compile_status = results_status</div><div class="">> + elif SERVER_EXEC_RESULT in test_type:</div>
<div class="">> + if new_entry.execution_status is None:</div><div class="">> + # If the server has not seen the test before, it will return</div><div class="">> + # None for the performance results analysis. In this case we</div>
<div class="">> + # will assume no rerun is needed, so assign unchanged.</div><div class="">> + if perf_status is None:</div><div class="">> + derived_perf_status = UNCHANGED_PASS</div><div class="">
> + else:</div><div class="">> + derived_perf_status = perf_status</div><div class="">> + new_entry.execution_status = derived_perf_status</div><div class="">> + else:</div><div class="">> + assert False, "Unexpected server result type."</div>
<div class="">> + cololated_results[test_name] = new_entry</div><div class="">> +</div><div class="">> + # Double check that all values are there for all tests.</div><div class="">> + for test in cololated_results.values():</div><div class="">
> + test._check()</div><div class="">> +</div><div class="">> + rerunable_benches = filter(lambda x: x._is_rerunable(),</div><div class="">> + cololated_results.values())</div><div class="">> + rerunable_benches.sort(key=lambda x: <a href="http://x.name/" class="">x.name</a>)</div>
<div class="">> + # Now lets actually do the reruns.</div><div class="">> + rerun_results = []</div><div class="">> + SUMMARY = "Rerunning {} of {} benchmarks."</div><div class="">> + note(SUMMARY.format(len(rerunable_benches),</div>
<div class="">> + len(cololated_results.values())))</div><div class="">> +</div><div class="">> + for i, bench in enumerate(rerunable_benches):</div><div class="">> + note("Rerunning: {} [{}/{}]".format(<a href="http://bench.name/" class="">bench.name</a>,</div>
<div class="">> + i + 1,</div><div class="">> + len(rerunable_benches)))</div><div class="">> + fresh_samples = rerun_test(config,</div><div class="">> + <a href="http://bench.name/" class="">bench.name</a>,</div>
<div class="">> + NUMBER_OF_RERUNS)</div><div class="">> + rerun_results.extend(fresh_samples)</div><div class="">> +</div><div class="">> + return rerun_results</div><div class="">> +</div><div class=""><br class=""></div><div class="">
This method seems more complicated than it needs to be. Can't we just scan the</div><div class="">server results for the list of tests we need to rerun, then filter out any that</div><div class="">we didn't run locally, then run those? It isn't clear to me we need to do the</div>
<div class="">collation and build up all the PastRunData objects.</div><div class=""> </div><div class="">> usage_info = """</div><div class="">> Script for running the tests in LLVM's test-suite repository.</div><div class="">> @@ -1233,6 +1529,9 @@ class NTTest(builtintest.BuiltinTest):</div>
<div class="">> group.add_option("", "--build-threads", dest="build_threads",</div><div class="">> help="Number of compilation threads",</div><div class="">> type=int, default=0, metavar="N")</div>
<div class="">> + group.add_option("", "--rerun", dest="rerun",</div><div class="">> + help="Rerun tests that have regressed.",</div><div class="">> + action="store_true", default=False)</div>
<div class="">> </div><div class="">> group.add_option("", "--remote", dest="remote",</div><div class="">> help=("Execute remotely, see "</div><div class="">> @@ -1495,3 +1794,13 @@ class NTTest(builtintest.BuiltinTest):</div>
<div class="">> </div><div class="">> else:</div><div class="">> test_results = run_test(nick, None, config)</div><div class="">> + if opts.rerun:</div><div class="">> + self.log("Performing any needed reruns.")</div>
<div class="">> + server_report = self.submit_helper(config, commit=False)</div><div class="">> + new_samples = _process_reruns(config, server_report, test_results)</div><div class="">> + test_results.update_report(new_samples)</div>
<div class="">> +</div><div class="">> + # persist report with new samples.</div><div class="">> + lnt_report_path = config.report_path(None)</div><div class="">> + os.unlink(lnt_report_path)</div><div class="">
> + lnt_report_file = open(lnt_report_path, 'w')</div><div class=""><br class=""></div><div class="">Why bother to unlink here? </div><div class=""><br class=""></div><div class="">> + print >>lnt_report_file, test_results.render()</div>
<div class="">> + lnt_report_file.close()</div><div class="">> </div><div class="">> if config.output is not None:</div><div class="">> self.print_report(test_results, config.output)</div><div class="">> -- </div>
<div class="">> 1.9.3 (Apple Git-50)+GitX</div></div><div class=""><br class=""></div><div class="">--</div></div><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On Mon, Jul 21, 2014 at 3:32 PM, Chris Matthews <span dir="ltr" class=""><<a href="mailto:chris.matthews@apple.com" target="_blank" class="">chris.matthews@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br class="">
<br class="">
Here is an update to the rerun patch discussed last year. Attached is a patch for the NT test suite as well as a new rerun test in the LNT test suite.<br class="">
<br class="">
The patch does not perform well (reruns take quite a while). I’d like to address that in a separate patch.<br class="">
<br class="">
</blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></body></html>