Skip to content

Commit

Permalink
Fix Python GIL lock handling (Fixes #6524, Fixes #5631) (#6525)
Browse files Browse the repository at this point in the history
* Fix Python GIL lock handling (Fixes #6524, Fixes #5631)

As disscussed in #6523 (comment)
and later in #6524,
pybind11 v2.8.1 added some defensive checks that fail for halide,
namely in `python_tutorial_lesson_04_debugging_2`
and `python_tutorial_lesson_05_scheduling_1`.

#6524 (comment) notes:
> * Python calls a Halide-JIT-generated function , which runs with the GIL held.
> * Halide runtime spawns worker threads.
> * The worker threads try to call pybind11's py::print function to emit traces.
> * Pybind11 complains, correctly, that the worker thread doesn't hold the GIL.
>
> Trying to acquire the GIL hangs, because the main thread is still holding it. I tried teaching the main thread to release the GIL (as suggested in #5631), but I still saw hangs when I tried this.

I have tried, and just dropping the lock before calling into halide,
or just acquiring it in `halide_python_print` doesn't work,
we need to do both.

I have verified that the two tests fail without this fix,
and pass with it.

(cherry picked from commit b8eb22d)
  • Loading branch information
LebedevRI authored and alexreinking committed Jan 5, 2022
1 parent 6c2adf2 commit ae942b2
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 3 deletions.
1 change: 1 addition & 0 deletions python_bindings/src/PyError.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ void halide_python_error(void *, const char *msg) {
}

void halide_python_print(void *, const char *msg) {
py::gil_scoped_acquire acquire;
py::print(msg, py::arg("end") = "");
}

Expand Down
9 changes: 8 additions & 1 deletion python_bindings/src/PyFunc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ void define_func(py::module &m) {
.def(
"realize",
[](Func &f, Buffer<> buffer, const Target &target) -> void {
py::gil_scoped_release release;
f.realize(buffer, target);
},
py::arg("dst"), py::arg("target") = Target())
Expand All @@ -125,14 +126,20 @@ void define_func(py::module &m) {
.def(
"realize",
[](Func &f, std::vector<Buffer<>> buffers, const Target &t) -> void {
py::gil_scoped_release release;
f.realize(Realization(buffers), t);
},
py::arg("dst"), py::arg("target") = Target())

.def(
"realize",
[](Func &f, const std::vector<int32_t> &sizes, const Target &target) -> py::object {
return realization_to_object(f.realize(sizes, target));
std::optional<Realization> r;
{
py::gil_scoped_release release;
r = std::move(f.realize(sizes, target));
}
return realization_to_object(*r);
},
py::arg("sizes") = std::vector<int32_t>{}, py::arg("target") = Target())

Expand Down
9 changes: 8 additions & 1 deletion python_bindings/src/PyPipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,27 @@ void define_pipeline(py::module &m) {

.def(
"realize", [](Pipeline &p, Buffer<> buffer, const Target &target) -> void {
py::gil_scoped_release release;
p.realize(Realization(buffer), target);
},
py::arg("dst"), py::arg("target") = Target())

// This will actually allow a list-of-buffers as well as a tuple-of-buffers, but that's OK.
.def(
"realize", [](Pipeline &p, std::vector<Buffer<>> buffers, const Target &t) -> void {
py::gil_scoped_release release;
p.realize(Realization(buffers), t);
},
py::arg("dst"), py::arg("target") = Target())

.def(
"realize", [](Pipeline &p, std::vector<int32_t> sizes, const Target &target) -> py::object {
return realization_to_object(p.realize(std::move(sizes), target));
std::optional<Realization> r;
{
py::gil_scoped_release release;
r = std::move(p.realize(std::move(sizes), target));
}
return realization_to_object(*r);
},
py::arg("sizes") = std::vector<int32_t>{}, py::arg("target") = Target())

Expand Down
5 changes: 4 additions & 1 deletion src/PythonExtensionGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,9 @@ void PythonExtensionGen::compile(const LoweredFunc &f) {
// Python already converted this.
}
}
dest << " int result = " << f.name << "(";
dest << " int result;\n";
dest << " Py_BEGIN_ALLOW_THREADS\n";
dest << " result = " << f.name << "(";
for (size_t i = 0; i < args.size(); i++) {
if (i > 0) {
dest << ", ";
Expand All @@ -359,6 +361,7 @@ void PythonExtensionGen::compile(const LoweredFunc &f) {
}
}
dest << ");\n";
dest << " Py_END_ALLOW_THREADS\n";
release_buffers();
dest << R"INLINE_CODE(
if (result != 0) {
Expand Down

0 comments on commit ae942b2

Please sign in to comment.