Closed Bug 646763 Opened 13 years ago Closed 5 years ago

|make check-ooms| doesn't work and should be removed

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: paul.biggar, Assigned: iain)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Fix makefile condition (obsolete) — Splinter Review
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)
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)
Attached patch Fix |make check-ooms| (obsolete) — Splinter Review
Attachment #523242 - Attachment is obsolete: true
Attachment #523836 - Flags: review?(nnethercote)
Attachment #523836 - Flags: review?(nnethercote) → review+
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
Attached patch Make check-ooms (obsolete) — Splinter Review
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 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-
(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.
Attachment #523955 - Attachment is obsolete: true
Assignee: paul.biggar → general
Assignee: general → nobody
Iain, does `make check-ooms` warrant this? (or whether it is still relevant)
Flags: needinfo?(iireland)
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
Attachment #533693 - Attachment is obsolete: true
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb5fbd4432e3
Remove obsolete |make check-ooms| code r=tcampbell,froydnj
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.