pgmagick doesn't release GIL

[homm]

The idea is that image processing library could release global interpreter lock on heavy computing when it isn't interacting with python's interpretation structures. Most of the python libraries do this, while pgmagick doesn't.

This is the test case:

from __future__ import print_function

import sys
import time
import threading
from io import BytesIO

try:
    from PIL import Image
except ImportError:
    Image = None
try:
    import cv2
except ImportError:
    cv2 = None
try:
    from wand.image import Image as WandImage
except ImportError:
    WandImage = None
try:
    from pgmagick import Image as PGImage, FilterTypes, Blob, Geometry
except ImportError:
    PGImage = None
try:
    from pyvips import Image as VipsImage
except ImportError:
    VipsImage = None


imagename = sys.argv[1]


def deferred_noop():
    time.sleep(.2)


def deferred_pillow():
    im = Image.open(imagename)
    im.resize((1024, 768), Image.BICUBIC)
    im.save(BytesIO(), format='jpeg', quality=85)


def deferred_opencv():
    im = cv2.imread(imagename)
    cv2.resize(im, (1024, 768), interpolation=cv2.INTER_AREA)
    cv2.imencode(".jpeg", im, [int(cv2.IMWRITE_JPEG_QUALITY), 85])


def deferred_wand():
    with WandImage(filename=imagename) as im:
        im.resize(1024, 768, 'catrom')
        im.compression_quality = 85
        im.format = 'jpeg'
        im.save(file=BytesIO())


def deferred_pgmagick():
    im = PGImage(imagename)

    im.filterType(FilterTypes.CatromFilter)
    im.zoom(Geometry(1024, 768))

    im.quality(85)
    im.magick('jpeg')
    im.write(Blob())


def deferred_vips():
    im = VipsImage.new_from_file(imagename)
    im.resize(0.4, kernel='cubic')
    im.write_to_buffer('.jpeg', Q=85)



def test_deferred(deferred):
    start = time.time()
    t = threading.Thread(target=deferred)
    t.start()
    n = 0

    while t.isAlive():
        time.sleep(.001)
        n += 1

    print('>>> {:<20} time: {:1.3f}s  {:>3} switches'.format(
        deferred.__name__, time.time() - start, n))


test_deferred(deferred_noop)
if not Image is None:
    test_deferred(deferred_pillow)
if not cv2 is None:
    test_deferred(deferred_opencv)
if not WandImage is None:
    test_deferred(deferred_wand)
if not PGImage is None:
    test_deferred(deferred_pgmagick)
if not VipsImage is None:
    test_deferred(deferred_vips)

And my results:

>>> deferred_noop        time: 0.202s  145 switches
>>> deferred_pillow      time: 0.140s   63 switches
>>> deferred_opencv      time: 0.181s  154 switches
>>> deferred_wand        time: 0.225s  194 switches
>>> deferred_pgmagick    time: 0.096s    0 switches
>>> deferred_vips        time: 0.088s   78 switches

[komackaj]

Hi,
other libraries use either pure C API (PIL, OpenCV) and release GIL manually, SWIG with --thread or ctypes (WandImage) where GIL is autoreleased.

I believe there is a way to make it work using wrapper approach displayed in https://stackoverflow.com/a/18648366/5097156. It is nice, that it allows specifying functions, that should release GIL - so you don't pay the cost for releasing and obtaining GIL for every call to pgmagick.

If this will work, we should decide:

• which functions/methods should release GIL
• whether we want to autorelease GIL or provide a new function (e.g. Image.resize_no_gil), so we won't affect currently working code.

[komackaj]

Hi,
the aforementioned approach worked quite fine, with this WIP changeset the results of test case on image 5184x3456 are

# python3 setup.py build 
# PYTHONPATH=build/lib.linux-x86_64-3.5/ python3 test/gil.py /tmp/test_image.jpg
>>> deferred_noop        time: 0.202s  161 switches
>>> deferred_pillow      time: 0.851s  657 switches
>>> deferred_pgmagick    time: 0.649s   40 switches

[homm]

That's cool! Thanks for the update.

[komackaj]

@hhatto
Hi, could you have a look at this so we could move this forward?

[hhatto]

It is a very good point, and this wip changeset is cool.

which functions/methods should release GIL

In my opinion, I think that this is good solution.
And I think that I implement with using build option.

for example:

$ PGMAGICK_RELEASE_GIL=1 python setup.py build

diff --git a/setup.py b/setup.py
index f74f2fd..8571066 100644
--- a/setup.py
+++ b/setup.py
@@ -185,6 +185,9 @@ if _version:
 else:
     _version = '%s version: ???' % (LIBRARY)

+if 'PGMAGICK_RELEASE_GIL' in os.environ:
+    ext_compile_args.append("-DPGMAGICK_RELEASE_GIL")
+

 def version():
     """Return version string."""

diff --git a/src/_Image.cpp b/src/_Image.cpp
index c34bfb7..a7303b5 100644
--- a/src/_Image.cpp
+++ b/src/_Image.cpp
@@ -9,6 +9,12 @@
 #include "_Pixels.h"
 #include "_WithGuard.h"

+#ifdef PGMAGICK_RELEASE_GIL
+#define _GIL(stmt) with<no_gil>(stmt)
+#else
+#define _GIL(stmt) stmt
+#endif
+
 using namespace boost::python;

 namespace  {
@@ -309,7 +315,7 @@ void __Image()
 #else
         .def("write", (void (Magick::Image::*)(Magick::Blob*, const std::string&, const unsigned int) )&Magick::Image::write)
 #endif
-        .def("zoom", with<no_gil>(&Magick::Image::zoom))
+        .def("zoom", _GIL(&Magick::Image::zoom))
         .def("adjoin", (void (Magick::Image::*)(const bool) )&Magick::Image::adjoin)
         .def("adjoin", (bool (Magick::Image::*)() const)&Magick::Image::adjoin)
         .def("antiAlias", (void (Magick::Image::*)(const bool) )&Magick::Image::antiAlias)

But, I can not afford to implement it, now 😭

Patches welcome 😃