Closed
Bug 646763
Opened 14 years ago
Closed 6 years ago
|make check-ooms| doesn't work and should be removed
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: paul.biggar, Assigned: iain)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
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).
Attachment #523242 -
Flags: review?(nnethercote)
Reporter | ||
Comment 1•14 years ago
|
||
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.
Attachment #523242 -
Flags: review?(nnethercote)
Reporter | ||
Comment 2•14 years ago
|
||
Attachment #523242 -
Attachment is obsolete: true
Attachment #523836 -
Flags: review?(nnethercote)
Updated•14 years ago
|
Attachment #523836 -
Flags: review?(nnethercote) → review+
Reporter | ||
Comment 3•14 years ago
|
||
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
Reporter | ||
Comment 4•14 years ago
|
||
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.
Attachment #523836 -
Attachment is obsolete: true
Attachment #523955 -
Flags: review?(dmandelin)
Comment 5•14 years ago
|
||
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:
Attachment #523955 -
Flags: review?(dmandelin) → review-
Reporter | ||
Comment 6•14 years ago
|
||
(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.
Reporter | ||
Comment 7•13 years ago
|
||
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.
Attachment #523955 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Assignee: paul.biggar → general
Updated•10 years ago
|
Assignee: general → nobody
Iain, does `make check-ooms` warrant this? (or whether it is still relevant)
Flags: needinfo?(iireland)
Assignee | ||
Comment 9•6 years ago
|
||
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.
Assignee: nobody → iireland
Flags: needinfo?(iireland)
Summary: |make check-ooms| doesn't work → |make check-ooms| doesn't work and should be removed
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #533693 -
Attachment is obsolete: true
Comment 11•6 years ago
|
||
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb5fbd4432e3
Remove obsolete |make check-ooms| code r=tcampbell,froydnj
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 12•6 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•