I didn't check that |make check-ooms| was actually triggering on tinderbox, just that I didn't break anything else. This explains why the number of OOMs is still fluctuating. So I said "ifndef DEBUG", which is always false. I should have been checking that the value of MOZ_DEBUG is 1, since MOZ_DEBUG is what --enable-debug actually sets (in rules.mk).

Comment on attachment 523242 [details] [diff] [review] Fix makefile condition I tried this on tryserver, and it times out due to no output. I'll prepare a new patch that throws in some output to allow this.

http://hg.mozilla.org/tracemonkey/rev/ee8f319bc315 And then I disabled it because the SM tests use a weird directory structure: http://hg.mozilla.org/tracemonkey/rev/b0f4398a0657

Tis probably about time we had someone look at this Python. This simplifies the OOM checker by fetching the command to run from jit-test.py rather than building it itself. It also tidies up a bit of code, adds comment and makes things a little cleaner. On the Makefile.in side, I tried a few different ways to make this work. The problem is that different configure flags lead to different OOM counts. I actually counted the number of OOMs in each variation of JITs, but I don't think it's reasonable to expect others to keep that updated, so I removed them and just went with the counts for both JITs enabled. If/when we get a handle on this, we can work on the other moving parts.

Comment on attachment 523955 [details] [diff] [review] Make check-ooms The file-handling code is not very Pythonic. Here, I would import find_tests from jit_test.py and use that. relpath() in that file might also be useful here. > def get_js_files(): >- (out, err, exit) = run('find ../jit-test/tests -name "*.js"') >- if (err, exit) != ("", 0): >- sys.exit("Wrong directory, run from an objdir") >- return out.split() >+ jittest_dir = os.path.dirname(jittest) >+ tests_dir = os.path.join(jittest_dir, "tests") >+ out = check_run('find %s -name "*.js"' % (tests_dir)) >+ files = out.split() >+ >+ # remove the prefix, leaving a string jit-test.py can use as a pattern >+ files = [f[len(tests_dir)+1:] for f in files] >+ >+ return files Similarily, here it would be better to import get_test_cmd and use that. You might need to refactor a bit to get everything you need in there. >-command_template = 'shell/js' \ >- + ' -m -j -p' \ >- + ' -e "const platform=\'darwin\'; const libdir=\'../jit-test/lib/\';"' \ >- + ' -f ../jit-test/lib/prolog.js' \ >- + ' -f %s' >+jittest = os.path.join(os.path.dirname(sys.argv[0]), '..', 'jit-test', 'jit_test.py') >+ >+def get_command_template(file): >+ >+ # Actually run jit_test.py to find out it's command line arguments >+ command = "%s %s -s %s" % (jittest, "js", file) >+ out = check_run(command) >+ >+ # first line only >+ template = out.split("\n")[0] >+ >+ # Cut off the filename at the end >+ template = template[:len(template)-len(file)] >+ >+ return template > > > # Blacklists are things we don't want to see in our logs again (though we do > # want to count them when they happen). Whitelists we do want to see in our > # logs again, principally because the information we have isn't enough. > > blacklist = {} > add_to_blacklist(r"('', '', 1)") # 1 means OOM if the shell hasn't launched yet. >@@ -191,53 +220,53 @@ whitelist.add(r"('', 'out of memory\nout > # Options > parser = OptionParser(usage=usage) > parser.add_option("-r", "--regression", action="store", metavar="REGRESSION_COUNT", help=help, > type="int", dest="regression", default=None) > > (OPTIONS, args) = parser.parse_args() > > >+files = get_js_files() > if OPTIONS.regression != None: > # TODO: This should be expanded as we get a better hang of the OOM problems. > # For now, we'll just check that the number of OOMs in one short file does not > # increase. >- files = ["../jit-test/tests/arguments/args-createontrace.js"] >-else: >- files = get_js_files() >+ files = files[0:1] > > # Use a command-line arg to reduce the set of files > if len (args): > files = [f for f in files if f.find(args[0]) != -1] > > > if OPTIONS.regression == None: > # Don't use a logfile, this is automated for tinderbox. > log = file("../OOM_log", "w") > > >+command_template = get_command_template(files[0]) > num_failures = 0 > for f in files: > > # Run it once to establish boundaries >- command = (command_template + ' -O') % (f) >- out, err, exit = run(command) >+ command = "%s%s -O" % (command_template, f) >+ out = check_run(command) > max = re.match(".*OOM max count: (\d+).*", out, flags=re.DOTALL).groups()[0] > max = int(max) > > # OOMs don't recover well for the first 20 allocations or so. > # TODO: revisit this. > for i in range(20, max): > > if OPTIONS.regression == None: > print "Testing allocation %d/%d in %s" % (i,max,f) > else: > sys.stdout.write('.') # something short for tinderbox, no space or \n > >- command = (command_template + ' -A %d') % (f, i) >+ command = "%s%s -A %d" % (command_template, f, i) > out, err, exit = run(command) > > # Success (5 is SM's exit code for controlled errors) > if exit == 5 and err.find("out of memory") != -1: > continue > > # Failure > else: >diff --git a/js/src/Makefile.in b/js/src/Makefile.in >--- a/js/src/Makefile.in >+++ b/js/src/Makefile.in >@@ -598,21 +598,32 @@ check-vanilla-new: > > ifeq ($(OS_ARCH),Linux) > check:: check-vanilla-new > endif > > # Help ensure that the number of OOM errors in SpiderMonkey doesn't increase. > # If the number of OOM errors changes, update the number below. We intend this > # number to go down over time, by fixing OOMs. >+ifdef ENABLE_METHODJIT >+ifdef ENABLE_TRACEJIT >+NUM_OOMS=137 >+endif >+endif >+ >+ifdef NUM_OOMS > check-ooms: >- $(wildcard $(RUN_TEST_PROGRAM)) $(PYTHON) -u $(srcdir)/config/find_OOM_errors.py --regression 125 >+ $(wildcard $(RUN_TEST_PROGRAM)) $(PYTHON) -u $(srcdir)/config/find_OOM_errors.py --regression $(NUM_OOMS) >+else >+check-ooms: >+ @echo "TEST-PASS | find_OOM_errors.py | We don't track OOMs in this configuration (SKIP)" >+endif > > ifeq ($(MOZ_DEBUG),1) >-#check:: check-ooms >+check:: check-ooms > endif > > ## Prevent regressing in our deprecation of non-preferred memory management functions. > # We use all the files in the distribution so that different configurations > # don't give different results. We skip the contents of objdirs using |find| > # (it can't be done with %-expansion, because the files we want to skip aren't > # in the vpath). > ALL_FILES=$(shell find $(srcdir) \( -name "*.cpp" -o -name "*.h" \) -not -path "*/dist/*") >diff --git a/js/src/config/find_OOM_errors.py b/js/src/config/find_OOM_errors.py >--- a/js/src/config/find_OOM_errors.py >+++ b/js/src/config/find_OOM_errors.py >@@ -14,22 +14,36 @@ result if more or less errors are found. > > import hashlib > import re > import shlex > import subprocess > import sys > import threading > import time >+import os > > from optparse import OptionParser > > ##################################################################### > # Utility functions > ##################################################################### >+ >+def check_run(command, *args, **kwargs): >+ (out, err, exit) = run(command, *args, **kwargs) >+ if (err, exit) != ("", 0): >+ print "Error: " + str(command) >+ print out >+ print err >+ print exit >+ sys.exit(-1) >+ >+ return out >+ >+ > def run(args, stdin=None): > class ThreadWorker(threading.Thread): > def __init__(self, pipe): > super(ThreadWorker, self).__init__() > self.all = "" > self.pipe = pipe > self.setDaemon(True) > >@@ -64,20 +78,25 @@ def run(args, stdin=None): > except KeyboardInterrupt, e: > sys.exit(-1) > > stdout, stderr = stdout_worker.all, stderr_worker.all > result = (stdout, stderr, proc.returncode) > return result > > def get_js_files(): >- (out, err, exit) = run('find ../jit-test/tests -name "*.js"') >- if (err, exit) != ("", 0): >- sys.exit("Wrong directory, run from an objdir") >- return out.split() >+ jittest_dir = os.path.dirname(jittest) >+ tests_dir = os.path.join(jittest_dir, "tests") >+ out = check_run('find %s -name "*.js"' % (tests_dir)) >+ files = out.split() >+ >+ # remove the prefix, leaving a string jit-test.py can use as a pattern >+ files = [f[len(tests_dir)+1:] for f in files] >+ >+ return files > > > > ##################################################################### > # Blacklisting > ##################################################################### > def in_blacklist(sig): > return sig in blacklist >@@ -158,21 +177,31 @@ def clean_output(err): > > return err > > > ##################################################################### > # Consts, etc > ##################################################################### > >-command_template = 'shell/js' \ >- + ' -m -j -p' \ >- + ' -e "const platform=\'darwin\'; const libdir=\'../jit-test/lib/\';"' \ >- + ' -f ../jit-test/lib/prolog.js' \ >- + ' -f %s' >+jittest = os.path.join(os.path.dirname(sys.argv[0]), '..', 'jit-test', 'jit_test.py') >+ >+def get_command_template(file): >+ >+ # Actually run jit_test.py to find out it's command line arguments >+ command = "%s %s -s %s" % (jittest, "js", file) >+ out = check_run(command) >+ >+ # first line only >+ template = out.split("\n")[0] >+ >+ # Cut off the filename at the end >+ template = template[:len(template)-len(file)] >+ >+ return template > > > # Blacklists are things we don't want to see in our logs again (though we do > # want to count them when they happen). Whitelists we do want to see in our > # logs again, principally because the information we have isn't enough. > > blacklist = {} > add_to_blacklist(r"('', '', 1)") # 1 means OOM if the shell hasn't launched yet. >@@ -191,53 +220,53 @@ whitelist.add(r"('', 'out of memory\nout > # Options > parser = OptionParser(usage=usage) > parser.add_option("-r", "--regression", action="store", metavar="REGRESSION_COUNT", help=help, > type="int", dest="regression", default=None) > > (OPTIONS, args) = parser.parse_args() > > >+files = get_js_files() > if OPTIONS.regression != None: > # TODO: This should be expanded as we get a better hang of the OOM problems. > # For now, we'll just check that the number of OOMs in one short file does not > # increase. >- files = ["../jit-test/tests/arguments/args-createontrace.js"] >-else: >- files = get_js_files() >+ files = files[0:1] > > # Use a command-line arg to reduce the set of files > if len (args): > files = [f for f in files if f.find(args[0]) != -1] > > > if OPTIONS.regression == None: > # Don't use a logfile, this is automated for tinderbox. > log = file("../OOM_log", "w") > > >+command_template = get_command_template(files[0]) > num_failures = 0 > for f in files: > > # Run it once to establish boundaries >- command = (command_template + ' -O') % (f) >- out, err, exit = run(command) >+ command = "%s%s -O" % (command_template, f) >+ out = check_run(command) > max = re.match(".*OOM max count: (\d+).*", out, flags=re.DOTALL).groups()[0] > max = int(max) > > # OOMs don't recover well for the first 20 allocations or so. > # TODO: revisit this. > for i in range(20, max): > > if OPTIONS.regression == None: > print "Testing allocation %d/%d in %s" % (i,max,f) > else: > sys.stdout.write('.') # something short for tinderbox, no space or \n > >- command = (command_template + ' -A %d') % (f, i) >+ command = "%s%s -A %d" % (command_template, f, i) > out, err, exit = run(command) > > # Success (5 is SM's exit code for controlled errors) > if exit == 5 and err.find("out of memory") != -1: > continue > > # Failure > else:

(In reply to comment #5) > Comment on attachment 523955 [details] [diff] [review] > Make check-ooms > > The file-handling code is not very Pythonic. > > Here, I would import find_tests from jit_test.py and use that. relpath() in > that file might also be useful here. > > Similarily, here it would be better to import get_test_cmd and use that. You > might need to refactor a bit to get everything you need in there. Never though of iporting jit_test.py. That's much more elegant.

This rewrites a lot of find_OOM_errors to reuse the code from jit_test.py. Since there is a merge of jit-test and jstests in bug 638219, I'm going to hold off on this til djc is done.

Iain, does `make check-ooms` warrant this? (or whether it is still relevant)

This code hasn't been enabled since 2011: https://searchfox.org/mozilla-central/source/js/src/Makefile.in#30 At the time, it was disabled because it didn't work well. It is basically a clunky precursor of oomTest, written as a python script wrapping the shell. It relies on the obsolete "-A <allocations_before_oom>" infrastructure, which no longer exists. In short, there's no reason for this code to still be hanging around. Let's delete it.

Pushed by iireland@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb5fbd4432e3 Remove obsolete |make check-ooms| code r=tcampbell,froydnj