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

Remove all WITH_* flags from _imaging.c and other flags #8211

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

homm
Copy link
Member

@homm homm commented Jul 7, 2024

Rationale

  • The value of these flags is questionable. I don't see real use cases.
  • Python code is not aware of these flags. After compiling without some of them, Pillow functionality is not guaranteed.

@@ -215,16 +198,12 @@ PyImaging_AsImaging(PyObject *op) {

void
ImagingSectionEnter(ImagingSectionCookie *cookie) {
#ifdef WITH_THREADING
Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried to measure performance penalty for saving state.

Empty function call:

In [2]: %timeit Image.core.test_threading()
64.1 ns ± 0.842 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

With ImagingSectionEnter/ImagingSectionLeave:

In [6]: %timeit Image.core.test_threading()
109 ns ± 0.951 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

@homm homm changed the title Remove all WITH_* flags from _imaging.c Remove all WITH_* flags from _imaging.c and other flags Jul 7, 2024
@wiredfool
Copy link
Member

LGTM, This probably deserves a changelog entry -- as technically it's possible that people have been editing this.

#endif

#ifdef WITH_UNSHARPMASK
/* Kevin Cazabon's unsharpmask extension */
Copy link
Member

Choose a reason for hiding this comment

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

Just for clarity - you're removing 'Kevin Cazabon' presumably because the code has been modified over time?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the reason were due to modifications over time, I would have included additional names accordingly. The actual reason is that I was unsure of the specific value this information provides in the comments. However, if this is important, I can revert the change.

@hugovk
Copy link
Member

hugovk commented Jul 18, 2024

Please mention this in the release notes: https://github.com/python-pillow/Pillow/blob/main/docs/releasenotes/11.0.0.rst

by @radarhere, lost during rebase
@homm
Copy link
Member Author

homm commented Jul 28, 2024

@hugovk @wiredfool
Release notes added

@homm homm merged commit d8447de into python-pillow:main Aug 2, 2024
53 checks passed
@homm homm deleted the remove-c-flags branch August 2, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants