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

[Enhancement] GIL release #36

Closed
f-fanni opened this issue May 25, 2020 · 2 comments
Closed

[Enhancement] GIL release #36

f-fanni opened this issue May 25, 2020 · 2 comments

Comments

@f-fanni
Copy link
Contributor

f-fanni commented May 25, 2020

As we briefly discussed in pull request #35, it could be beneficial to release the GIL during the computation, to allow other threads to run concurrently.

However, I'm opening a issue instead of a new pull request, because I realized that in the current state my previous commit is not thread safe. Indeed, even though the marcher does not write in any shared memory, it reads directly from the python variables passed as arguments. This means, for example, that while thread A is computing the SDF for phi, thread B could change the content in phi.

This lead to the following possibilities:

  1. Do not release the GIL. It limit performances in multi-threaded applications, but has good performances in single-thread.
  2. Copy the parameters and then release the GIL. It allows for safe multithreading, but the copy of the parameters could be slow, and could finish the memory available, if the matrices are big enough. The parameters copy can cause a loss in performances in single thread.
  3. Release the GIL without copying the parameters, and leave on the programmer the responsibility for thread safeness. It looks like a bad idea, and is what my previous commit does.
  4. Add a parameter to select a behavior among the previous 3 points. This would be the most flexible choice, but would need to be well documented.
@jkfurtney
Copy link
Member

jkfurtney commented May 25, 2020

OK, good catch, that would have been hard to track down later. I think option 3 is out, this is quite dangerous. Like the idea of having an optional argument allow_threading which makes copies of the input arrays and releases the GIL inside the C++ code (toggles your options 1 and 2). It should default to False to keep the existing behavior for existing code. It would be easy to create the copies with np.copy in the pre_process_args in pfmm.py. I would think doing the copy in Python is still fast and it keeps the C++ code simple.

@jkfurtney
Copy link
Member

I thought about this a little more and decided not to pursue this. Shared memory multi-threading is difficult to do correctly and would add complexity to the library. At least for now, the multiprocessing library should allow people to make use of multi-core computers for parametric studies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants