Allow condition in GILStatNode by noamher · Pull Request #2860 · cython/cython

noamher · 2019-02-22T21:47:05Z

Implementation for issue 2579.
Allowing statements such as:

with nogil(numeric is int):
  <some code>

which will either run inside a nogil block if condition is True, or will run without a nogil block if the condition is False.

Original form still works the same (with nogil: ...).

Still missing: errors tests.

scoder · 2019-02-23T07:37:58Z

For error tests, look at the ones in tests/errors/. They use # mode: error and list their compiler error messages in a string constant (which is actually parsed as text lines and not evaluated). Just write broken code in there, let Cython report the error messages, and copy them into the file to "fix" the test.

scoder

Nice! Yes, error tests are missing. Those are the right way to test the actual nogil behaviour, because doing Python stuff in a nogil block should raise a compile error.

scoder · 2019-02-23T07:25:53Z

Cython/Compiler/Nodes.py

    #
    #   state   string   'gil' or 'nogil'

+    child_attrs = ["body", "condition", "finally_clause", "finally_except_clause"]


I'd spell this
child_attrs = ["condition"] + NogilTryFinallyStatNode.child_attrs
Just to avoid accidental inconsistencies in the future.

scoder · 2019-02-23T07:26:57Z

Cython/Compiler/Optimize.py

+                node.condition = None
+
+            # Condition is False - the body of the GILStatNode
+            # should run without changing changing the state of the gil


typo (changing changing)

Typo fixed.

scoder · 2019-02-23T07:29:22Z

Cython/Compiler/ParseTreeTransforms.py

    def visit_GILStatNode(self, node):
+        if node.condition is not None:
+            error(node.pos, "Non-constant condition in a "
+                            "`with %s(<condition>)` statement" % node.state)


Nice. You could also use node.condition.pos instead of node.pos here, which then points directly to the non-constant condition.

Changed to node.condtion.pos.

scoder · 2019-02-23T07:31:27Z

tests/run/nogil_conditional.pyx

+    with nogil(True):
+        x = f_nogil(x)
+        with gil(True):
+            x = f_gil(x)


Note that this is not a function that needs the GIL. It's one that acquires the GIL itself, on entry. Just use a normal Python function instead.

changed f_gil to normal Python function.

scoder · 2019-02-23T07:39:47Z

tests/run/nogil_conditional.pyx

+    return y
+
+
+cdef int with_gil_func() except 0 with gil:


except 0 is a bit dangerous since 0 is also the default success value to return from a cdef int function when there is no explicit return x.

changed to -1.
Note that in tests/run/nogil.pyx it is still 0.

Ah, thanks. I fixed these tests and also added a compiler warning for this case.

scoder · 2019-02-23T07:49:38Z

tests/run/nogil_conditional.pyx

+    """
+    cdef int res = 0
+
+    if fused_with_object is object:


I think this condition makes the test a bit boring, because it discards all nogil tests in the int case without even looking at them, so the two alternative cases are not actually tested. Maybe you could add a third fused types test that tries to actually use the GIL differently based on the type?

When adding a test for different behavior based on the actual fused type I see that in these cases, the condition (e.g. numeric is float) is not constant during GilCheck which fails the test.

Should I add logic to check for these special kind of expressions (reuse the logic from ReplaceFusedTypeChecks) or should I change some previous transform so that these expressions become constant?

It looks like a simple .visit_GILStatNode() in that transform that works like .visit_IfStatNode() should suffice.
(Actually, I wonder if it wouldn't have been best to make RepFTCh inherit from ConstFold, but that's not something to fix up right now.)

in that transform

You are referring to ReplaceFusedTypeChecks, right?

Added it to ReplaceFusedTypeChecks, worked like a charm.

scoder · 2019-02-23T08:08:23Z

BTW, there is apparently a Py2 distutils problem in the test runner that I just fixed in master. If you merge that back (or rebase your branch), then the tests should be ok.
Edit: Fixed a second time in 605cc21, sorry.

noamher · 2019-02-23T09:20:06Z

@scoder rebased on latest master.

noamher · 2019-02-23T18:37:59Z

@scoder
I'm writing the errors tests.
One of the tests involves calling a function in the condition (e.g. with nogil(foo(x)): ...) so that the condition is not constant.

I get the following exception:

Traceback (most recent call last):
  File "/home/noam/dev/cython/Cython/Compiler/Visitor.py", line 180, in _visit
    return handler_method(obj)
  File "/home/noam/dev/cython/Cython/Compiler/Visitor.py", line 514, in visit_SimpleCallNode
    if function.type.is_pyobject:
AttributeError: 'NoneType' object has no attribute 'is_pyobject'

It seems that during the CythonTransform, the condition of the GilStatNode which is a SimpleCallNode is visited. For some reason the type of the function is missing.

I am guessing that cython expects the condition of the GilStatNode to already be without a gil so that a functioned called in this scope should be a cdef / cpdef thus having a type.

I defined foo as a normal python function without a return type.

Does my analysis make sense?
What could be a solution?

(note that we still want the compiler to raise an error in this case, but a different error - Non-constant condition for GilStatNode error)

noamher · 2019-02-23T18:43:32Z

I believe the same thing happens with with nogil(x < 10): ... but in SwitchTransform with a PrimaryCmpNode.

Traceback (most recent call last):
  File "/home/noam/dev/cython/Cython/Compiler/Visitor.py", line 180, in _visit
    return handler_method(obj)
  File "/home/noam/dev/cython/Cython/Compiler/Optimize.py", line 1262, in visit_PrimaryCmpNode
    None, node, True)
  File "/home/noam/dev/cython/Cython/Compiler/Optimize.py", line 1154, in extract_common_conditions
    not_in, var, conditions = self.extract_conditions(condition, allow_not_in)
  File "/home/noam/dev/cython/Cython/Compiler/Optimize.py", line 1104, in extract_conditions
    elif not cond.is_python_comparison():
  File "/home/noam/dev/cython/Cython/Compiler/ExprNodes.py", line 12342, in is_python_comparison
    and (self.has_python_operands()
  File "/home/noam/dev/cython/Cython/Compiler/ExprNodes.py", line 12697, in has_python_operands
    return (self.operand1.type.is_pyobject
AttributeError: 'NoneType' object has no attribute 'is_pyobject'

scoder · 2019-02-23T18:59:05Z

Ah, right. You have to call self.condition.analyse_declarations(env) in GILStatNode.analyse_declarations(), and self.condition = self.condition.analyse_expressions(env) in GILStatNode.analyse_expressions(), before the base class upcall (I'd say). Those two methods are needed by the respective declarations/expression analysis transforms and use explicit tree traversal since they often need to do special things with their child nodes.

scoder · 2019-02-23T18:59:30Z

See? Good to have error tests. :)

noamher · 2019-02-23T19:48:53Z

Where exactly should self.condition = self.condition.analyse_expressions(env) go?

Because we probably want it to be evaluated before the GilStatNode changes the state of the GIL.

Is this okay?

    def analyse_expressions(self, env):
        env.use_utility_code(
            UtilityCode.load_cached("ForceInitThreads", "ModuleSetupCode.c"))
        self.condition = self.condition.analyse_expressions(env)  # <<<<<<<
        was_nogil = env.nogil
        env.nogil = self.state == 'nogil'
        node = TryFinallyStatNode.analyse_expressions(self, env)
        env.nogil = was_nogil
        return node

scoder · 2019-02-23T19:54:08Z

Yes, exactly. It's the first child of the node that gets evaluated, before the nodes changes the GIL state. (That's also a reason for it to appear first in child_attrs, which is traversed in order.)

scoder · 2019-02-23T20:08:07Z

I've added some documentation about the AST to the HackerGuide in the wiki:
https://github.com/cython/cython/wiki/HackerGuide#the-parse-tree-ast
Updates, comments etc. welcome (although not in this ticket :) ).

noamher · 2019-02-23T20:33:17Z

Documentation looks good, very clear.

What about documentation for Cython users regarding the conditional with gil/nogil statement?

Should I do that as part of this PR or will it be done for all the features of Cython 3.0.0 before it is released?

scoder · 2019-02-23T20:43:06Z

I'm yet to see a case where documentation magically appears out of thin air when we announce a release date. ;o)
It would be great if you could add a feature description to the nogil docs.

noamher · 2019-02-23T22:04:51Z

Should examples involving fused types be documented in the nogil docs or in the fused types docs?

scoder · 2019-02-24T06:43:56Z

Excellent question, the fused types docs. Mentioning them elsewhere would just be confusing (pun half-intended).

scoder · 2019-02-24T07:05:31Z

The error tests look good, BTW. I think this is ready to be merged when the docs are done.

noamher · 2019-02-24T07:17:19Z

Thx.

Do you think I should add a test in which the condition is a compile-time definition?

scoder · 2019-02-24T08:42:17Z

Shouldn't make a difference, but well, it's a use case, so why not have it covered. Feel free to swap some of the True/False constants with a FREE_GIL constant or so. Shouldn't be more than that.

noamher · 2019-02-25T19:18:18Z

@scoder how can I compile the docs into html (or something else) to check that my ref annotations work?

noamher · 2019-02-26T08:36:58Z

@scoder waiting for your input. I think everything should be covered now.

scoder · 2019-02-26T18:35:51Z

how can I compile the docs into html

If you have Sphinx installed, you can run make -C docs html.

scoder · 2019-02-26T18:41:17Z

Very nice feature, thanks!

scoder reviewed Feb 23, 2019

View reviewed changes

scoder added this to the 3.0 milestone Feb 23, 2019

scoder added feature Cython Language Feature labels Feb 23, 2019

Allow condition in GILStatNode

5ff75cd

noamher force-pushed the conditional-gilstatnode branch from 6a45480 to 9afd1fd Compare February 23, 2019 09:19

noamher added 2 commits February 23, 2019 22:13

Create run tests for conditional GILStatNode

73133f6

Create errors tests for conditional GILStatNode

412b232

noamher force-pushed the conditional-gilstatnode branch from 9afd1fd to 412b232 Compare February 23, 2019 20:14

Test conditional GILStatNode with compile-time constant as condition

f8e58c5

noamher force-pushed the conditional-gilstatnode branch 2 times, most recently from 220687c to 9521466 Compare February 25, 2019 19:15

Add conditional GILStatNode usage to documentation

5dc832d

noamher force-pushed the conditional-gilstatnode branch from 9521466 to 5dc832d Compare February 25, 2019 19:16

scoder merged commit 1112a9c into cython:master Feb 26, 2019

Uh oh!

Conversation

noamher commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scoder commented Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noamher commented Feb 23, 2019

Uh oh!

noamher commented Feb 23, 2019

Uh oh!

noamher commented Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

noamher commented Feb 23, 2019

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

noamher commented Feb 23, 2019

Uh oh!

scoder commented Feb 23, 2019

Uh oh!

noamher commented Feb 23, 2019

Uh oh!

scoder commented Feb 24, 2019

Uh oh!

scoder commented Feb 24, 2019

Uh oh!

noamher commented Feb 24, 2019

Uh oh!

scoder commented Feb 24, 2019

Uh oh!

noamher commented Feb 25, 2019

Uh oh!

noamher commented Feb 26, 2019

Uh oh!

noamher commented Feb 22, 2019 •

edited

Loading

scoder commented Feb 23, 2019 •

edited

Loading

noamher commented Feb 23, 2019 •

edited

Loading