Skip to content

Commit

Permalink
Acquire/release the GIL while processing tile requests
Browse files Browse the repository at this point in the history
Fixes crashes on some Linux distros, potentially improves performance.

When handling tile requests we currently use an openmp critical block in a
callback registered with libmypaint. The callback calls into Python code
without locking the GIL. This sometimes crashes mypaint in numpy's memory
cache allocator on some Linux distros that compile numpy with run-time
asserts (without `-DNDEBUG`), like Gentoo, as numpy uses Python's GIL
internally as a locking mechanism for its non-thread-safe global cache
management.

Acquiring the GIL in the C callback, before calling into Python, ensures
that the GIL is still locked by the current thread when it reaches numpy's
code, and thus prevents the crashes. We yield the GIL whenever Python code
calls again into libmypaint, This allows other threads to acquire it, and
concurrent callbacks to run, which prevents deadlocks that would otherwise
happen while waiting for all the callbacks to finish on Python's side. When
libmypaint is done we re-acquire the GIL, and return up to the callback
where the GIL is released again after some Python reference count
bookkeeping.

The OpenMP critical block is no longer necessary after introducing the GIL
locking mechanism. This would potentially improve performance as the C code
in libmypaint can process multiple callbacks at the same time during the
`Py_BEGIN_ALLOW_THREADS' period that yields the GIL.
  • Loading branch information
rozenglass authored and jplloyd committed Sep 12, 2020
1 parent 08bc044 commit 4429d78
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 5 deletions.
8 changes: 6 additions & 2 deletions lib/brush.hpp
Expand Up @@ -66,8 +66,12 @@ class Brush {
bool stroke_to (Surface * surface, float x, float y, float pressure, float xtilt, float ytilt, double dtime, float viewzoom, float viewrotation, float barrel_rotation, bool linear)
{
MyPaintSurface *c_surface = surface->get_surface_interface();
bool retval = mypaint_brush_stroke_to(c_brush, c_surface, x, y, pressure, xtilt, ytilt, dtime, viewzoom, viewrotation, barrel_rotation, linear);
return retval;
bool stroke_finished_or_empty;

Py_BEGIN_ALLOW_THREADS
stroke_finished_or_empty = mypaint_brush_stroke_to(c_brush, c_surface, x, y, pressure, xtilt, ytilt, dtime, viewzoom, viewrotation, barrel_rotation, linear);
Py_END_ALLOW_THREADS
return stroke_finished_or_empty;
}

double get_total_stroke_painting_time()
Expand Down
7 changes: 5 additions & 2 deletions lib/pythontiledsurface.cpp
Expand Up @@ -36,8 +36,9 @@ tile_request_start(MyPaintTiledSurface *tiled_surface, MyPaintTileRequest *reque
const int ty = request->ty;
PyArrayObject* rgba = NULL;

#pragma omp critical
{
PyGILState_STATE gstate = PyGILState_Ensure();

rgba = (PyArrayObject*)PyObject_CallMethod(self->py_obj, "_get_tile_numpy", "(iii)", tx, ty, readonly);
if (rgba == NULL) {
request->buffer = NULL;
Expand All @@ -59,7 +60,9 @@ tile_request_start(MyPaintTiledSurface *tiled_surface, MyPaintTileRequest *reque
Py_DECREF((PyObject *)rgba);
request->buffer = (uint16_t*)PyArray_DATA(rgba);
}
} // #end pragma opt critical

PyGILState_Release(gstate);
}


}
Expand Down
4 changes: 3 additions & 1 deletion lib/tiledsurface.hpp
Expand Up @@ -69,7 +69,9 @@ class TiledSurface : public Surface {
MyPaintRectangle* rects = this->bbox_rectangles;
MyPaintRectangles bboxes = {BBOXES, rects};

mypaint_surface_end_atomic((MyPaintSurface *)c_surface, &bboxes);
Py_BEGIN_ALLOW_THREADS
mypaint_surface_end_atomic((MyPaintSurface *)c_surface, &bboxes);
Py_END_ALLOW_THREADS

// The capacity of the bounding box array will most often exceed the number
// of rectangles that are actually used. The call to mypaint_surface_end_atomic
Expand Down

0 comments on commit 4429d78

Please sign in to comment.