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

Release GIL while calling WebPAnimDecoderGetNext #7782

Merged
merged 5 commits into from Feb 22, 2024

Conversation

evanmiller
Copy link
Contributor

WebPAnimDecoderGetNext is a relatively expensive pure-C call that currently holds the Python GIL. Release it!

(Haven't tested this branch but it seemed like a straightforward change.)

@radarhere
Copy link
Member

Testing with

import timeit
from PIL import Image
im = Image.open('Tests/images/iss634.webp')

def decode():
  im._decoder.reset()
  for i in range(im.n_frames):
    im._decoder.get_next()

print(timeit.timeit(decode, number=1000))

I find it hard to say that this is definitively faster than main.

@evanmiller
Copy link
Contributor Author

Hi, I don't expect it to be faster than main in a single-threaded context – the point of releasing the GIL is to allow multiple threads to call get_next() concurrently (i.e. on different images).

@evanmiller
Copy link
Contributor Author

evanmiller commented Feb 7, 2024

I think you'll want a test with code like

import concurrent
import timeit
from PIL import Image
images = [Image.open('Tests/images/iss634.webp') for _ in range(100)]

def decode(im):
  im._decoder.reset()
  for i in range(im.n_frames):
    im._decoder.get_next()

with concurrent.futures.ThreadPoolExecutor(max_workers=5) as pool:
  pool.map(decode, images)

(untested but hopefully you get the idea)

src/_webp.c Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

You're right, that test code does demonstrate a substantial speed increase.

Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@evanmiller
Copy link
Contributor Author

Updated w/ your code suggestion... I think you will have to re-approve the workflow run

src/_webp.c Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Feb 8, 2024

Please could you post a summary of the speed increase?

And let's include this in the release notes.

@radarhere
Copy link
Member

I've pushed a commit with release notes, based on https://pillow.readthedocs.io/en/stable/releasenotes/9.3.0.html#release-gil-when-converting-images-using-matrix-operations

@hugovk hugovk added the automerge Automatically merge PRs that are ready label Feb 22, 2024
@radarhere radarhere merged commit f8a54b7 into python-pillow:main Feb 22, 2024
57 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PRs that are ready Performance WebP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants