Closed Bug 1073312 Opened 6 years ago Closed 6 years ago

Test DMD on TBPL (Linux)

Categories

(Core :: DMD, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file, 3 obsolete files)

DMD currently has compile coverage on TBPL (in debug builds) but it has no test coverage. It should.

There is a DMD test that you can run locally by starting up the browser with DMD='--mode=test' and then running memory/replace/dmd/check_test_output.py. glandium suggests that we can make an xpcshell test that does this, and preliminary testing suggests that it will work.

We might need to add some stuff to the xpcshell test harness to (a) set an environment variable before a test, and (b) run a checking script after a test.

Note to self: testing/xpcshell/runxpcshelltests.py will probably need modifying, and maybe testing/mozbase/manifestparser/manifestparser (the .ini parser). And there'll likely be a memory/replace/dmd/test/xpcshell.ini file.
Depends on: 1074008
Attached patch Test DMD on TBPL (obsolete) — Splinter Review
This passes when I run it by itself, locally, on Linux and Mac. But there are
lots of failures on a try run: https://tbpl.mozilla.org/?tree=Try&rev=96d112479757

Still, it's at a point where feedback would be useful. Some questions:

- It has /usr/bin/diff hardwired in, which is bad. What's the best way to
  compare the contents of two files from xpcshell code?

- I'm not sure what's the best thing to do with the intermediate files. The
  test*.json files go in the cwd, the subsequent files go in tmpdir, and I
  don't delete any of them but probably should.

- The setting of DMD may be affecting tests beyond this one. Also, I'll
  probably need to set other env vars (LD_PRELOAD, etc.) as well.
Attachment #8496742 - Flags: feedback?(mh+mozilla)
Comment on attachment 8496742 [details] [diff] [review]
Test DMD on TBPL

Review of attachment 8496742 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'm answering all your questions below.

::: memory/replace/dmd/dmd.py
@@ +90,5 @@
> +                matched = True
> +                break
> +        if matched:
> +            out(fmt.format(0, ': ... DMD.cpp ...'))
> +        return

No trace at all when there is no match?

::: memory/replace/dmd/test/test.js
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +
> +// This is a dummy file that never gets executed. What happens instead is that
> +// DMD takes control extremely early, runs its test operations and then exists,
> +// long before any execution would happen in this file.

This comment is probably wrong.

@@ +52,5 @@
> +  let diffFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +  let diffProcess = Cc["@mozilla.org/process/util;1"]
> +                      .createInstance(Components.interfaces.nsIProcess);
> +  // njn: hardwired, and assumes forward slashes
> +  diffFile.initWithPath('/usr/bin/diff');

This very likely doesn't work on Windows. I /think/ diff is in '/bin' there. But even then, that wouldn't work because /bin/diff is a msys path and here you're expected to give a windows path.
I don't know if we have a (minimalistic) js implementation of diff, but if we don't, maybe we should have one. Maybe we have something equivalent to python's assertEqual for lists?
Other than that, the test looks fine to me, but my xpcom is rusty. I don't know if we usually cleanup temporary files from tests. I'd say we probably do, but that's an assumption. At least I can find some evidence that we do. Check, for instance, the last bits of browser/devtools/scratchpad/test/browser_scratchpad_reset_undo.js. Try getting feedback from some frontend people?

::: memory/replace/dmd/test/xpcshell.ini
@@ +1,3 @@
> +[DEFAULT]
> +head =
> +tail =

I'm not sure you need the empty lines. If you do, please file a separate bug because that shouldn't be necessary.

::: testing/xpcshell/runxpcshelltests.py
@@ +589,5 @@
>          test_dir = os.path.dirname(name)
>          self.buildXpcsCmd(test_dir)
> +
> +        #print('\n\n\n', self.xpcsCmd, '\n\n\n')
> +        #self.env["DMD"] = '--mode=test'

I'm sure you can remove this.

@@ +1395,5 @@
>  
>              if self.testPath and name.find(self.testPath) == -1:
>                  continue
>  
> +            if 'dmd' in test_object:

if test_object.get('dmd') == 'true':

@@ +1398,5 @@
>  
> +            if 'dmd' in test_object:
> +                env = kwargs['env']
> +                env['DMD'] = '--mode=test'
> +                env['PYTHON'] = sys.executable

This block should go in XPCShellTestThread.run_test(), which would solve your bleeding environment problems.

You'll need the LD_LIBRARY_PATH/DYLD_LIBRARY_PATH/MOZ_REPLACE_MALLOC_LIB logic from automation.py. I don't know why the xpcshell test harness doesn't use automation.py (ted would know), but I won't ask you to make that happen :).
Attachment #8496742 - Flags: feedback?(mh+mozilla) → feedback+
Summary: Test DMD on TBPL → Test DMD on TBPL (Linux)
Blocks: 1076446
Blocks: 1077230
Attached patch Test DMD on TBPL (Linux-only) (obsolete) — Splinter Review
Things of note.

- I'm only running on Linux for now. On Mac, 10.8 works but 10.6 crashes after
  DMD runs its test mode. On Windows, DMD asserts part-way through its test
  mode, and the use of /usr/bin/diff is a problem. Dependent bugs are filed for
  these. (But note that DMD starts running correctly on both platforms, which
  is good.)

- DMD now doesn't print anything at startup when it's enabled but not running.
  This avoids lots of test spam in debug builds, which have DMD enabled.

- DMD's test mode now doesn't quit once it's finished. This allows the xpcshell
  test to do its thing afterwards with the produced files.

- Overly-clever compilers were optimizing the test code by omitting unused
  calls to malloc() and unrolling some loops. These break the tests, so I've
  made changes to foil these optimizations.

- check_test_output.py is gone. Good riddance.

- dmd.py needed some tweaking to work with breakpad symbols and to filter out
  non-deterministic stack traces differences.

- The xpcshell test is called test_full.js because it does full testing of
  DMD -- both the C++ code and the script. Later I will add another test called
  test_script.js that just does additional test of the dmd.py script.
Attachment #8499292 - Flags: review?(mh+mozilla)
Attachment #8496742 - Attachment is obsolete: true
Attached patch Test DMD on TBPL (Linux-only) (obsolete) — Splinter Review
Changes since the last version:

- I discovered FileUtils.jsm, which makes test_full.js a bit nicer.

- On Mac, the setting of DYLD_INSERT_LIBRARIES is busted. I have a comment
  (marked with "njn") explaining what's going on. Suggestions are welcome!
Attachment #8500180 - Flags: review?(mh+mozilla)
Attachment #8499292 - Attachment is obsolete: true
Attachment #8499292 - Flags: review?(mh+mozilla)
Blocks: 1078979
This updated patch fixes the Mac preload path issues by munging self.xrePath.
Attachment #8500863 - Flags: review?(mh+mozilla)
Attachment #8500180 - Attachment is obsolete: true
Attachment #8500180 - Flags: review?(mh+mozilla)
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

Review of attachment 8500863 [details] [diff] [review]:
-----------------------------------------------------------------

As said previously, my XPCOM js is rusty, can you find someone to double-check the test code?

::: memory/replace/dmd/DMD.cpp
@@ +1891,5 @@
> +  // This test relies on the compiler not doing various optimizations, such as
> +  // eliding unused malloc() calls or unrolling loops with fixed iteration
> +  // counts. So we want a constant value that the compiler can't determine
> +  // statically, and we use that in various ways to prevent the above
> +  // optimizations from happening.

Why not just use pragmas that disable optimizations?

#pragma optimize("", off) // for MSVC
#pragma GCC push_options
#pragma GCC optimize("O0")
...
#pragma GCC pop_options
#pragma optimize("", on) // for MSVC

... although I'm not sure if clang has support for that yet.

You could also extract the code that needs never be optimized in a separate source file, and build that source file with -O0.

@@ +1924,5 @@
>    }
>    free(a);
>  
> +  // Note: 8 bytes is the smallest requested size that gives consistent
> +  // behaviour across all platforms with jemalloc.

s/platforms/architectures/. There should be no difference between osx, linux and windows for a given CPU instruction set.

::: testing/xpcshell/runxpcshelltests.py
@@ +617,5 @@
> +                contents_dir, _ = os.path.split(self.xrePath)
> +                libdmd = os.path.join(contents_dir, 'MacOS', 'libdmd.dylib')
> +            elif sys.platform == 'win32':
> +                preloadEnvVar = 'MOZ_REPLACE_MALLOC_LIB'
> +                libdmd = os.path.join(self.xrePath, 'dmd.dll')

Please file a followup to consolidate this with all the other places that do the same thing.
Attachment #8500863 - Flags: review?(mh+mozilla) → review+
> 
> Why not just use pragmas that disable optimizations?
> ... although I'm not sure if clang has support for that yet.

Because clang doesn't have support :)


> > +  // Note: 8 bytes is the smallest requested size that gives consistent
> > +  // behaviour across all platforms with jemalloc.
> 
> s/platforms/architectures/. There should be no difference between osx, linux
> and windows for a given CPU instruction set.

Linux is different with mozjemalloc -- it always allocates a word. On other platforms there are 2-byte and 4-byte size classes.
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

ttaubert suggested you as an xpcshell test expert. Can you please review just test_dmd.js for general xpcshell concerns. Thanks!
Attachment #8500863 - Flags: review?(ttaubert)
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

Review of attachment 8500863 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/runxpcshelltests.py
@@ +613,5 @@
> +            elif sys.platform == 'osx' or sys.platform == 'darwin':
> +                preloadEnvVar = 'DYLD_INSERT_LIBRARIES'
> +                # self.xrePath is <prefix>/Contents/Resources.
> +                # We need <prefix>/Contents/MacOS/libdmd.dylib.
> +                contents_dir, _ = os.path.split(self.xrePath)

contents_dir = os.path.dirname(self.xrePath)
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

(In reply to Nicholas Nethercote [:njn] from comment #8)
> ttaubert suggested you as an xpcshell test expert. Can you please review
> just test_dmd.js for general xpcshell concerns. Thanks!

I'm not an xpcshell expert, I have written a few tests but that's it. Did you maybe mean someone else? Sorry.
Attachment #8500863 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 8500863 [details] [diff] [review]
> Test DMD on TBPL (Linux-only)
> 
> (In reply to Nicholas Nethercote [:njn] from comment #8)
> > ttaubert suggested you as an xpcshell test expert. Can you please review
> > just test_dmd.js for general xpcshell concerns. Thanks!

I think you meant glandium instead of ttaubert at the beginning of that sentence, but I was probably not clear enough. I was suggesting ttaubert as someone who might know better than I do who could take a look at this.

> I'm not an xpcshell expert, I have written a few tests but that's it. Did
> you maybe mean someone else? Sorry.

Thus the following question: Tim, do you know who could take a look at the xpcshell test?
(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Tim Taubert [:ttaubert] from comment #10)
> > I'm not an xpcshell expert, I have written a few tests but that's it. Did
> > you maybe mean someone else? Sorry.
> 
> Thus the following question: Tim, do you know who could take a look at the
> xpcshell test?

David I think has a lot of experience with writing xpcshell tests. I hope you can help here?
Flags: needinfo?(dteller)
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

jaws said he would take a look.

jaws: it's a slightly strange test. DMD's "test mode" runs before this code executes, and that produces some JSON files which this code uses as inputs. It then runs dmd.py over those JSON files and produces some output, which is then compared with the expected output.

glandium's ok'd the rest of the patch, but if you could look at test_dmd.js to see that I'm not doing anything bad xpcshell-wise, that'd be great. Thank you.
Attachment #8500863 - Flags: review?(jaws)
Flags: needinfo?(dteller)
Attachment #8500863 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
I can look at the test here later today, but I lack approximately a boatload of context. What is DMD?
DMD is documented here: https://wiki.mozilla.org/Performance/MemShrink/DMD

Really, I just want to know if test_dmd.js is doing anything egregiously stupid. Thanks.
Comment on attachment 8500863 [details] [diff] [review]
Test DMD on TBPL (Linux-only)

Review of attachment 8500863 [details] [diff] [review]:
-----------------------------------------------------------------

I don't want to stall what otherwise looks like a considerable amount of nice work, but I do think it'd be better to avoid using diff here. Please see my suggestion below; I could live with landing it with the other nits fixed and the rework to avoid diff done in the windows bug that the patch mentions.

::: memory/replace/dmd/test/test_dmd.js
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

Nit: const {classes: Cc, interfaces: Ci, utils: Cu} = Components

@@ +7,5 @@
> +
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +
> +// The xpcshell test harness sets PYTHON so we can read it here.
> +let gEnv = Cc['@mozilla.org/process/environment;1']

Nit: unlike in python, in JS we lean to using double quotes except for one-character strings.

@@ +15,5 @@
> +// If we're testing locally, the script is in 'CurProcD'. Otherwise, it is in
> +// another location that we have to find.
> +let gDmdScriptFile = FileUtils.getFile('CurProcD', ['dmd.py']);
> +if (!gDmdScriptFile.exists()) {
> +  gDmdScriptFile = FileUtils.getFile('CurWorkD', [])

Nit: semicolon

@@ +16,5 @@
> +// another location that we have to find.
> +let gDmdScriptFile = FileUtils.getFile('CurProcD', ['dmd.py']);
> +if (!gDmdScriptFile.exists()) {
> +  gDmdScriptFile = FileUtils.getFile('CurWorkD', [])
> +  while (gDmdScriptFile.path.indexOf('xpcshell') != -1) {

Nit: while (gDmdScriptFile.path.contains('xpcshell')) {

@@ +29,5 @@
> +  // |actualFile| for consistency. It is removed once we've finished.
> +  let expectedFile =
> +    FileUtils.getFile('CurWorkD', ['full-' + aKind + '-expected' + aN + '.txt'])
> +  let actualFile =
> +    FileUtils.getFile('CurWorkD', ['full-' + aKind + '-actual'   + aN + '.txt'])

Nit: semicolons (also on the previous statement)

@@ +51,5 @@
> +
> +  // Compare |expectedFile| with |actualFile|. Difference are printed to
> +  // stdout.
> +
> +  let diffFile = new FileUtils.File("/usr/bin/diff");

This seems far too fragile, aside from not working on Windows. It seems like it'd be better to just read both JSON files and compare with deepEqual?

@@ +77,5 @@
> +  // asynchronously.
> +  for (let i = 1; i <= 4; i++) {
> +      let jsonFile = FileUtils.getFile('CurWorkD', ['full' + i + '.json']);
> +      test(jsonFile, 'heap', ['--ignore-reports'], i)
> +      test(jsonFile, 'reports', [], i)

Nit: semicolons, including on the previous line.
Attachment #8500863 - Flags: review?(gijskruitbosch+bugs) → feedback+
Thanks, Gijs! I've fixed all the nits.

On the diff front: my plan (and what my in-progress Windows patch does) is to attempt to use /usr/bin/diff in a try block, and if an exception is thrown, to fall back to reading the files directly and using a string comparison. The motivation here is that it's *much* nicer to get a proper diff when something goes wrong. The alternative is for the test to print out the expected and actual results, and then you have to get the log and strip out everything except those results, and put it into two files and then do a diff. Which is doable, but I've had to do a lot of try runs to get this test working on Linux and Mac and so it's been a great help to have the program do the diff itself.
I'm also *very* keen to land this, since we currently have no continuous testing of DMD :)
(In reply to Nicholas Nethercote [:njn] from comment #17)
> Thanks, Gijs! I've fixed all the nits.
> 
> On the diff front: my plan (and what my in-progress Windows patch does) is
> to attempt to use /usr/bin/diff in a try block, and if an exception is
> thrown, to fall back to reading the files directly and using a string
> comparison. The motivation here is that it's *much* nicer to get a proper
> diff when something goes wrong. The alternative is for the test to print out
> the expected and actual results, and then you have to get the log and strip
> out everything except those results, and put it into two files and then do a
> diff. Which is doable, but I've had to do a lot of try runs to get this test
> working on Linux and Mac and so it's been a great help to have the program
> do the diff itself.

Did you try to see what deepEqual prints in case of inequality?
https://hg.mozilla.org/mozilla-central/rev/b30e3a050567
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.