Performance hints for nogil functions requiring exception checks

Cython/Compiler/Errors.py

@@ -191,6 +191,20 @@ def _write_file_encode(file, line):
         file.write(line.encode('ascii', 'replace'))
 
 
+def performance_hint(position, message):
+    if not Options.show_performance_hints:
+        return
+    warn = CompileWarning(position, message)
+    line = "performance hint: %s\n" % warn
+    listing_file = threadlocal.cython_errors_listing_file
+    if listing_file:
+        _write_file_encode(listing_file, line)
+    echo_file = threadlocal.cython_errors_echo_file
+    if echo_file:
+        _write_file_encode(echo_file, line)
+    return warn
+
+
 def message(position, message, level=1):
     if level < LEVEL:
         return

Cython/Compiler/ExprNodes.py

@@ -25,7 +25,7 @@
 
 from .Errors import (
     error, warning, InternalError, CompileError, report_error, local_errors,
-    CannotSpecialize)
+    CannotSpecialize, performance_hint)
 from .Code import UtilityCode, TempitaUtilityCode
 from . import StringEncoding
 from . import Naming
@@ -4944,7 +4944,7 @@ def analyse_types(self, env, getting=True):
 
             elif index.type.is_int or index.type.is_pyobject:
                 if index.type.is_pyobject and not self.warned_untyped_idx:
-                    warning(index.pos, "Index should be typed for more efficient access", level=2)
+                    performance_hint(index.pos, "Index should be typed for more efficient access")
                     MemoryViewIndexNode.warned_untyped_idx = True
 
                 self.is_memview_index = True
@@ -6468,6 +6468,9 @@ def generate_result_code(self, code):
                     exc_checks.append("%s == %s" % (self.result(), func_type.return_type.cast_code(exc_val)))
                 if exc_check:
                     if nogil:
+                        if not exc_checks:
+                            PyrexTypes.write_noexcept_performance_hint(
+                                self.pos, function_name=None, void_return=self.type.is_void)
                         code.globalstate.use_utility_code(
                             UtilityCode.load_cached("ErrOccurredWithGIL", "Exceptions.c"))
                         exc_checks.append("__Pyx_ErrOccurredWithGIL()")

Cython/Compiler/Options.py

@@ -148,6 +148,10 @@ def __repr__(self):
 #: Number of function closure instances to keep in a freelist (0: no freelists)
 closure_freelist_size = 8
 
+#: Show performance hints -- these aren't warning but some people may not want
+# to see them
+show_performance_hints = True
+
 
 def get_directive_defaults():
     # To add an item to this list, all accesses should be changed to use the new

Cython/Compiler/PyrexTypes.py

@@ -20,7 +20,7 @@
 from . import StringEncoding
 from . import Naming
 
-from .Errors import error, CannotSpecialize
+from .Errors import error, CannotSpecialize, performance_hint
 
 
 class BaseType(object):
@@ -5424,6 +5424,17 @@ def cap_length(s, max_prefix=63, max_len=1024):
     hash_prefix = hashlib.sha256(s.encode('ascii')).hexdigest()[:6]
     return '%s__%s__etc' % (hash_prefix, s[:max_len-17])
 
+def write_noexcept_performance_hint(pos, function_name = None, void_return = False):
+    on_what = "on '%s' " % function_name if function_name else ""
+    msg = (
+        "Exception check %swill always require the GIL to be acquired. Possible solutions:\n"
+        "\t1. Declare the function as 'noexcept' if you control the definition and "
+                                "you're sure you don't want the function to raise exceptions.\n"
+    ) % on_what
+    if void_return:
+        msg += "\t2. Use an 'int' return type on the function to allow an error code to be returned."
+    performance_hint(pos, msg)
+
 def remove_cv_ref(tp, remove_fakeref=False):
     # named by analogy with c++ std::remove_cv_ref
     last_tp = None

Cython/Compiler/Scanning.py

@@ -131,6 +131,7 @@ class SourceDescriptor(object):
     A SourceDescriptor should be considered immutable.
     """
     filename = None
+    in_utility_code = False
 
     _file_type = 'pyx'
 

Cython/Compiler/Symtab.py

@@ -931,6 +931,14 @@ def declare_cfunction(self, name, type, pos,
             var_entry.scope = entry.scope
             entry.as_variable = var_entry
         type.entry = entry
+        pos_filetype = pos[0]._file_type if pos else None
+        if (type.exception_check and type.exception_value is None and type.nogil and
+                not pos[0].in_utility_code and
+                defining and  # don't warn about external functions here - the user likely can't do anything
+                pos_filetype != 'pxd'  # again, exclude extern functions (is_pxd is unreliable here)
+                ):
+            PyrexTypes.write_noexcept_performance_hint(
+                pos, function_name=name, void_return=type.return_type.is_void)
         return entry
 
     def declare_cgetter(self, name, return_type, pos=None, cname=None,

Cython/Compiler/TreeFragment.py

@@ -39,14 +39,18 @@ def find_module(self, module_name, from_module=None, pos=None, need_pxd=1, absol
 
 
 def parse_from_strings(name, code, pxds=None, level=None, initial_pos=None,
-                       context=None, allow_struct_enum_decorator=False):
+                       context=None, allow_struct_enum_decorator=False,
+                       in_utility_code=False):
     """
     Utility method to parse a (unicode) string of code. This is mostly
     used for internal Cython compiler purposes (creating code snippets
     that transforms should emit, as well as unit testing).
 
     code - a unicode string containing Cython (module-level) code
     name - a descriptive name for the code source (to use in error messages etc.)
+    in_utility_code - used to suppress some messages from utility code. False by default
+                      because some generated code snippets like properties and dataclasses
+                      probably want to see those messages.
 
     RETURNS
 
@@ -66,6 +70,8 @@ def parse_from_strings(name, code, pxds=None, level=None, initial_pos=None,
     if initial_pos is None:
         initial_pos = (name, 1, 0)
     code_source = StringSourceDescriptor(name, code)
+    if in_utility_code:
+        code_source.in_utility_code = True
 
     scope = context.find_module(module_name, pos=initial_pos, need_pxd=False)
 

Cython/Compiler/UtilityCode.py

@@ -124,7 +124,8 @@ def get_tree(self, entries_only=False, cython_scope=None):
         context.cython_scope = cython_scope
         #context = StringParseContext(self.name)
         tree = parse_from_strings(
-            self.name, self.impl, context=context, allow_struct_enum_decorator=True)
+            self.name, self.impl, context=context, allow_struct_enum_decorator=True,
+            in_utility_code=True)
         pipeline = Pipeline.create_pipeline(context, 'pyx', exclude_classes=excludes)
 
         if entries_only:

runtests.py

@@ -633,7 +633,8 @@ def build_extension(self, ext):
 
 
 class ErrorWriter(object):
-    match_error = re.compile(r'(warning:)?(?:.*:)?\s*([-0-9]+)\s*:\s*([-0-9]+)\s*:\s*(.*)').match
+    match_error = re.compile(
+        r'(warning:|performance hint:)?(?:.*:)?\s*([-0-9]+)\s*:\s*([-0-9]+)\s*:\s*(.*)').match
 
     def __init__(self, encoding=None):
         self.output = []
@@ -646,21 +647,28 @@ def write(self, value):
 
     def _collect(self):
         s = ''.join(self.output)
-        results = {'errors': [], 'warnings': []}
+        results = {'error': [], 'warning': [], 'performance hint': []}
         for line in s.splitlines():
             match = self.match_error(line)
             if match:
-                is_warning, line, column, message = match.groups()
-                results['warnings' if is_warning else 'errors'].append((int(line), int(column), message.strip()))
+                message_type, line, column, message = match.groups()
+                if not message_type:
+                    message_type = 'error'
+                else:
+                    message_type = message_type[:-1]  # drop final :
+                results[message_type].append((int(line), int(column), message.strip()))
 
-        return [["%d:%d: %s" % values for values in sorted(results[key])] for key in ('errors', 'warnings')]
+        return [["%d:%d: %s" % values for values in sorted(results[key])] for key in ('error', 'warning', 'performance hint')]
 
     def geterrors(self):
         return self._collect()[0]
 
     def getwarnings(self):
         return self._collect()[1]
 
+    def getperformancehints(self):
+        return self._collect()[2]
+
     def getall(self):
         return self._collect()
 
@@ -850,6 +858,7 @@ def handle_directory(self, path, context):
     def build_tests(self, test_class, path, workdir, module, module_path, expect_errors, tags):
         warning_errors = 'werror' in tags['tag']
         expect_warnings = 'warnings' in tags['tag']
+        expect_perf_hints = 'perf_hints' in tags['tag']
 
         extra_directives_list = [{}]
 
@@ -891,7 +900,8 @@ def build_tests(self, test_class, path, workdir, module, module_path, expect_err
         preparse_list = tags.get('preparse', ['id'])
         tests = [ self.build_test(test_class, path, workdir, module, module_path,
                                   tags, language, language_level,
-                                  expect_errors, expect_warnings, warning_errors, preparse,
+                                  expect_errors, expect_warnings, expect_perf_hints,
+                                  warning_errors, preparse,
                                   pythran_dir if language == "cpp" else None,
                                   add_cython_import=add_cython_import,
                                   extra_directives=extra_directives)
@@ -903,7 +913,7 @@ def build_tests(self, test_class, path, workdir, module, module_path, expect_err
         return tests
 
     def build_test(self, test_class, path, workdir, module, module_path, tags, language, language_level,
-                   expect_errors, expect_warnings, warning_errors, preparse, pythran_dir, add_cython_import,
+                   expect_errors, expect_warnings, expect_perf_hints, warning_errors, preparse, pythran_dir, add_cython_import,
                    extra_directives):
         language_workdir = os.path.join(workdir, language)
         if not os.path.exists(language_workdir):
@@ -920,6 +930,7 @@ def build_test(self, test_class, path, workdir, module, module_path, tags, langu
                           preparse=preparse,
                           expect_errors=expect_errors,
                           expect_warnings=expect_warnings,
+                          expect_perf_hints=expect_perf_hints,
                           annotate=self.annotate,
                           cleanup_workdir=self.cleanup_workdir,
                           cleanup_sharedlibs=self.cleanup_sharedlibs,
@@ -987,7 +998,8 @@ def filter_test_suite(test_suite, selector):
 
 class CythonCompileTestCase(unittest.TestCase):
     def __init__(self, test_directory, workdir, module, module_path, tags, language='c', preparse='id',
-                 expect_errors=False, expect_warnings=False, annotate=False, cleanup_workdir=True,
+                 expect_errors=False, expect_warnings=False, expect_perf_hints=False,
+                 annotate=False, cleanup_workdir=True,
                  cleanup_sharedlibs=True, cleanup_failures=True, cython_only=False, test_selector=None,
                  fork=True, language_level=2, warning_errors=False,
                  test_determinism=False, shard_num=0,
@@ -1005,6 +1017,7 @@ def __init__(self, test_directory, workdir, module, module_path, tags, language=
         self.name = module if self.preparse == "id" else "%s_%s" % (module, preparse)
         self.expect_errors = expect_errors
         self.expect_warnings = expect_warnings
+        self.expect_perf_hints = expect_perf_hints
         self.annotate = annotate
         self.cleanup_workdir = cleanup_workdir
         self.cleanup_sharedlibs = cleanup_sharedlibs
@@ -1142,8 +1155,8 @@ def runTest(self):
     def runCompileTest(self):
         return self.compile(
             self.test_directory, self.module, self.module_path, self.workdir,
-            self.test_directory, self.expect_errors, self.expect_warnings, self.annotate,
-            self.add_cython_import)
+            self.test_directory, self.expect_errors, self.expect_warnings, self.expect_perf_hints,
+            self.annotate, self.add_cython_import)
 
     def find_module_source_file(self, source_file):
         if not os.path.exists(source_file):
@@ -1189,7 +1202,7 @@ def split_source_and_output(self, source_file, workdir, add_cython_import=False)
             encoding = detect_opened_file_encoding(f, default=None)
 
         with io_open(source_file, 'r', encoding='ISO-8859-1') as source_and_output:
-            error_writer = warnings_writer = None
+            error_writer = warnings_writer = perf_hint_writer = None
             out = io_open(os.path.join(workdir, os.path.basename(source_file)),
                           'w', encoding='ISO-8859-1')
             try:
@@ -1200,6 +1213,9 @@ def split_source_and_output(self, source_file, workdir, add_cython_import=False)
                     elif line.startswith(u"_WARNINGS"):
                         out.close()
                         out = warnings_writer = ErrorWriter(encoding=encoding)
+                    elif line.startswith(u"_PERFORMANCE_HINTS"):
+                        out.close()
+                        out = perf_hint_writer = ErrorWriter(encoding=encoding)
                     else:
                         if add_cython_import and line.strip() and not (
                                 line.startswith(u'#') or line.startswith(u"from __future__ import ")):
@@ -1212,7 +1228,8 @@ def split_source_and_output(self, source_file, workdir, add_cython_import=False)
                 out.close()
 
         return (error_writer.geterrors() if error_writer else [],
-                warnings_writer.geterrors() if warnings_writer else [])
+                warnings_writer.geterrors() if warnings_writer else [],
+                perf_hint_writer.geterrors() if perf_hint_writer else [])
 
     def run_cython(self, test_directory, module, module_path, targetdir, incdir, annotate,
                    extra_compile_options=None):
@@ -1381,10 +1398,10 @@ def get_ext_fullpath(ext_name, self=build_extension):
         return get_ext_fullpath(module)
 
     def compile(self, test_directory, module, module_path, workdir, incdir,
-                expect_errors, expect_warnings, annotate, add_cython_import):
-        expected_errors = expected_warnings = errors = warnings = ()
-        if expect_errors or expect_warnings or add_cython_import:
-            expected_errors, expected_warnings = self.split_source_and_output(
+                expect_errors, expect_warnings, expect_perf_hints, annotate, add_cython_import):
+        expected_errors = expected_warnings = expected_perf_hints = errors = warnings = ()
+        if expect_errors or expect_warnings or expect_perf_hints or add_cython_import:
+            expected_errors, expected_warnings, expected_perf_hints = self.split_source_and_output(
                 module_path, workdir, add_cython_import)
             test_directory = workdir
             module_path = os.path.join(workdir, os.path.basename(module_path))
@@ -1395,7 +1412,7 @@ def compile(self, test_directory, module, module_path, workdir, incdir,
                 sys.stderr = ErrorWriter()
                 with self.stats.time(self.name, self.language, 'cython'):
                     self.run_cython(test_directory, module, module_path, workdir, incdir, annotate)
-                errors, warnings = sys.stderr.getall()
+                errors, warnings, perf_hints = sys.stderr.getall()
             finally:
                 sys.stderr = old_stderr
             if self.test_determinism and not expect_errors:
@@ -1420,6 +1437,8 @@ def compile(self, test_directory, module, module_path, workdir, incdir,
         tostderr = sys.__stderr__.write
         if expected_warnings or (expect_warnings and warnings):
             self._match_output(expected_warnings, warnings, tostderr)
+        if expected_perf_hints or (expect_perf_hints and perf_hints):
+            self._match_output(expected_perf_hints, perf_hints, tostderr)
         if 'cerror' in self.tags['tag']:
             if errors:
                 tostderr("\n=== Expected C compile error ===\n")

tests/run/nogil.pxd

@@ -0,0 +1 @@
+cdef void voidexceptnogil_in_pxd() nogil

tests/run/nogil.pyx

@@ -1,4 +1,7 @@
 # mode: run
+# tag: perf_hints
+
+from nogil_other cimport voidexceptnogil_in_other_pxd
 
 try:
     from StringIO import StringIO
@@ -169,7 +172,7 @@ def test_copy_array_exception(n):
     """
     copy_array_exception(n)
 
-def test_copy_array_exception_nogil(n): 
+def test_copy_array_exception_nogil(n):
     """
     >>> test_copy_array_exception_nogil(20)
     Traceback (most recent call last):
@@ -179,3 +182,37 @@ def test_copy_array_exception_nogil(n):
     cdef int cn = n
     with nogil:
         copy_array_exception(cn)
+
+# Should still get a warning even though it's declared in a pxd
+cdef void voidexceptnogil_in_pxd() nogil:
+    pass
+
+def test_performance_hint_nogil():
+    """
+    >>> test_performance_hint_nogil()
+    """
+    with nogil:
+        voidexceptnogil_in_pxd()
+        # The function call should generate a performance hint, but the definition should
+        # not (since it's in an external pxd we don't control)
+        voidexceptnogil_in_other_pxd()
+
+
+# Note that we're only able to check the first line of the performance hint
+_PERFORMANCE_HINTS = """
+20:9: Exception check will always require the GIL to be acquired. Possible solutions:
+24:5: Exception check on 'f' will always require the GIL to be acquired. Possible solutions:
+34:5: Exception check on 'release_gil_in_nogil' will always require the GIL to be acquired. Possible solutions:
+39:6: Exception check on 'release_gil_in_nogil2' will always require the GIL to be acquired. Possible solutions:
+49:28: Exception check will always require the GIL to be acquired. Possible solutions:
+51:29: Exception check will always require the GIL to be acquired. Possible solutions:
+55:5: Exception check on 'get_gil_in_nogil' will always require the GIL to be acquired. Possible solutions:
+59:6: Exception check on 'get_gil_in_nogil2' will always require the GIL to be acquired. Possible solutions:
+68:24: Exception check will always require the GIL to be acquired. Possible solutions:
+70:25: Exception check will always require the GIL to be acquired. Possible solutions:
+133:5: Exception check on 'copy_array_exception' will always require the GIL to be acquired. Possible solutions:
+184:28: Exception check will always require the GIL to be acquired. Possible solutions:
+187:5: Exception check on 'voidexceptnogil_in_pxd' will always require the GIL to be acquired. Possible solutions:
+195:30: Exception check will always require the GIL to be acquired. Possible solutions:
+198:36: Exception check will always require the GIL to be acquired. Possible solutions:
+"""

tests/run/nogil_other.pxd

@@ -0,0 +1,2 @@
+cdef inline void voidexceptnogil_in_other_pxd() nogil:
+    pass