Performance hints for nogil functions requiring exception checks #5673

da-woods · 2023-09-01T18:57:24Z

I've created a "not warning" category of message to alert users to performance improvements. It won't cause -Werror to fail and it can be disabled.

Based on #5324 (comment) it would be good if it could appear in annotated html files. However, that's

beyond me right now (but not excluded in future)
probably not hugely useful to most users with existing code bases since they won't be looking at annotated files

I'm not sure this is the perfect solution, but I think some kind of diagnostic is necessary to help people with this.

scoder

Interesting. Does this also solve the problem that we issue warnings from within utility code?

scoder · 2023-09-01T19:17:12Z

Cython/Compiler/Symtab.py

+        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].is_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)
+            ):
+            msg = (
+                "Exception check on '%s' will always require the GIL to be acquired. Possible solutions:"
+                "\n\t1. Declare the function as 'noexcept' if you're sure the function should not raise exceptions."
+             ) % name
+            if type.return_type.is_void:
+                msg += "\n\t2. Return a dummy integer for Cython to use as an exception flag."
+            performance_hint(pos, msg)


Given that you managed to write more or less the same message twice in two entirely different wordings but with just a few additional conditions in the second incarnation, this loudly screams for a shared helper function.

Also, "requires to be reacquired" sounds funny. Maybe there's a different word that you could use for one or the other.

scoder · 2023-09-01T19:20:50Z

Cython/Compiler/Symtab.py

@@ -931,6 +931,19 @@ 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].is_utility_code and


in_utility_code would read a bit better here, I think. Just a suggestion.

runtests.py

da-woods · 2023-09-01T19:33:16Z

Does this also solve the problem that we issue warnings from within utility code?

Not in this PR. But it could probably be used as a mechanism to do so.

Co-authored-by: scoder <stefan_ml@behnel.de>

scoder · 2023-09-03T06:28:50Z

Cython/Compiler/Errors.py

@@ -284,3 +298,14 @@ def reset():

 def get_errors_count():
    return threadlocal.cython_errors_count
+
+def noexcept_performance_hint_helper(pos, function_name = None, void_return = False):


"xyz_helper" is rarely a good name for a function since every function helps with something. That doesn't tell much about what it actually does. Actual verbs like write_ or issue_ seem better.

Suggested change

def noexcept_performance_hint_helper(pos, function_name = None, void_return = False):

def write_noexcept_performance_hint(pos, function_name=None, void_return=False):

This seems too specific for Errors.py. You probably considered a couple of places before you put it here, and I agree that it's not easy to decide. But maybe somewhere in PyrexTypes.py? The hint that we give seems mostly related to C function types. I would consider adding a new module for this, but I'm not sure that we will have many cases where we issue the same hint from more than one place. We'll see about that…

I created a new module (and also moved one function in ExprNodes that seemed to fit in the same place).

If you think that's excessive I can put it in PyrexTypes though

Changed my mind because it caused a circular import and just moved it to PyrexTypes

This reverts commit 36f61fe.

Cython/Compiler/PyrexTypes.py

scoder · 2023-09-04T07:04:31Z

Cython/Compiler/Symtab.py

+        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)


Does this also exclude inline functions in .pxd files?

Why is is_pxd unreliable?

Does this also exclude inline functions in .pxd files?

Yes - intentionally (I'm working on the basis that anything in a pxd is likely something the user doesn't control, and that they get a message when they use it anyway)

Why is is_pxd unreliable?

Not sure off the top of my head, but it didn't seem to be set in the example that I tested. I'll go back and see if I can fix that though.

(Will also come back to the other two comments later)

It looks like in_pxd means that Cython generates code to cimport the function by declaring function pointers (which conflicts with inline). I've had a go at trying to fix that so it's probably a bit better.

runtests.py

scoder · 2023-09-04T07:13:59Z

runtests.py

+    def getperformancehints(self):
+        return self._collect()[2]
+


Is this used anywhere? Do we need to add it? (And, BTW, are the other two corresponding methods really used anywhere?)

scoder · 2023-09-04T07:32:47Z

runtests.py

-                 expect_errors=False, expect_warnings=False, annotate=False, cleanup_workdir=True,
+                 expect_errors=False, expect_warnings=False, expect_perf_hints=False,


I wonder if it's time to clean this up and pass a single expect_log=('warnings', 'errors') option around (name is up for debate…). We're really carrying around a lot of options by now, and I feel bad every time I add one to the list.

Doesn't have to be in this PR, though. Adding this one doesn't make it much worse.

Co-authored-by: scoder <stefan_ml@behnel.de>

da-woods added 2 commits August 14, 2023 12:14

Add "performance_hint" for non-warning performance hints

2a52e35

Test for performance hints

2bddaa9

scoder reviewed Sep 1, 2023

View reviewed changes

runtests.py Outdated Show resolved Hide resolved

da-woods and others added 4 commits September 2, 2023 09:12

Update runtests.py

0cec6eb

Co-authored-by: scoder <stefan_ml@behnel.de>

Deduplication suggested by review

b59807d

Spelling

66f8cb3

Code style

857844a

scoder reviewed Sep 3, 2023

View reviewed changes

da-woods added 4 commits September 3, 2023 11:00

Move messages to a separate file

36f61fe

Revert "Move messages to a separate file"

197f44f

This reverts commit 36f61fe.

Rename function and move to pyrextypes

4c73d61

Merge branch 'master' into performance_hints

3e0da5f

scoder reviewed Sep 4, 2023

View reviewed changes

da-woods and others added 4 commits September 4, 2023 08:49

Apply suggestions from code review

217e0c7

Co-authored-by: scoder <stefan_ml@behnel.de>

Apply suggestions from code review

daf413d

Co-authored-by: scoder <stefan_ml@behnel.de>

More review changes

c3910e9

Inline not in pxd

3390c1a

matusvalo added this to the 3.0.3 milestone Sep 10, 2023

scoder added feature Error Reporting labels Sep 29, 2023

scoder approved these changes Sep 29, 2023

View reviewed changes

scoder merged commit 401e880 into cython:master Sep 29, 2023
76 checks passed

scoder mentioned this pull request Oct 6, 2023

[BUG] SciPy failure to build with Cython 3.0.3 (bisected) #5748

Closed

isc-adang mentioned this pull request Oct 10, 2023

[autoupdate] Update Cython from 3.0.2 to 3.0.3 intersystems/iknow#366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance hints for nogil functions requiring exception checks #5673

Performance hints for nogil functions requiring exception checks #5673

da-woods commented Sep 1, 2023

scoder left a comment

scoder Sep 1, 2023

scoder Sep 1, 2023

scoder Sep 1, 2023

da-woods commented Sep 1, 2023

scoder Sep 3, 2023

da-woods Sep 3, 2023

da-woods Sep 3, 2023

scoder Sep 4, 2023

da-woods Sep 4, 2023

da-woods Sep 4, 2023

scoder Sep 4, 2023

scoder Sep 4, 2023

	def noexcept_performance_hint_helper(pos, function_name = None, void_return = False):
	def write_noexcept_performance_hint(pos, function_name=None, void_return=False):

		expect_errors=False, expect_warnings=False, annotate=False, cleanup_workdir=True,
		expect_errors=False, expect_warnings=False, expect_perf_hints=False,

Performance hints for nogil functions requiring exception checks #5673

Performance hints for nogil functions requiring exception checks #5673

Conversation

da-woods commented Sep 1, 2023

scoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-woods commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment