Skip to content

Release the GIL whenever possible to allow multithreading from python #123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 19, 2024
Merged

Release the GIL whenever possible to allow multithreading from python #123

merged 3 commits into from
Dec 19, 2024

Conversation

theartful
Copy link
Contributor

@theartful theartful commented Dec 3, 2024 •
edited
Loading

This enables multithreading from within python. Example experiment:

import concurrent.futures
import time

import numpy as np
import poselib

num_tasks = 100

# Prepare task
camera = {
    "model": "SIMPLE_PINHOLE",
    "width": 1280,
    "height": 720,
    "params": [1.2 * 1280, 1280/2, 720/2]
}
points2D_1 = np.random.uniform(low=0.0, high=720, size=(100, 2))
points2D_2 = np.random.uniform(low=0.0, high=720, size=(100, 2))

now = time.time()

with concurrent.futures.ThreadPoolExecutor() as e:
    for _ in range(num_tasks):
        e.submit(poselib.estimate_relative_pose, points2D_1, points2D_2,
                 camera, camera)

then = time.time()

print(f"Done in {then - now} seconds")

Output on my machine before:

Done in 46.720627784729004 seconds

After:

Done in 2.9223525524139404 seconds

@pablospe
Copy link
Collaborator

@vlarsson Check this PR is you are ok with it.

@vlarsson
Copy link
Collaborator

My understanding of the python GIL is limited. I don't know if there is any benefit in having some of these as callguards m.def("foo", &foo, py::call_guard<py::gil_scoped_release>()); ? For the ones where the GIL is released for the entire function call.

So am I correct in understanding that we need to have the GIL whenever we write to py::dict? Or why are we re-acquiring the GIL in some of these?

@theartful
Copy link
Contributor Author

theartful commented Dec 14, 2024 •
edited
Loading

@vlarsson as far as I know, whenever we deal with python objects we should hold the GIL. When writing this PR, I experimented with simply always releasing the GIL, but unsurprisingly I got crashes. Maybe writing an entirely new object (py::dict) can be ok, but I wouldn't risk it.

m.def("foo", &foo, py::call_guard<py::gil_scoped_release>()); Is probably cleaner. I will change the simple functions to use that.

I will also experiment with not re-acquiring the GIL when writing py::dicts and get back to you.

@theartful
Copy link
Contributor Author

theartful commented Dec 14, 2024 •
edited
Loading

@vlarsson Holding the GIL while writing py::dict is necessary. I tried changing estimate_relative_pose_wrapper to release the GIL, and not re-acquire it back, and I reran the same experiment I shared in the PR, and I got segfault.

I changed the code to use py::call_guard whenever possible (the force push for rebasing).

@vlarsson
Copy link
Collaborator

Ah sorry, the recent PR #125 added some merge conflicts to this. Could you fix this? and then we can merge it.

@theartful
Copy link
Contributor Author

Should be ok now

vlarsson
vlarsson approved these changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@vlarsson vlarsson left a comment

Choose a reason for hiding this comment

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

Thanks!

@vlarsson vlarsson merged commit adc1848 into PoseLib:master Dec 19, 2024
1 check passed
@vlarsson vlarsson mentioned this pull request Aug 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablospe pablospe pablospe approved these changes

@vlarsson vlarsson vlarsson approved these changes

@Parskatt Parskatt Awaiting requested review from Parskatt Parskatt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theartful @pablospe @vlarsson