Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Getting "performance hint: ..." #6001

Closed
YoSTEALTH opened this issue Feb 15, 2024 · 9 comments · Fixed by #6088
Closed

[BUG] Getting "performance hint: ..." #6001

YoSTEALTH opened this issue Feb 15, 2024 · 9 comments · Fixed by #6088

Comments

@YoSTEALTH
Copy link
Contributor

Describe the bug

I keep getting these "performance hint" bellow.

Goal is to run io_uring_queue_init with nogil, since nogil required other functions to be nogil as well I have set trap_error to be nogil as well and only trigger error when return value is < 0, why with gil: block is used.

I don't want to use noexcept for trap_error since half the function does need to trigger error.

Code to reproduce the behaviour:

# liburing.pyx
cpdef int io_uring_queue_init(unsigned int entries, io_uring ring, unsigned int flags=0) nogil:
    return trap_error(io_uring_queue_init_c(entries, ring.ptr, flags))
# error.pyx
cpdef inline int trap_error(int no) nogil:
    if no >= 0:
        return no
    with gil:
        raise_error(no)


cdef inline void raise_error(signed int no=-1) except *:
    no = -errno or no
    cdef str error = strerror(-no).decode()
    raise OSError(-no, error)
performance hint: src/liburing/liburing.pyx:61:21: Exception check after calling 'trap_error' will always
require the GIL to be acquired. Declare 'trap_error' as 'noexcept' if you control the definition and
you're sure you don't want the function to raise
exceptions.

Expected behaviour

No response

OS

Linux

Python version

3.12

Cython version

3.0.8

Additional context

No response

@da-woods
Copy link
Contributor

da-woods commented Feb 15, 2024

I can't reproduce with the code given here. To make it compile I slightly modified it to the below:

# liburing.pyx

from error cimport trap_error

cpdef int io_uring_queue_init(int x) nogil:
    return trap_error(x)
# error.pxd

cpdef int trap_error(int no) nogil
# error.pyx

from libc.errno cimport errno
from libc.string cimport strerror

cpdef inline int trap_error(int no) nogil:
    if no >= 0:
        return no
    with gil:
        raise_error(no)


cdef inline void raise_error(signed int no=-1) except *:
    no = -errno or no
    cdef str error = strerror(-no).decode()
    raise OSError(-no, error)

I get no performance warnings, which I believe is correct.

@YoSTEALTH
Copy link
Contributor Author

YoSTEALTH commented Feb 15, 2024

@da-woods I had the same code before(tried all the combination). When you first compile the code you get the performance hint: warning, the second time you don't get the message because the ./build/ folder needs to be deleted and compile again fresh to see the message again.

It kept tripping me as well...

p.s. you can see actual code at: https://github.com/YoSTEALTH/Liburing/tree/master/src/liburing

@YoSTEALTH
Copy link
Contributor Author

@da-woods have you tried the code after deleting ./build/?

@da-woods
Copy link
Contributor

da-woods commented Feb 28, 2024

Sorry you're right. There is a warning performance hint.

That looks to be by design and that we don't assume the -1 return value from functions declared in pxd files:

if (return_type.exception_value is not None and (visibility != 'extern' and not in_pxd)):

I think unfortunately that is correct, and that you can compile something with:

# in the .pxd
cpdef int trap_error(int no) nogil
# in the .pyx file
cpdef inline int trap_error(int no) except -2 nogil:

so if you just see the pxd file you can't reason about the error code.

That suggests we need to improve the performance hint to tell you that since I don't think we can change the behaviour easily.

It might also we worth warning about "exception value in pyx but not in pxd" but I'm not sure about that.

@YoSTEALTH
Copy link
Contributor Author

That didn't work, was still getting the same message. So, I tried random stuff and this seem to work:

# error.pxd
cpdef int trap_error(int no) except -1 nogil
# error.pyx
cpdef inline int trap_error(int no) except -1 nogil:

Don't even know whats going on! But happy the message is gone :)

@da-woods
Copy link
Contributor

Your solution is right. The code I posted was an explanation, not something you should do

@YoSTEALTH
Copy link
Contributor Author

@da-woods Cool, Thank your very much for your amazing support.

YoSTEALTH added a commit to YoSTEALTH/Liburing that referenced this issue Feb 28, 2024
@da-woods
Copy link
Contributor

I'm going to reopen this because I think we could improve the text of the performance hints here.

@da-woods da-woods reopened this Feb 28, 2024
da-woods added a commit to da-woods/cython that referenced this issue Mar 16, 2024
In order to be called efficiently, these functions must
have the exception specification set in the pxd file since
Cython is unable to assume an implicit exception specification.

This improves the quality of the messages to draw attention to
this detail.

Closes cython#6001
da-woods added a commit that referenced this issue Mar 29, 2024
In order to be called efficiently, these functions must
have the exception specification set in the pxd file since
Cython is unable to assume an implicit exception specification.

This improves the quality of the messages to draw attention to
this detail.

Closes #6001
da-woods added a commit that referenced this issue Mar 29, 2024
In order to be called efficiently, these functions must
have the exception specification set in the pxd file since
Cython is unable to assume an implicit exception specification.

This improves the quality of the messages to draw attention to
this detail.

Closes #6001
@da-woods da-woods added this to the 3.0.10 milestone Mar 29, 2024
@YoSTEALTH
Copy link
Contributor Author

@da-woods For io_uring_enter since trap_error is being used, also by other functions, getting lots of these messages:

performance hint: src/liburing/syscall.pxd:7:24: No exception value declared for 'io_uring_enter' in pxd file.
Users cimporting this function and calling it without the gil will always require an exception check.
Suggest adding an explicit exception value.

tested with 3.0.10 and 3.1.0a0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants