Fix some issues with opt-viewer tests, plus disable them on Windows

[zturner]

Summary

Back in D40202 I reported an issue that cpu_count() wasn't working properly. At the time I thought this was only for me since no bots were failing, but as it turns out this is happening across the board on Windows. The reason I'm the only one that caught it is because I'm the only who has all the necessary dependencies installed so that this test would run at all. So for everyone else, the test was just being treated as Unsupported.

I managed to fix many of the issues, but there is still a race condition that I've been unable to resolve. Specifically, I get this call stack:

Traceback (most recent call last):
  File "D:/src/llvm-mono/llvm/tools/opt-viewer/opt-viewer.py", line 339, in <module>
    main()
  File "D:/src/llvm-mono/llvm/tools/opt-viewer/opt-viewer.py", line 336, in main
    print_progress)
  File "D:/src/llvm-mono/llvm/tools/opt-viewer/opt-viewer.py", line 251, in generate_report
    should_print_progress)
  File "D:\src\llvm-mono\llvm\tools\opt-viewer\optpmap.py", line 50, in pmap
    result = pool.map(_wrapped_func, func_and_args, *args, **kwargs)
  File "C:\Python27\lib\multiprocessing\pool.py", line 251, in map
    return self.map_async(func, iterable, chunksize).get()
  File "C:\Python27\lib\multiprocessing\pool.py", line 567, in get
    raise self._value
AttributeError: type object 'Passed' has no attribute 'demangler_lock'

error: command failed with exit status: 1

I know it's a race condition because if I pass --jobs=1 then the tests pass. Since I don't know this code well, I'm not going to investigate this further, but I did want to at least try to get the rest of the issues I've found resolved.

The main source of issues comes from a fundamental difference in the way Windows and Linux handle multiprocessing in Python. If you have a global variable named args, and then you run a bunch of workers in a multiprocessing.Pool, on Windows args will be undefined but on non-Windows it will be defined. I managed to fix all of these by propagating the values through to the functions and classes that needed them, and I also fixed the cpu_count() issue by having the command line argument default to None if it's not specified. This way the multiprocessing module will figure this out internally (the documentation says that if processes=None, it will use the cpu count. This method works on Windows, even though (for some reason) calling cpu_count() directly doesn't.

To ensure that the global variable issue doesn't creep back in, I've wrapped the core logic of the script in a main() function and then call that function. This way args isn't a global variable, so if someone is relying on it being a global variable it will fail on non-Windows from now on as well.

Someone else will need to investigate this outstanding race condition. I don't believe it's specific to Windows, so in theory it should be reproducible anywhere.

[rnk]

Python stuff looks good, but @anemet owns this so I'd ask him.

[anemet]

LGTM too. Thanks for getting back to this!

llvm/tools/opt-viewer/opt-stats.py

14 ↗

(On Diff #128787)

Do we still need this after your change?

llvm/trunk/test/tools/opt-viewer/lit.local.cfg

+import sys
+
 if 'have_opt_viewer_modules' not in config.available_features:
     config.unsupported = True
+
+# Windows has different multiprocessing behavior than non-Windows, which causes
+# all of the tests to fail presently under Windows.  Disable this until those
+# can be resolved.
+if sys.platform == 'win32':
+    config.unsupported = True

llvm/trunk/tools/opt-viewer/opt-diff.py

 try:
     from yaml import CLoader as Loader
 except ImportError:
     from yaml import Loader
 
 import optrecord
 import argparse
 from collections import defaultdict
-from multiprocessing import cpu_count, Pool
 
 if __name__ == '__main__':
     parser = argparse.ArgumentParser(description=desc)
     parser.add_argument(
         'yaml_dir_or_file_1',
         help='An optimization record file or a directory searched for optimization '
              'record files that are used as the old version for the comparison')
     parser.add_argument(
         'yaml_dir_or_file_2',
         help='An optimization record file or a directory searched for optimization '
              'record files that are used as the new version for the comparison')
     parser.add_argument(
         '--jobs',
         '-j',
-        default=cpu_count(),
+        default=None,
         type=int,
         help='Max job count (defaults to %(default)s, the current CPU count)')
     parser.add_argument(
         '--no-progress-indicator',
         '-n',
         action='store_true',
         default=False,
         help='Do not display any indicator of how many YAML files were read.')

llvm/trunk/tools/opt-viewer/opt-stats.py

if __name__ == '__main__':
     parser.add_argument(
         'yaml_dirs_or_files',
         nargs='+',
         help='List of optimization record files or directories searched '
              'for optimization record files.')
     parser.add_argument(
         '--jobs',
         '-j',
-        default=cpu_count(),
+        default=None,
         type=int,
         help='Max job count (defaults to %(default)s, the current CPU count)')
     parser.add_argument(
         '--no-progress-indicator',
         '-n',
         action='store_true',
         default=False,
         help='Do not display any indicator of how many YAML files were read.')

llvm/trunk/tools/opt-viewer/opt-viewer.py

 def suppress(remark):
     if remark.Name == 'sil.Specialized':
         return remark.getArgDict()['Function'][0].startswith('\"Swift.')
     elif remark.Name == 'sil.Inlined':
         return remark.getArgDict()['Callee'][0].startswith(('\"Swift.', '\"specialized Swift.'))
     return False
 
 class SourceFileRenderer:
-    def __init__(self, source_dir, output_dir, filename):
+    def __init__(self, source_dir, output_dir, filename, no_highlight):
         existing_filename = None
         if os.path.exists(filename):
             existing_filename = filename
         else:
             fn = os.path.join(source_dir, filename)
             if os.path.exists(fn):
                 existing_filename = fn
 
+        self.no_highlight = no_highlight
         self.stream = codecs.open(os.path.join(output_dir, optrecord.html_file_name(filename)), 'w', encoding='utf-8')
         if existing_filename:
             self.source_stream = open(existing_filename)
         else:
             self.source_stream = None
             print('''
 <html>
 <h1>Unable to locate file {}</h1>
 </html>
             '''.format(filename), file=self.stream)
 
         self.html_formatter = HtmlFormatter(encoding='utf-8')
         self.cpp_lexer = CppLexer(stripnl=False)
 
     def render_source_lines(self, stream, line_remarks):
         file_text = stream.read()
 
-        if args.no_highlight:
+        if self.no_highlight:
             html_highlighted = file_text.decode('utf-8')
         else:
             html_highlighted = highlight(
             file_text,
                 self.cpp_lexer,
                 self.html_formatter)
 
             # Note that the API is different between Python 2 and 3.  On
<tbody>''', file=self.stream)
         print('''
 </tbody>
 </table>
 </body>
 </html>''', file=self.stream)
 
 
 class IndexRenderer:
-    def __init__(self, output_dir, should_display_hotness):
+    def __init__(self, output_dir, should_display_hotness, max_hottest_remarks_on_index):
         self.stream = codecs.open(os.path.join(output_dir, 'index.html'), 'w', encoding='utf-8')
         self.should_display_hotness = should_display_hotness
+        self.max_hottest_remarks_on_index = max_hottest_remarks_on_index
 
     def render_entry(self, r, odd):
         escaped_name = cgi.escape(r.DemangledFunctionName)
         print(u'''
 <tr>
 <td class=\"column-entry-{odd}\"><a href={r.Link}>{r.DebugLocString}</a></td>
 <td class=\"column-entry-{odd}\">{r.RelativeHotness}</td>
 <td class=\"column-entry-{odd}\">{escaped_name}</td>

 <tr>
 <td>Source Location</td>
 <td>Hotness</td>
 <td>Function</td>
 <td>Pass</td>
 </tr>''', file=self.stream)
 
         max_entries = None
-        if should_display_hotness:
-            max_entries = args.max_hottest_remarks_on_index
+        if self.should_display_hotness:
+            max_entries = self.max_hottest_remarks_on_index
 
         for i, remark in enumerate(all_remarks[:max_entries]):
             if not suppress(remark):
                 self.render_entry(remark, i % 2)
         print('''
 </table>
 </body>
 </html>''', file=self.stream)
 
 
-def _render_file(source_dir, output_dir, ctx, entry):
+def _render_file(source_dir, output_dir, ctx, no_highlight, entry):
     global context
     context = ctx
     filename, remarks = entry
-    SourceFileRenderer(source_dir, output_dir, filename).render(remarks)
+    SourceFileRenderer(source_dir, output_dir, filename, no_highlight).render(remarks)
 
 
 def map_remarks(all_remarks):
     # Set up a map between function names and their source location for
     # function where inlining happened
     for remark in optrecord.itervalues(all_remarks):
         if isinstance(remark, optrecord.Passed) and remark.Pass == "inline" and remark.Name == "Inlined":
             for arg in remark.Args:
                 arg_dict = dict(list(arg))
                 caller = arg_dict.get('Caller')
                 if caller:
                     try:
                         context.caller_loc[caller] = arg_dict['DebugLoc']
                     except KeyError:
                         pass
 
 
 def generate_report(all_remarks,
                     file_remarks,
                     source_dir,
                     output_dir,
+                    no_highlight,
                     should_display_hotness,
+                    max_hottest_remarks_on_index,
                     num_jobs,
                     should_print_progress):
     try:
         os.makedirs(output_dir)
     except OSError as e:
         if e.errno == errno.EEXIST and os.path.isdir(output_dir):
             pass
         else:
             raise
 
-    _render_file_bound = functools.partial(_render_file, source_dir, output_dir, context)
+    _render_file_bound = functools.partial(_render_file, source_dir, output_dir, context, no_highlight)
     if should_print_progress:
         print('Rendering HTML files...')
     optpmap.pmap(_render_file_bound,
                  file_remarks.items(),
                  num_jobs,
                  should_print_progress)
 
     if should_display_hotness:
         sorted_remarks = sorted(optrecord.itervalues(all_remarks), key=lambda r: (r.Hotness, r.File, r.Line, r.Column, r.PassWithDiffPrefix, r.yaml_tag, r.Function), reverse=True)
     else:
         sorted_remarks = sorted(optrecord.itervalues(all_remarks), key=lambda r: (r.File, r.Line, r.Column, r.PassWithDiffPrefix, r.yaml_tag, r.Function))
-    IndexRenderer(args.output_dir, should_display_hotness).render(sorted_remarks)
+    IndexRenderer(output_dir, should_display_hotness, max_hottest_remarks_on_index).render(sorted_remarks)
 
     shutil.copy(os.path.join(os.path.dirname(os.path.realpath(__file__)),
             "style.css"), output_dir)
 
 
-if __name__ == '__main__':
+def main():
     parser = argparse.ArgumentParser(description=desc)
     parser.add_argument(
         'yaml_dirs_or_files',
         nargs='+',
         help='List of optimization record files or directories searched '
              'for optimization record files.')
     parser.add_argument(
         '--output-dir',
         '-o',
         default='html',
         help='Path to a directory where generated HTML files will be output. '
              'If the directory does not already exist, it will be created. '
              '"%(default)s" by default.')
     parser.add_argument(
         '--jobs',
         '-j',
-        default=cpu_count(),
+        default=None,
         type=int,
         help='Max job count (defaults to %(default)s, the current CPU count)')
     parser.add_argument(
         '--source-dir',
         '-s',
         default='',
         help='set source directory')
     parser.add_argument(
def main():
     parser.add_argument(
         '--no-highlight',
         action='store_true',
         default=False,
         help='Do not use a syntax highlighter when rendering the source code')
     parser.add_argument(
         '--demangler',
         help='Set the demangler to be used (defaults to %s)' % optrecord.Remark.default_demangler)
+
+    # Do not make this a global variable.  Values needed to be propagated through
+    # to individual classes and functions to be portable with multiprocessing across
+    # Windows and non-Windows.
     args = parser.parse_args()
 
     print_progress = not args.no_progress_indicator
     if args.demangler:
         optrecord.Remark.set_demangler(args.demangler)
 
     files = optrecord.find_opt_files(*args.yaml_dirs_or_files)
     if not files:
         parser.error("No *.opt.yaml files found")
         sys.exit(1)
 
     all_remarks, file_remarks, should_display_hotness = \
         optrecord.gather_results(files, args.jobs, print_progress)
 
     map_remarks(all_remarks)
 
     generate_report(all_remarks,
                     file_remarks,
                     args.source_dir,
                     args.output_dir,
+                    args.no_highlight,
                     should_display_hotness,
+                    args.max_hottest_remarks_on_index,
                     args.jobs,
                     print_progress)
+
+if __name__ == '__main__':
+    main()

llvm/trunk/tools/opt-viewer/optpmap.py

def pmap(func, iterable, processes, should_print_progress, *args, **kwargs):
     of the functions have finished being run.
     """
     global _current
     global _total
     _current = multiprocessing.Value('i', 0)
     _total = multiprocessing.Value('i', len(iterable))
 
     func_and_args = [(func, arg, should_print_progress,) for arg in iterable]
-    if processes <= 1:
+    if processes == 1:
         result = map(_wrapped_func, func_and_args, *args, **kwargs)
     else:
         pool = multiprocessing.Pool(initializer=_init,
                                     initargs=(_current, _total,),
                                     processes=processes)
         result = pool.map(_wrapped_func, func_and_args, *args, **kwargs)
 
     if should_print_progress:
         sys.stdout.write('\r')
     return result