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

Re-introduce thread pool for the sync functions (maybe) #22

Closed
sansyrox opened this issue Jun 21, 2021 · 14 comments · Fixed by #23
Closed

Re-introduce thread pool for the sync functions (maybe) #22

sansyrox opened this issue Jun 21, 2021 · 14 comments · Fixed by #23

Comments

@sansyrox
Copy link
Member

#20 (comment)

Read discussion above

And see if introducing a threadpool will impact asgi compliance(most probably not) but still check again.

@JackThomson2
Copy link
Contributor

Using https://docs.rs/tokio/1.7.1/tokio/task/fn.spawn_blocking.html may be the best way, it'll spawn inside tokios internal threadpool and allow you to await it!

@JackThomson2
Copy link
Contributor

JackThomson2 commented Jun 21, 2021

Also wondering if it's possible, as the routes are being added we store some meta data about them rust side, i.e. if they're async or not so we don't have to waste time during a request spent inside the GIL which I gather bottlenecks down to 1 thread.

@sansyrox
Copy link
Member Author

@JackThomson2 , I just did that. You can have a look here: #20

@sansyrox
Copy link
Member Author

Thank you for this(https://docs.rs/tokio/1.7.1/tokio/task/fn.spawn_blocking.html) . 😀
I will read more about it.

@JackThomson2
Copy link
Contributor

@JackThomson2 , I just did that. You can have a look here: #20

I suggest we store that info in the rust side, I guess the less time we spend in the GIL the better from what I gather. So each of the hashtables stores an enum of the py function + info about the function. Then a rough idea of what we could do. Something like


let res =  if is_async {
   python_async_fun().await
} else {
  spawn_blocking(move || { python_sync_fun() }).await
}```

Just a rough idea I had in my head :D 

@sansyrox
Copy link
Member Author

sansyrox commented Jun 22, 2021

@JackThomson2 , we have also have a gitter room in case you want to join (https://gitter.im/robyn_/community#)

I have also added the non blocking sync feature in the latest release. You can have a look 😄

@JackThomson2
Copy link
Contributor

@sansyrox My concern was around the fact that for an async function now we acquire the GIL 3x and for Sync 2x, this is going to bottleneck the server. This can be reduced by one each by storing the async info rust side rather than in a py dict

@sansyrox
Copy link
Member Author

sansyrox commented Jun 22, 2021

@JackThomson2 , how can you differentiate in rust between sync and async without calling those functions?
I was unable to find any way.

GIL 3x and for Sync 2x

For this, we can do one thing by acquiring gil once and allowing threads inside the code block. But from what I understand, those all are essentially the same things. Do correct me if I am wrong.

@JackThomson2
Copy link
Contributor

@sansyrox When the function is added can't we use the same method? So the router stores get_routes: DashMap<Route, PyFunction>

@sansyrox sansyrox reopened this Jun 22, 2021
@sansyrox
Copy link
Member Author

sansyrox commented Jun 22, 2021

@JackThomson2 , that makes sense.

We would save this gil acquision

    let function: PyFunction = Python::with_gil(|py| {
        let process_object_wrapper: &PyAny = process_object.as_ref(py);
        let py_dict = process_object_wrapper.downcast::<PyDict>().unwrap();
        let is_async: bool = py_dict.get_item("is_async").unwrap().extract().unwrap();
        let handler: &PyAny = py_dict.get_item("handler").unwrap();
        if is_async {
            let coro = handler.call0().unwrap();
            PyFunction::CoRoutine(coro.into())
        } else {
            PyFunction::SyncFunction(handler.into())
        }
    });

on every function call

@sansyrox
Copy link
Member Author

@JackThomson2 , thank you for the feedback. 😄 I'll implement it tomorrow.

But we are still locking it twice here

        PyFunction::CoRoutine(coro) => {
            let x = Python::with_gil(|py| {
                let x = coro.as_ref(py);
                pyo3_asyncio::into_future(x).unwrap()
            });
            let output = x.await.unwrap();
            Python::with_gil(|py| -> PyResult<String> {
                let contents: &str = output.extract(py).unwrap();
                Ok(contents.to_string())
            })
            .unwrap()
        }

Do you have any suggestions for this as well?

@JackThomson2
Copy link
Contributor

I think it looks like a limitation with the py03_asyncio, it may be worth asking the library owned if there's a way to return the result from the async function without having to enter the gil again

@sansyrox
Copy link
Member Author

sansyrox commented Jun 23, 2021

@JackThomson2 , I did talk a lot with the maintainer(when I was creating robyn initially). I don't think it is possible atm. I'll try asking again.

it may be worth asking the library owned if there's a way to return the result from the async function without having to enter the gil again

Also, it is restricted natively at pyo3 not pyo3-asyncio otherwise it results in a deadlock.

@sansyrox
Copy link
Member Author

Closing this issue as this seems to be complete. @JackThomson2 feel free to reopen if you have anything to add.

Thanks again! 😄

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

Successfully merging a pull request may close this issue.

2 participants