opt-viewer parallelized

[bcain]

Summary

Attempt to put opt-viewer critical items in parallel

Requires features from Python 2.7

Cruft left behind: timing printouts

TBD: Not identical output with reference, doesn't look like obvious bug but bears study before results can be satisfactory.

Performance
Below are performance results across various configurations. These were taken on an i5-5200U (dual core + HT). They were taken with a small subset of the YAML output of building Python 3.6.0b3 with LTO+PGO. 60 YAML files.

"multiprocessing" is the current submission contents. "baseline" is as of 544f14c6b2a07a94168df31833dba9dc35fd8289 (I think this is aka r287505).

"ImportError" vs "class<...CLoader>" below are just confirming the expected configuration (with/without CLoader).

**************************************** MULTIPROCESSING ****************************************
PyYAML:
	Traceback (most recent call last):
	  File "<string>", line 1, in <module>
	ImportError: cannot import name CLoader
	Python 2.7.12
processed input files 55.1229741573 secs
mapped remarks 0.0160808563232 secs
file rendering 1.31177091599 sec
index rendering 0.514272928238 sec
total time 56.9831430912 secs
160.52user 1.44system 0:57.39elapsed 282%CPU (0avgtext+0avgdata 157940maxresident)k
0inputs+7880outputs (0major+204566minor)pagefaults 0swaps

PyYAML+libYAML:
	<class 'yaml.cyaml.CLoader'>
	Python 2.7.12
processed input files 7.43243384361 secs
mapped remarks 0.0163559913635 secs
file rendering 1.27356004715 sec
index rendering 0.503628969193 sec
total time 9.24395084381 secs
21.52user 1.39system 0:09.66elapsed 237%CPU (0avgtext+0avgdata 158652maxresident)k
0inputs+7880outputs (0major+207754minor)pagefaults 0swaps

PyPy/PyYAML:
	Traceback (most recent call last):
	  File "<module>", line 1, in <module>
	ImportError: cannot import name 'CLoader'
	Python 2.7.12 (aff251e54385, Nov 09 2016, 18:02:49)
	[PyPy 5.6.0 with GCC 4.8.2]
processed input files 17.7338540554 secs
mapped remarks 0.0163650512695 secs
file rendering 2.23697400093 sec
index rendering 0.571249961853 sec
total time 20.5806679726 secs
57.87user 1.95system 0:21.16elapsed 282%CPU (0avgtext+0avgdata 282260maxresident)k
0inputs+7328outputs (0major+286354minor)pagefaults 0swaps
**************************************** BASELINE        ****************************************
PyYAML:
	Traceback (most recent call last):
	  File "<string>", line 1, in <module>
	ImportError: cannot import name CLoader
	Python 2.7.12
94.07user 0.86system 1:35.82elapsed 99%CPU (0avgtext+0avgdata 112204maxresident)k
0inputs+7880outputs (0major+30906minor)pagefaults 0swaps

PyYAML+libYAML:
	<class 'yaml.cyaml.CLoader'>
	Python 2.7.12
11.52user 0.64system 0:12.57elapsed 96%CPU (0avgtext+0avgdata 109588maxresident)k
0inputs+7880outputs (0major+29965minor)pagefaults 0swaps

PyPy/PyYAML:
	Traceback (most recent call last):
	  File "<module>", line 1, in <module>
	ImportError: cannot import name 'CLoader'
	Python 2.7.12 (aff251e54385, Nov 09 2016, 18:02:49)
	[PyPy 5.6.0 with GCC 4.8.2]
18.86user 0.97system 0:20.52elapsed 96%CPU (0avgtext+0avgdata 209160maxresident)k
0inputs+7880outputs (0major+48344minor)pagefaults 0swaps

[bcain]

Other notes:

• IIRC llvm depends on Python 2.7 so leveraging features from there shouldn't be a big deal.
• In the multiprocessing case we probably have a concurrency bug with the c++filt file descriptor, so we'd either need a multiprocessing.Lock() block in demangle() or have each process spawn their own.
• htmlformatter encoding was necessary to correctly handle input
• os.path.exists()/os.mkdir() was a TOCTOU bug.
• processes should probably have a heuristic derived from info like nproc

[bcain]

\

utils/opt-viewer/opt-viewer.py

327

Now that I take a look at it this logic might not be right to cascade that merged defaultdict[defaultdict[list]], I will investigate.

[bcain]

Fixed file remarks dictionary merge.

[bcain]

Fixed dict merge implementation.

[anemet]

In D26789#598293, @bcain wrote:

Other notes:

• IIRC llvm depends on Python 2.7 so leveraging features from there shouldn't be a big deal.
• In the multiprocessing case we probably have a concurrency bug with the c++filt file descriptor, so we'd either need a multiprocessing.Lock() block in demangle() or have each process spawn their own.

I don't think so. Demangle is not used during parsing.

• htmlformatter encoding was necessary to correctly handle input

Is there a way to detect if the input is UTF-8? This can probably go in irrespective of this review.

[fhahn]

Added a few Python specific comments.

utils/opt-viewer/opt-viewer.py

22

I'd move that right before import os.path.

31

I think PEP-8 suggests to group standard library imports together.

288

Is there a reason for having the imports here and not having them with the other imports?

337

It looks like hotnesses isn't used except for calculating the max_hotness. If that's the case I think you could use a generator expression instead of the list comprehension, which creates a new list object in memory.

[anemet]

In D26789#598698, @anemet wrote:

In D26789#598293, @bcain wrote:

• In the multiprocessing case we probably have a concurrency bug with the c++filt file descriptor, so we'd either need a multiprocessing.Lock() block in demangle() or have each process spawn their own.

I don't think so. Demangle is not used during parsing.

Sorry, you're right, I missed the fact that you also parallelized file rendering.

[bcain]

Per discussion on the list I will clean this up and make a new review.

Florian, I'll make the suggested changes.

[bcain]

Rebased for submission given review feedback
omitted timing printouts
deleted multithreading code (strictly multiprocessing now)

[bcain]

The 'hotness floor' content was commented out but should've been entirely omitted.

utils/opt-viewer/opt-viewer.py

386

whoops, I'd meant to exclude this from the submission. I'll purge it and consider it for a future submission.

[bcain]

omitted "hotness floor" content

[bcain]

Below are performance results across various configurations. These were taken on an i5-5200U (dual core + HT). They were taken with a small subset of the YAML output of building Python 3.6.0b3 with LTO+PGO. 60 YAML files.

"multiprocessing" is the current submission contents. "baseline" is as of 544f14c6b2a07a94168df31833dba9dc35fd8289 (I think this is aka r287505).

"ImportError" vs "class<...CLoader>" below are just confirming the expected configuration (with/without CLoader).

**************************************** MULTIPROCESSING ****************************************
PyYAML:
	Traceback (most recent call last):
	  File "<string>", line 1, in <module>
	ImportError: cannot import name CLoader
	Python 2.7.12
processed input files 55.1229741573 secs
mapped remarks 0.0160808563232 secs
file rendering 1.31177091599 sec
index rendering 0.514272928238 sec
total time 56.9831430912 secs
160.52user 1.44system 0:57.39elapsed 282%CPU (0avgtext+0avgdata 157940maxresident)k
0inputs+7880outputs (0major+204566minor)pagefaults 0swaps

PyYAML+libYAML:
	<class 'yaml.cyaml.CLoader'>
	Python 2.7.12
processed input files 7.43243384361 secs
mapped remarks 0.0163559913635 secs
file rendering 1.27356004715 sec
index rendering 0.503628969193 sec
total time 9.24395084381 secs
21.52user 1.39system 0:09.66elapsed 237%CPU (0avgtext+0avgdata 158652maxresident)k
0inputs+7880outputs (0major+207754minor)pagefaults 0swaps

PyPy/PyYAML:
	Traceback (most recent call last):
	  File "<module>", line 1, in <module>
	ImportError: cannot import name 'CLoader'
	Python 2.7.12 (aff251e54385, Nov 09 2016, 18:02:49)
	[PyPy 5.6.0 with GCC 4.8.2]
processed input files 17.7338540554 secs
mapped remarks 0.0163650512695 secs
file rendering 2.23697400093 sec
index rendering 0.571249961853 sec
total time 20.5806679726 secs
57.87user 1.95system 0:21.16elapsed 282%CPU (0avgtext+0avgdata 282260maxresident)k
0inputs+7328outputs (0major+286354minor)pagefaults 0swaps
**************************************** BASELINE        ****************************************
PyYAML:
	Traceback (most recent call last):
	  File "<string>", line 1, in <module>
	ImportError: cannot import name CLoader
	Python 2.7.12
94.07user 0.86system 1:35.82elapsed 99%CPU (0avgtext+0avgdata 112204maxresident)k
0inputs+7880outputs (0major+30906minor)pagefaults 0swaps

PyYAML+libYAML:
	<class 'yaml.cyaml.CLoader'>
	Python 2.7.12
11.52user 0.64system 0:12.57elapsed 96%CPU (0avgtext+0avgdata 109588maxresident)k
0inputs+7880outputs (0major+29965minor)pagefaults 0swaps

PyPy/PyYAML:
	Traceback (most recent call last):
	  File "<module>", line 1, in <module>
	ImportError: cannot import name 'CLoader'
	Python 2.7.12 (aff251e54385, Nov 09 2016, 18:02:49)
	[PyPy 5.6.0 with GCC 4.8.2]
18.86user 0.97system 0:20.52elapsed 96%CPU (0avgtext+0avgdata 209160maxresident)k
0inputs+7880outputs (0major+48344minor)pagefaults 0swaps

[bcain]

Corrected per review comments.

[bcain]

Moved 'os' module import per review comments.

[bcain]

Also note that some of the wrapping was changed. Florian suggested I try to maintain PEP8 so I just ran autopep8 and it wrapped some lines. If preferred I can revert these changes in order to simplify the diff.

[anemet]

Brian,

I was probably not completely clear but you can't add llvm-commits to an existing review. I does not appear in llvm-commits properly. That is why I said that you need to create a new one with it as one of the original subscribers.

Thanks,
Adam

[bcain]

obsoleted by D26967

utils/opt-viewer/opt-viewer.py

     from yaml import CLoader as Loader
 except ImportError:
     from yaml import Loader
+
+from multiprocessing import Pool
+from multiprocessing import Lock, cpu_count
+
 import argparse
+import errno
+import os
 import os.path
 import re
 import subprocess

 from pygments import highlight
 from pygments.lexers.c_cpp import CppLexer
 from pygments.formatters import HtmlFormatter
+import functools
+from collections import defaultdict
+import itertools
 
-parser = argparse.ArgumentParser(description=desc)
-parser.add_argument('yaml_files', nargs='+')
-parser.add_argument('output_dir')
-parser.add_argument('-source-dir', '-s', default='', help='set source directory')
-args = parser.parse_args()
 
-p = subprocess.Popen(['c++filt', '-n'], stdin=subprocess.PIPE, stdout=subprocess.PIPE)
+p = subprocess.Popen(
+    ['c++filt', '-n'], stdin=subprocess.PIPE, stdout=subprocess.PIPE)
+p_lock = Lock()
 
 
 def demangle(name):
-    p.stdin.write(name + '\n')
-    return p.stdout.readline().rstrip()
+    with p_lock:
+        p.stdin.write(name + '\n')
+        return p.stdout.readline().rstrip()
 
 
 class Remark(yaml.YAMLObject):

         # missing and no relative hotness information is displayed
         return cls.max_hotness != 0
 
-    # Map function names to their source location for function where inlining happened
+    # Map function names to their source location for function where inlining
+    # happened
     caller_loc = dict()
 
     def __getattr__(self, name):
         # If hotness is missing, assume 0
         if name == 'Hotness':
             return 0
-        raise AttributeError
+
+        raise AttributeError("'Remark' object has no attribute: '{}'".format(name))
 
     @property
     def File(self):

         return "".join(values)
 
     @property
+    def relative_hotness_pct(self):
+        return int(round(self.Hotness * 100 / Remark.max_hotness))
+
+    @property
     def RelativeHotness(self):
         if Remark.should_display_hotness():
-            return "{}%".format(int(round(self.Hotness * 100 / Remark.max_hotness)))
+            return "{}%".format(self.relative_hotness_pct)
         else:
             return ''
 
     @property
     def key(self):
-        return (self.__class__, self.Pass, self.Name, self.File, self.Line, self.Column, self.message)
+        return (
+            self.__class__,
+            self.Pass,
+            self.Name,
+            self.File,
+            self.Line,
+            self.Column,
+            self.message)
 
 
 class Analysis(Remark):

 
 
 class SourceFileRenderer:
-    def __init__(self, filename):
+    def __init__(self, source_dir, output_dir, filename):
         existing_filename = None
         if os.path.exists(filename):
             existing_filename = filename
         else:
-            fn = os.path.join(args.source_dir, filename)
+            fn = os.path.join(source_dir, filename)
             if os.path.exists(fn):
                 existing_filename = fn
 
-        self.stream = open(os.path.join(args.output_dir, SourceFileRenderer.html_file_name(filename)), 'w')
+        self.stream = open(
+            os.path.join(
+                output_dir,
+                SourceFileRenderer.html_file_name(filename)),
+            'w')
         if existing_filename:
             self.source_stream = open(existing_filename)
         else:

 </html>
             '''.format(filename), file=self.stream)
 
-        self.html_formatter = HtmlFormatter()
+        self.html_formatter = HtmlFormatter(encoding='utf-8')
         self.cpp_lexer = CppLexer()
 
     def render_source_line(self, linenum, line):

         dl = Remark.caller_loc.get(r.Function)
         if dl:
             link = Remark.make_link(dl['File'], dl['Line'] - 2)
-            inlining_context = "<a href={link}>{r.DemangledFunctionName}</a>".format(**locals())
+            inlining_context = "<a href={link}>{r.DemangledFunctionName}</a>".format(
+                **locals())
 
         # Column is the number of characters *including* tabs, keep those and
         # replace everything else with spaces.

 <td>Source</td>
 <td>Inline Context</td>
 </tr>''', file=self.stream)
-        for (linenum, line) in enumerate(self.source_stream.readlines(), start=1):
+        for (
+                linenum,
+                line) in enumerate(
+                self.source_stream.readlines(),
+                start=1):
             self.render_source_line(linenum, line)
             for remark in line_remarks.get(linenum, []):
                 self.render_inline_remarks(remark, line)

 
 
 class IndexRenderer:
-    def __init__(self):
-        self.stream = open(os.path.join(args.output_dir, 'index.html'), 'w')
+
+    def __init__(self, output_dir):
+        self.stream = open(os.path.join(output_dir, 'index.html'), 'w')
 
     def render_entry(self, r):
         print('''

 </html>''', file=self.stream)
 
 
-all_remarks = dict()
-file_remarks = dict()
+def get_remarks(input_file):
+    max_hotness = 0
+    all_remarks = dict()
+    file_remarks = defaultdict(functools.partial(defaultdict, list))
+
+    with open(input_file) as f:
+        docs = yaml.load_all(f, Loader=Loader)
+
+        for remark in docs:
+            # Avoid remarks withoug debug location or if they are duplicated
+            if not hasattr(remark, 'DebugLoc') or remark.key in all_remarks:
+                continue
+            all_remarks[remark.key] = remark
+
+            file_remarks[remark.File][remark.Line].append(remark)
+
+            max_hotness = max(max_hotness, remark.Hotness)
+
+    return max_hotness, all_remarks, file_remarks
+
+
+def _render_file(source_dir, output_dir, entry):
+    try:
+        filename, remarks = entry
+        SourceFileRenderer(source_dir, output_dir, filename).render(remarks)
+    except Exception as e:
+        print('detail', e)
+
 
-for input_file in args.yaml_files:
-    f = open(input_file)
-    docs = yaml.load_all(f, Loader=Loader)
-    for remark in docs:
-        # Avoid remarks withoug debug location or if they are duplicated
-        if not hasattr(remark, 'DebugLoc') or remark.key in all_remarks:
-            continue
-        all_remarks[remark.key] = remark
+def gather_results(filenames):
+    pool = Pool(processes=cpu_count())
+    all_remarks = dict()
+    file_remarks = defaultdict(functools.partial(defaultdict, list))
+    remarks = pool.map(get_remarks, filenames)
 
-        file_remarks.setdefault(remark.File, dict()).setdefault(remark.Line, []).append(remark)
+    def merge_dicts(dicts):
+        merged = defaultdict(functools.partial(defaultdict, list))
 
-        Remark.max_hotness = max(Remark.max_hotness, remark.Hotness)
+        for k, v in itertools.chain(*[d.iteritems() for d in dicts]):
+            for k_, v_ in v.items():
+                merged[k][k_] += v_
 
-# Set up a map between function names and their source location for function where inlining happened
-for remark in all_remarks.itervalues():
-    if type(remark) == Passed and remark.Pass == "inline" and remark.Name == "Inlined":
-        for arg in remark.Args:
-            caller = arg.get('Caller')
-            if caller:
+        return merged
+
+    file_remark_dicts = [entry[2] for entry in remarks]
+    file_remarks = merge_dicts(file_remark_dicts)
+
+    # merge results:
+    for hotness, all_rem, file_rem in remarks:
+        all_remarks.update(all_rem)
+
+    Remark.max_hotness = max(entry[0] for entry in remarks)
+
+    return all_remarks, file_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 all_remarks.itervalues():
+        if isinstance(remark, Passed) and remark.Pass == "inline" and remark.Name == "Inlined":
+            for arg in remark.Args:
+                caller = arg.get('Caller')
+                if caller:
                     Remark.caller_loc[caller] = arg['DebugLoc']
 
-if Remark.should_display_hotness():
-    sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: r.Hotness, reverse=True)
-else:
-    sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: (r.File, r.Line, r.Column))
 
-if not os.path.exists(args.output_dir):
-    os.mkdir(args.output_dir)
+def generate_report(all_remarks, output_dir):
+    if Remark.should_display_hotness():
+        sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: r.Hotness,
+            reverse=True)
+    else:
+        sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: (
+                r.File, r.Line, r.Column))
+
+    try:
+        os.makedirs(output_dir)
+    except OSError as e:
+        if e.errno == errno.EEXIST and os.path.isdir(output_dir):
+            pass
+        else:
+            raise
+
+    pool = Pool(processes=cpu_count())
+    _render_file_bound = functools.partial(_render_file, args.source_dir, args.output_dir)
+    pool.map(_render_file_bound, file_remarks.items())
+
+    IndexRenderer(args.output_dir).render(sorted_remarks)
+
+    shutil.copy(
+        os.path.join(
+            os.path.dirname(
+                os.path.realpath(__file__)),
+            "style.css"),
+        output_dir)
+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser(description=desc)
+    parser.add_argument('yaml_files', nargs='+')
+    parser.add_argument('output_dir')
+    parser.add_argument(
+        '-source-dir',
+        '-s',
+        default='',
+        help='set source directory')
+    args = parser.parse_args()
+
+    if len(args.yaml_files) == 0:
+        parser.print_help()
+        sys.exit(1)
 
-for (filename, remarks) in file_remarks.iteritems():
-    SourceFileRenderer(filename).render(remarks)
+    all_remarks, file_remarks = gather_results(args.yaml_files)
 
-IndexRenderer().render(sorted_remarks)
+    map_remarks(all_remarks)
 
-shutil.copy(os.path.join(os.path.dirname(os.path.realpath(__file__)), "style.css"), args.output_dir)
+    generate_report(all_remarks, args.output_dir)