fix: async access to gil updates; notes on perf #159
Conversation
| let generator = self.generator.clone(); | ||
| let event_loop = self.event_loop.clone(); | ||
|
|
||
| let stream = tokio::task::spawn_blocking(move || { |
There was a problem hiding this comment.
Could we add a small comment here for future newbie readers here? For example, (correct me where I'm wrong), is the key point of this block's diff that it is putting the Python GIL acquisition onto a dedicated task that won't block the async runtime - whereas previously is was blocking the runtime?
There was a problem hiding this comment.
I think this is a very subtle but important detail to the overall system performance that could just look like a simple indentation to the right from the previous code.
There was a problem hiding this comment.
Yeah. I added a comment in the top-level readme, but i'll add a note here.
This does add overheads. Under low GIL contention, this is more expensive, because we have to schedule another a task on a blocking thread pool, then await it.
Under high GIL contentions, we need to offload it or we will strongly block the async threads.
No way to know, so we pay the price of the GIL - this time two fold.
There was a problem hiding this comment.
I meant can we put a comment like that (similar to README) co-located inline with the code here when someone is scanning through how the code works?
There was a problem hiding this comment.
Not blocking (pun intended), but I think it would be useful
There was a problem hiding this comment.
fixed here
There was a problem hiding this comment.
merged too quick. added to the next PR.
What does the PR do?