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

Is CFFI still needed? #7223

Closed
Yay295 opened this issue Jun 22, 2023 · 5 comments · Fixed by #7236
Closed

Is CFFI still needed? #7223

Yay295 opened this issue Jun 22, 2023 · 5 comments · Fixed by #7236

Comments

@Yay295
Copy link
Contributor

Yay295 commented Jun 22, 2023

The CFFI is only used in PyAccess as a replacement for the C implementation in Access.c/PixelAccess. This was done in #476 to improve performance when using PyPy (see performance profiling in #816). However, that was nine years ago, and the PyPy cpyext has been greatly improved since then. Is PyAccess still necessary for performance on PyPy, or can we go back to just using the C implementation?

@hugovk
Copy link
Member

hugovk commented Jun 22, 2023

How does performance profiling look now with PyPy, with and without CFFI?

@radarhere
Copy link
Member

Running Tests/bench_cffi_access.py,

PyPy3.9 on Windows - https://github.com/radarhere/Pillow/actions/runs/5343847790/jobs/9687477077#step:27:46

PyAccess - get: breaking at 30 iterations, 0.343930 per iteration
PyAccess - set: breaking at 32 iterations, 0.317073 per iteration
C-api - get: breaking at 245 iterations, 0.040906 per iteration
C-api - set: breaking at 259 iterations, 0.038629 per iteration

PyPy3.10 on Windows - https://github.com/radarhere/Pillow/actions/runs/5343847790/jobs/9687477221#step:27:46

PyAccess - get: breaking at 36 iterations, 0.278341 per iteration
PyAccess - set: breaking at 42 iterations, 0.243066 per iteration
C-api - get: breaking at 208 iterations, 0.048108 per iteration
C-api - set: breaking at 198 iterations, 0.050532 per iteration

PyPy3.9 on Ubuntu - https://github.com/radarhere/Pillow/actions/runs/5343847795/jobs/9687476737#step:8:63

PyAccess - get: breaking at 58 iterations, 0.173087 per iteration
PyAccess - set: breaking at 76 iterations, 0.132276 per iteration
C-api - get: breaking at 187 iterations, 0.053598 per iteration
C-api - set: breaking at 193 iterations, 0.052231 per iteration

PyPy3.10 on Ubuntu - https://github.com/radarhere/Pillow/actions/runs/5343847795/jobs/9687476573

PyAccess - get: breaking at 63 iterations, 0.160498 per iteration
PyAccess - set: breaking at 83 iterations, 0.121258 per iteration
C-api - get: breaking at 197 iterations, 0.050895 per iteration
C-api - set: breaking at 204 iterations, 0.050110 per iteration

So PyAccess is running 2-8 times faster than the C API.

@Yay295
Copy link
Contributor Author

Yay295 commented Jun 22, 2023

So PyAccess is running 2-8 times faster than the C API.

Isn't that backwards? In the last test PyAccess took 0.12-0.16 seconds per iteration, while the C API took 0.05 seconds per iteration. So it looks like the C API is actually faster than PyAccess.

@wiredfool
Copy link
Member

I'm fine with taking it out -- There were some somewhat janky hacks to make that work (the unsafe members in the c struct) so it would be nice to clean that all up. Assuming that the benchmarks are pointing the right way.

@radarhere
Copy link
Member

I've created PR #7236 to stop using PyAccess, and deprecate it.

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.

4 participants