Taking talos to the next step, we have a variety of files that we report via the xperf toolchain while running tp5. We have some historical data and we have a nice list of files and min/max bytes we reference. During our xperf analysis locally after a test run, we currently just build a summary and upload it to datazilla. Now we should take a hardcoded list of files/bytes and look for things which fall outside of that list. In addition, we will currently ignore all files in the dependentlibs.list which is distributed with the build: MSVCR100.dll MSVCP100.dll mozglue.dll nss3.dll mozjs.dll mozalloc.dll gkmedias.dll xul.dll attached is a first pass at the xperf_whitelist.json file. The goal is to: 1) find a good xperf_whitelist.json definition 2) determine if we should track read/write separate? 3) read dependentlibs.list from the build we are testing (example above) 4) add dependentlibs to the whitelist 5) for all data we are reporting, ensure it all is in the xperf_whitelist.json 6) build up a list of all filenames/bytes which do not fit 7) report errors for each filename that we have 8) talos job turns orange 9) ensure all data is still uploaded to datazilla for future mining

Comment on attachment 769041 [details] [diff] [review] add xperf processing to talos (0.9) Review of attachment 769041 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with nits and things I don't get ::: talos/run_tests.py @@ +327,4 @@ > utils.startLogger(level) > > # run tests > + sys.exit(run_tests(parser)) Originally: Why do you sys.exit here? Now: Oh, I get it. Maybe update the comment wrt the exit code. ::: talos/ttest.py @@ +438,5 @@ > + counter = None > + if 'cm' in vars(): > + counters = cm > + self.testCleanup(browser_config, profile_dir, test_config, counters, temp_dir) > + # by returning 1, we report an orange to buildbot A link to these magic numbers would be nice ::: talos/utils.py @@ +73,5 @@ > + def __init__(self, msg): > + self.msg = msg > + def __str__(self): > + return repr(self.msg) > + I'm not sure what you're trying to do here. You should be able to do class talosRegression(Exception): """a helpful docstring might be nice""" ::: talos/xtalos/etlparser.py @@ +312,5 @@ > + > + if approot: > + fhandle = open('%s\\dependentlibs.list' % approot, 'r') > + libs = fhandle.read() > + fhandle.close() Coupla things: 1. the file location is relative to CWD, not to __file__ ; this seems wrong. 2. what if the file doesn't exist? @@ +315,5 @@ > + libs = fhandle.read() > + fhandle.close() > + > + for lib in libs: > + wl_temp[lib] = {'ignore': True} fhandle.read returns a string; this iterates over the characters, not the lines. use .readlines or splitlines @@ +367,5 @@ > + for error in errors: > + print "TEST-UNEXPECTED-FAIL : xperf: %s" % error > + errorFile = open('browser_failures.txt', 'w') > + errorFile.write('\n'.join(errors)) > + errorFile.close() This seems...a bit complicated to me. But I will default to your judgement for now. @@ +379,4 @@ > 'etl_filename': 'output.etl', > 'outputFile': None, > 'processID': None, > + 'approot': None, What is this? ::: talos/xtalos/xtalos.py @@ +89,5 @@ > > + self.add_option("-a", "--approot", dest="approot", > + help="Provide the root directory of the application we are testing to find related files (i.e. dependentlibs.list)") > + defaults["approot"] = None > + Not sure I get it ....still

thanks for the feedback on the previous patch! I have addressed your nits and questions. The one dangling issue is the use of 'approot'. We need to determine where the file 'dependentlibs.list' is located at, specifically the one related to the application we are testing. This is determined with approot and we pass that in from the main runner into bcontroller and then xtalos.

Comment on attachment 772807 [details] [diff] [review] add xperf processing to talos (1.0) Review of attachment 772807 [details] [diff] [review]: ----------------------------------------------------------------- ::: talos/run_tests.py @@ +327,5 @@ > utils.startLogger(level) > > + # run tests - return success/warn/error for buildbot > + # http://docs.buildbot.net/latest/developer/results.html > + sys.exit(run_tests(parser)) run_tests returns None ::: talos/ttest.py @@ +436,5 @@ > > + except talosRegression, tr: > + counter = None > + if 'cm' in vars(): > + counters = cm counters = vars().get('cm', counters) That said, A. this is repeat code B. the whole thing is a bit of an anti-pattern; AIUI, - 'counters' is a list of counters; cm is a counter manager that is instantiated only if counters is non empty: http://hg.mozilla.org/build/talos/file/a11542b55a70/talos/ttest.py#l361 - so, we strangely look in vars() which is probably wrong anyway since counters could be per test, at least in theory C. the right way to do this is just to fix cm to be None per test if counters is not none I could be content with taking this here or as a follow up @@ +450,4 @@ > except talosError, te: > self.testCleanup(browser_config, profile_dir, test_config, counters, temp_dir) > raise > + return 0 There seems to be some confusion re return values; you return 0 and 1 here, which are not used, though an exception is raised if -1. What is currently returned is a results object. I'm not sure what should be done here, but guessing....not this ::: talos/utils.py @@ +70,5 @@ > return repr(self.msg) > > +class talosRegression(Exception): > + """When a regression is detected at runtime, report it properly > + Currently this is a shell exception so we can detect the class type Not sure what this means? ::: talos/xtalos/etlparser.py @@ +305,5 @@ > # close the file handle > outFile.close() > > + # We still like to have the outputfile to record the raw data, now filter out acceptable files/ranges > + fHandle = open('xtalos\\xperf_whitelist.json', 'r') should be a fully qualified path?

updated to address review feedback. This involved a lot of return code exactness. Tested on try, still able to get oranges and reds when I expect.

Comment on attachment 773506 [details] [diff] [review] add xperf processing to talos (2.0) Review of attachment 773506 [details] [diff] [review]: ----------------------------------------------------------------- going to hold off review pending questions.....seems very close. ::: talos/run_tests.py @@ +278,5 @@ > + if retVal == 1: > + raise talosError("Test Failure Detected") > + talos_results.add(retVal) > + if retVal != 0: > + break Do we really want to skip the rest of the tests on failure? ::: talos/ttest.py @@ +404,5 @@ > if not os.path.isfile(browser_log_filename): > raise talosError("no output from browser [%s]" % browser_log_filename) > > + # ensure the browser log exists > + browser_failures_filename = 'browser_failures.txt' What if this file is present before you begin a talos run? @@ +438,5 @@ > + counters = vars().get('cm', counters) > + self.testCleanup(browser_config, profile_dir, test_config, counters, temp_dir) > + # by returning 1, we report an orange to buildbot > + # http://docs.buildbot.net/latest/developer/results.html > + return 1 Why not reraise the talosRegression instead of mixing return types of numbers and actual results? ::: talos/xtalos/etlparser.py @@ +307,5 @@ > > + # We still like to have the outputfile to record the raw data, now filter out acceptable files/ranges > + filename = None > + wl_temp = {} > + if os.path.exists('xtalos') and os.path.exists(os.path.join('xtalos', 'xperf_whitelist.json')): Again, this is a local path, correct? and it should be an abspath ABICT path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'xperf_whitelist.json') I would also advise making this a variable so that you don't have to reconstruct it. @@ +339,5 @@ > + for row in filekeys: > + filename = row[0] > + filename = filename.lower() > + # take care of 'program files (x86)' matching 'program files' > + filename = filename.replace(" (x86)", '') evil but necessary :( @@ +373,5 @@ > +# elif wl[filename]['maxcount'] < (files[row]['DiskReadCount'] + files[row]['DiskWriteCount']): > +# errors.append("%s: %s accesses, more than expected maximum: %s" % (filename, (files[row]['DiskReadCount'] + files[row]['DiskWriteCount']), wl[filename]['maxcount'])) > + else: > + errors.append("File '%s' was accessed and we were not expecting it. DiskReadCount: %s, DiskWriteCount: %s, DiskReadBytes: %s, DiskWriteBytes: %s" % (filename, files[row]['DiskReadCount'], files[row]['DiskWriteCount'], files[row]['DiskReadBytes'], files[row]['DiskWriteBytes'])) > + This is all rather painful but I don't know how to make it better. ::: talos/xtalos/xperf_whitelist.json @@ +1,1 @@ > +{"{firefox}\\Crash Reports\\{time}": {"ignore": true}, "C:\\$Mft": {"ignore": true}, "C:\\$Extend\\$UsnJrnl:$J": {"ignore": true}, "C:\\Windows\\Prefetch\\FIREFOX.EXE-59265DAF.pf": {"ignore": true}, "{firefox}\\xul.dll": {"mincount": 636, "maxcount": 640, "minbytes": 41680896, "maxbytes": 41943040}, "{profile}\\localstore.rdf": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\dependentlibs.list": {"mincount": 4, "maxcount": 4, "minbytes": 16384, "maxbytes": 16384}, "{profile}\\content-prefs.sqlite": {"mincount": 6, "maxcount": 6, "minbytes": 65768, "maxbytes": 65768}, "{profile}\\extensions.ini": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\mozjs.dll": {"mincount": 106, "maxcount": 110, "minbytes": 6946816, "maxbytes": 7208960}, "{profile}\\extensions\\pageloader@mozilla.org\\chrome.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 600, "maxbytes": 600}, "{profile}\\mimetypes.rdf": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\nss3.dll": {"mincount": 58, "maxcount": 58, "minbytes": 3801088, "maxbytes": 3801088}, "{firefox}\\profiles.ini": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "c:\\programdata\\nvidia corporation\\drs\\nvdrssel.bin": {"mincount": 2, "maxcount": 2, "minbytes": 2, "maxbytes": 2}, "{firefox}\\browser\\components\\components.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 68, "maxbytes": 68}, "{profile}\\places.sqlite": {"mincount": 8, "maxcount": 8, "minbytes": 196808, "maxbytes": 196808}, "{profile}\\pluginreg.dat": {"mincount": 2, "maxcount": 2, "minbytes": 1892, "maxbytes": 1892}, "{firefox}\\defaults\\pref\\channel-prefs.js": {"mincount": 4, "maxcount": 4, "minbytes": 1432, "maxbytes": 1432}, "c:\\talos-slave\\talos-data\\talos\\page_load_test\\tp5n\\tp5n.manifest": {"mincount": 8, "maxcount": 8, "minbytes": 32768, "maxbytes": 32768}, "c:\\windows\\system32\\dwrite.dll": {"mincount": 4, "maxcount": 4, "minbytes": 16384, "maxbytes": 90112}, "c:\\users\\desktop.ini": {"mincount": 2, "maxcount": 2, "minbytes": 352, "maxbytes": 352}, "{desktop}\\desktop.ini": {"mincount": 6, "maxcount": 6, "minbytes": 1692, "maxbytes": 1692}, "{firefox}\\mozalloc.dll": {"mincount": 2, "maxcount": 2, "minbytes": 131072, "maxbytes": 131072}, "c:\\windows\\fonts\\staticcache.dat": {"mincount": 2, "maxcount": 2, "minbytes": 120, "maxbytes": 120}, "c:\\windows\\system32\\spool\\drivers\\color\\srgb color space profile.icm": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\mozglue.dll": {"mincount": 4, "maxcount": 6, "minbytes": 262144, "maxbytes": 393216}, "{profile}\\search.json": {"mincount": 12, "maxcount": 12, "minbytes": 33350, "maxbytes": 33350}, "{profile}\\prefs.js": {"mincount": 4, "maxcount": 4, "minbytes": 19612, "maxbytes": 20136}, "{profile}\\compatibility.ini": {"mincount": 2, "maxcount": 34, "minbytes": 734, "maxbytes": 8192}, "{profile}\\places.sqlite-shm": {"mincount": 2, "maxcount": 2, "minbytes": 65536, "maxbytes": 65536}, "{firefox}\\msvcp100.dll": {"mincount": 14, "maxcount": 14, "minbytes": 917504, "maxbytes": 917504}, "{profile}\\user.js": {"mincount": 4, "maxcount": 4, "minbytes": 6000, "maxbytes": 6000}, "{profile}\\permissions.sqlite": {"mincount": 14, "maxcount": 14, "minbytes": 4424, "maxbytes": 4424}, "{firefox}\\crash reports\\lastcrash": {"mincount": 2, "maxcount": 2, "minbytes": 20, "maxbytes": 20}, "{firefox}\\browser\\chrome.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 80, "maxbytes": 80}, "{firefox}\\msvcr100.dll": {"mincount": 24, "maxcount": 24, "minbytes": 1572864, "maxbytes": 1572864}, "{firefox}\\gkmedias.dll": {"mincount": 106, "maxcount": 106, "minbytes": 6946816, "maxbytes": 6946816}, "c:\\program files\\desktop.ini": {"mincount": 2, "maxcount": 2, "minbytes": 352, "maxbytes": 352}} Any reason this has to be minified? Personally I'd prefer readable JSON

Comment on attachment 773506 [details] [diff] [review] add xperf processing to talos (2.0) Review of attachment 773506 [details] [diff] [review]: ----------------------------------------------------------------- Going to go ahead and r- due to the severity of the questions. I'm really not a fan of mixing return types of ints (really, sys.exit codes) with actual test results. IMHO a try-catch pattern is better. ::: talos/run_tests.py @@ +278,5 @@ > + if retVal == 1: > + raise talosError("Test Failure Detected") > + talos_results.add(retVal) > + if retVal != 0: > + break Do we really want to skip the rest of the tests on failure? ::: talos/ttest.py @@ +404,5 @@ > if not os.path.isfile(browser_log_filename): > raise talosError("no output from browser [%s]" % browser_log_filename) > > + # ensure the browser log exists > + browser_failures_filename = 'browser_failures.txt' What if this file is present before you begin a talos run? @@ +438,5 @@ > + counters = vars().get('cm', counters) > + self.testCleanup(browser_config, profile_dir, test_config, counters, temp_dir) > + # by returning 1, we report an orange to buildbot > + # http://docs.buildbot.net/latest/developer/results.html > + return 1 Why not reraise the talosRegression instead of mixing return types of numbers and actual results? ::: talos/xtalos/etlparser.py @@ +307,5 @@ > > + # We still like to have the outputfile to record the raw data, now filter out acceptable files/ranges > + filename = None > + wl_temp = {} > + if os.path.exists('xtalos') and os.path.exists(os.path.join('xtalos', 'xperf_whitelist.json')): Again, this is a local path, correct? and it should be an abspath ABICT path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'xperf_whitelist.json') I would also advise making this a variable so that you don't have to reconstruct it. @@ +339,5 @@ > + for row in filekeys: > + filename = row[0] > + filename = filename.lower() > + # take care of 'program files (x86)' matching 'program files' > + filename = filename.replace(" (x86)", '') evil but necessary :( @@ +373,5 @@ > +# elif wl[filename]['maxcount'] < (files[row]['DiskReadCount'] + files[row]['DiskWriteCount']): > +# errors.append("%s: %s accesses, more than expected maximum: %s" % (filename, (files[row]['DiskReadCount'] + files[row]['DiskWriteCount']), wl[filename]['maxcount'])) > + else: > + errors.append("File '%s' was accessed and we were not expecting it. DiskReadCount: %s, DiskWriteCount: %s, DiskReadBytes: %s, DiskWriteBytes: %s" % (filename, files[row]['DiskReadCount'], files[row]['DiskWriteCount'], files[row]['DiskReadBytes'], files[row]['DiskWriteBytes'])) > + This is all rather painful but I don't know how to make it better. ::: talos/xtalos/xperf_whitelist.json @@ +1,1 @@ > +{"{firefox}\\Crash Reports\\{time}": {"ignore": true}, "C:\\$Mft": {"ignore": true}, "C:\\$Extend\\$UsnJrnl:$J": {"ignore": true}, "C:\\Windows\\Prefetch\\FIREFOX.EXE-59265DAF.pf": {"ignore": true}, "{firefox}\\xul.dll": {"mincount": 636, "maxcount": 640, "minbytes": 41680896, "maxbytes": 41943040}, "{profile}\\localstore.rdf": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\dependentlibs.list": {"mincount": 4, "maxcount": 4, "minbytes": 16384, "maxbytes": 16384}, "{profile}\\content-prefs.sqlite": {"mincount": 6, "maxcount": 6, "minbytes": 65768, "maxbytes": 65768}, "{profile}\\extensions.ini": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\mozjs.dll": {"mincount": 106, "maxcount": 110, "minbytes": 6946816, "maxbytes": 7208960}, "{profile}\\extensions\\pageloader@mozilla.org\\chrome.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 600, "maxbytes": 600}, "{profile}\\mimetypes.rdf": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\nss3.dll": {"mincount": 58, "maxcount": 58, "minbytes": 3801088, "maxbytes": 3801088}, "{firefox}\\profiles.ini": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "c:\\programdata\\nvidia corporation\\drs\\nvdrssel.bin": {"mincount": 2, "maxcount": 2, "minbytes": 2, "maxbytes": 2}, "{firefox}\\browser\\components\\components.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 68, "maxbytes": 68}, "{profile}\\places.sqlite": {"mincount": 8, "maxcount": 8, "minbytes": 196808, "maxbytes": 196808}, "{profile}\\pluginreg.dat": {"mincount": 2, "maxcount": 2, "minbytes": 1892, "maxbytes": 1892}, "{firefox}\\defaults\\pref\\channel-prefs.js": {"mincount": 4, "maxcount": 4, "minbytes": 1432, "maxbytes": 1432}, "c:\\talos-slave\\talos-data\\talos\\page_load_test\\tp5n\\tp5n.manifest": {"mincount": 8, "maxcount": 8, "minbytes": 32768, "maxbytes": 32768}, "c:\\windows\\system32\\dwrite.dll": {"mincount": 4, "maxcount": 4, "minbytes": 16384, "maxbytes": 90112}, "c:\\users\\desktop.ini": {"mincount": 2, "maxcount": 2, "minbytes": 352, "maxbytes": 352}, "{desktop}\\desktop.ini": {"mincount": 6, "maxcount": 6, "minbytes": 1692, "maxbytes": 1692}, "{firefox}\\mozalloc.dll": {"mincount": 2, "maxcount": 2, "minbytes": 131072, "maxbytes": 131072}, "c:\\windows\\fonts\\staticcache.dat": {"mincount": 2, "maxcount": 2, "minbytes": 120, "maxbytes": 120}, "c:\\windows\\system32\\spool\\drivers\\color\\srgb color space profile.icm": {"mincount": 2, "maxcount": 2, "minbytes": 8192, "maxbytes": 8192}, "{firefox}\\mozglue.dll": {"mincount": 4, "maxcount": 6, "minbytes": 262144, "maxbytes": 393216}, "{profile}\\search.json": {"mincount": 12, "maxcount": 12, "minbytes": 33350, "maxbytes": 33350}, "{profile}\\prefs.js": {"mincount": 4, "maxcount": 4, "minbytes": 19612, "maxbytes": 20136}, "{profile}\\compatibility.ini": {"mincount": 2, "maxcount": 34, "minbytes": 734, "maxbytes": 8192}, "{profile}\\places.sqlite-shm": {"mincount": 2, "maxcount": 2, "minbytes": 65536, "maxbytes": 65536}, "{firefox}\\msvcp100.dll": {"mincount": 14, "maxcount": 14, "minbytes": 917504, "maxbytes": 917504}, "{profile}\\user.js": {"mincount": 4, "maxcount": 4, "minbytes": 6000, "maxbytes": 6000}, "{profile}\\permissions.sqlite": {"mincount": 14, "maxcount": 14, "minbytes": 4424, "maxbytes": 4424}, "{firefox}\\crash reports\\lastcrash": {"mincount": 2, "maxcount": 2, "minbytes": 20, "maxbytes": 20}, "{firefox}\\browser\\chrome.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 80, "maxbytes": 80}, "{firefox}\\msvcr100.dll": {"mincount": 24, "maxcount": 24, "minbytes": 1572864, "maxbytes": 1572864}, "{firefox}\\gkmedias.dll": {"mincount": 106, "maxcount": 106, "minbytes": 6946816, "maxbytes": 6946816}, "c:\\program files\\desktop.ini": {"mincount": 2, "maxcount": 2, "minbytes": 352, "maxbytes": 352}} Any reason this has to be minified? Personally I'd prefer readable JSON

take 3, this removes all the hardcoded file names and paths.

Found a case in retriggers where Prefetch is not guaranteed to be case sensitive in the log file we collect from xperf, support for Prefetch and prefetch now.

Comment on attachment 779321 [details] [diff] [review] add xperf processing to talos (3.1) Review of attachment 779321 [details] [diff] [review]: ----------------------------------------------------------------- Yep, much much better! r++,would review again! ::: talos/ttest.py @@ +404,5 @@ > if not os.path.isfile(browser_log_filename): > raise talosError("no output from browser [%s]" % browser_log_filename) > > + # ensure the browser log exists > + if os.path.exists(browser_config['error_filename']): I would find it desirable to check before hand if the error (regression?) file exists; otherwise, it appears to me that a second run can/will die here. ::: talos/xtalos/xperf_whitelist.json @@ +36,5 @@ > + "{firefox}\\crash reports\\lastcrash": {"mincount": 2, "maxcount": 2, "minbytes": 20, "maxbytes": 20}, > + "{firefox}\\browser\\chrome.manifest": {"mincount": 2, "maxcount": 2, "minbytes": 80, "maxbytes": 80}, > + "{firefox}\\msvcr100.dll": {"mincount": 24, "maxcount": 24, "minbytes": 1572864, "maxbytes": 1572864}, > + "{firefox}\\gkmedias.dll": {"mincount": 106, "maxcount": 106, "minbytes": 6946816, "maxbytes": 6946816}, > + "c:\\program files\\desktop.ini": {"mincount": 2, "maxcount": 2, "minbytes": 352, "maxbytes": 352} would be nice to have comments explaining what these #s come from. that said, YAGNI(now) and i'm guessing that these numbers are for "experts only"

fixed issue to remove error file if it already exists and landed: https://hg.mozilla.org/build/talos/rev/425eed1c8bff this still needs a deployment to go live.