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

Disable CYTHON_FAST_PYCALL on Py3.10 (0.29.x) #4735

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

da-woods
Copy link
Contributor

It causes issues while profiling or debugging where global
variables can end up inadvertently changed.

Test largely copied from @seberg's reproducer

Fixes #4609

It causes issues while profiling or debugging where global
variables can end up inadvertently changed.

Fixes cython#4609
Comment on lines +195 to +196
// On Python 3.10 it causes issues when used while profiling/debugging
#define CYTHON_FAST_PYCALL (PY_VERSION_HEX < 0x030A0000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this perhaps be guarded with a debug-flag for python 3.10 to not regress performance in release builds?

Copy link
Contributor Author

@da-woods da-woods Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Unfortunately it's Python-level debugging (i.e using sys.settrace) that causes the problem so it needs applying generally

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the difference shows up in benchmarks?

@da-woods
Copy link
Contributor Author

@mattip I made a microbenchmark to try to measure this:

def call_with_arg(func, int count, arg):
    cdef int i
    sum = 0
    for i in range(count):
        sum += func(arg)
    return sum

gl = dict()
exec("""
def pyfunc(a):
    return a*2+1
""", gl)
pyfunc = gl["pyfunc"]

def getstats(repeats):
    import numpy as np
    return {"mean": np.mean(repeats), "median": np.median(repeats), "min": np.min(repeats)}

def run():
    from timeit import repeat
    print(getstats(repeat("call_with_arg(pyfunc, 1000, 11.5)", globals=globals(), number=10000, repeat=10)))

(I did also try a few variations with calling a=arg, *args and **kwds, but they don't look to go through this code-path anyway so I've omitted them)

Representative results are:

Before
{'mean': 1.691903970806743, 'median': 1.7044297735119471, 'min': 1.615239927021321}

After
{'mean': 1.6774068667989923, 'median': 1.6857521675119642, 'min': 1.6102126939804293}

To be honest I think my laptop is throttling itself too quickly to be really useful as a benchmark. However, I'm pretty confident that it won't cause a dramatic performance regression though.

Feel free to investigate more yourself if it's something you're worried about though - I'm sure I haven't covered every case!

@scoder
Copy link
Contributor

scoder commented Apr 15, 2022

I was recently asked what the performance difference is between the Limited API and CPython-optimised Cython, and had no answer. I then tried it with a simple caching example that uses ints, dicts and lists, and got a bit more than a 10% slowdown for the Limited API case. So, there are visible differences in the tricks that Cython pulls in the C-API, but in the end, these tricks have to add up to reach that difference, and are obviously highly use case specific. Disabling one of them should usually have a small effect in most cases, unless your code really depends on it.

That said – if you want better performance, use Cython 3. If you value extreme language stability instead, stay with Cython 0.29.x for now. But be aware that maintenance for 0.29.x will probably end soon after Cython 3.0 is out. And that won't take as forever as it seemed for the last few years.

@seberg
Copy link
Contributor

seberg commented May 6, 2022

Unfortunately, I somewhat expect the number of people running into this may skyrocket soon due to Python 3.10 getting more prelevant. So the packages using Cython should do bug-fix releases soon, ideally with this patch included.

Can we make sure the next bug-fix release happens soonish?

@seberg
Copy link
Contributor

seberg commented May 11, 2022

At this point, I think this is the last thing that would be nice to have a for a new NumPy release. I suppose, we could try to work-around it in NumPy, but it would be more convenient to have a release.

As far as I can tell, all issues marked for 0.29.29 are fixed (just not closed because they are not necessarily fixed for Cython 3)?
How ard would it be to do a release? Because I it would seem pretty helpful.

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

Successfully merging this pull request may close these issues.

5 participants