Move Python::attach calls outside of tokio event loop in future_into_py #60
Conversation
stepancheg
force-pushed
the
with-gil-2
branch
3 times, most recently
from
0075bdd to
b42373a
Compare
|
Can I have code review here please? This PR may or may not lead to performance degradation depending on use case, but I'm fairly certain this PR is correct even if it makes performance worse in certain scenarios. Normal well-behaving Python thread may legally hold GIL for 5 milliseconds at a time (more if a thread is doing some operations which are supposed to be fast, but in fact slow like some operations on very large lists which are atomic). It is worse if there is large number of threads, and especially if an app is CPU-bound. Given that, call to Maybe @kylebarron? |
| where | ||
| F: Future<Output = PyResult<T>> + Send + 'static, | ||
| T: for<'py> IntoPyObject<'py>, | ||
| T: for<'py> IntoPyObject<'py> + Send + 'static, |
There was a problem hiding this comment.
This is a breaking change, right? Should we highlight that in the Changelog? What if someone wants to use this with a non-Send return type?
There was a problem hiding this comment.
Practically it is uncommon to have a future which is Send, but result is not Send.
However, generic code (like generic wrappers of future_into_py) is affected, so it is worth highlighting in changelog. Will do.
| if e.is_panic() { | ||
| // We should not hold GIL inside async-std/tokio event loop, | ||
| // because a blocked task may prevent other tasks from progressing. | ||
| R::spawn_blocking(|| { |
There was a problem hiding this comment.
Kinda hard to tell from the diff, but the only change here is the addition of spawn_blocking?
There was a problem hiding this comment.
Yes.
There is "hide whitespace" option in GitHub diff viewer.
stepancheg
force-pushed
the
with-gil-2
branch
from
b42373a to
742439a
Compare
|
Updated the changelog. |
There was a problem hiding this comment.
Ideally I'd love to get another approval before merging.
Also, the release cadence for breaking releases of pyo3-async-runtimes usually matches the upstream pyo3 version, and pyo3 0.26 was just released, so I'm not sure on the release timeline for this.
stepancheg
force-pushed
the
with-gil-2
branch
from
742439a to
f6ebac3
Compare
|
As long as this is committed, we can use revision from git (and report back if we find something wrong with it). @davidhewitt or @iksteen maybe you could also approve the PR? |
|
Thank you two for the reviews! |
|
FWIW, in the last days after we starting using this change, our GIL situation seems improved, although we cannot say that with 100% confidence because there are too many changes in Python code happened, and monitoring for our python clients is not perfect. We did not observe regressions though. |
Resolved #58 (not sure, completely or not).
Previous PR #62 improves the situation, this improves it further.
This PR includes #59.