feat: get rid of intermediate representations of requests and responses

[AntoineRR]

Description

This PR is a draft to pass a Request and Response struct to the Python routes (Response is used in the "after" middleware).
Currently, I managed to do it, but unfortunately the body of the Request and Response objects is wrapped in the ActixBytesWrapper struct so it is unusable on Python's side.

This will be a breaking change, as the response and request objects you get from Python's struct won't be dictionaries anymore. This would be used like this:

@app.get("/sync/param/:id")
def sync_param(request: Request):
    id = request.params["id"]
    return id

instead of this:

@app.get("/sync/param/:id")
def sync_param(request: dict):
    id = request["params"]["id"]
    return id

[netlify]

✅ Deploy Preview for robyn canceled.

Name	Link
🔨 Latest commit	857f894
🔍 Latest deploy log	https://app.netlify.com/sites/robyn/deploys/64090f158c60c40008cb82a1

[sansyrox]

@AntoineRR , I think this PR will conflict with #363 .

[AntoineRR]

Yes probably, we should merge the other PR prior to this one and I'll do the corrections after. This PR is not ready anyway for now

[AntoineRR]

@sansyrox, I have something that works now.
With this PR, we can fix the middlewares as well as #244.
There are some limitations regarding request and response modification in middlewares however:

• I could not add a header by doing headers["new"] = "header" but had to create a new dictionary and set it as the headers with request.headers = new_dict
• The str of the body is stored in request.body.content. I would like to make it accessible directly from request.body but couldn't do that for now.

If you have any idea on how to fix this, let me know !

[AntoineRR]

Hey @sansyrox, I improved the way the body is handled in this PR, using as_str, as_bytes and set_body methods. Setting the headers in the Response still isn't the best (maybe we could use a set_headers method as well?) but I think the PR is ready for a first review.

[sansyrox]

Thanks for the great work @AntoineRR 😄

I have a few comments at the PR and I will also try to do another review tomorrow 😅

[AntoineRR]

Hey @sansyrox, thanks a lot for the great suggestions. I updated the code.
There is just one more thing I am still struggling to do, and it is modifying the headers directly through request.headers["some_key"] = "some_value". This should be doable by changing the type of headers to Py<Pydict>, but I would like to avoid this as it would make us use the GIL in more places in the code where I feel it shouldn't be required. LMK if you have a suggestion for this!

[sansyrox]

Hey @AntoineRR 👋

Firstly, thank you for the amazing work! 😄

Regarding your query -> Actually Py containers are GIL independent. (https://docs.rs/pyo3/latest/pyo3/struct.Py.html) So, you won't be making extra GIL calls. Additionally, we will be reducing the copies of structs.

[AntoineRR]

@sansyrox I think we have to acquire the GIL to perform any operation on the headers when it's wrapped in a Py, be it read or write. For example we'll have to do it when adding global headers to Request and Response.

[sansyrox]

@sansyrox I think we have to acquire the GIL to perform any operation on the headers when it's wrapped in a Py, be it read or write. For example we'll have to do it when adding global headers to Request and Response.

@AntoineRR , I wouldn't worry too much about it as I was going through the codebase and we can optimize this by GIL batching(something that is highly due).

And I believe this will provide a much better interface to work with. And good developer experience is the top most priority imo

[AntoineRR]

What do you mean by "GIL batching"? Does this mean you want to provide a way to perform several operations at once while holding the GIL?
If we don't mind acquiring the GIL in some more places, I can try to wrap the field in a Py. Being able to assign to the headers like headers["new"] = "value" is a must have for developer experience of course ^^

[sansyrox]

@AntoineRR ,

What do you mean by "GIL batching"? Does this mean you want to provide a way to perform several operations at once while holding the GIL?

exactly!

If we don't mind acquiring the GIL in some more places, I can try to wrap the field in a Py.

I believe this will be a temporary drop in performance and we can fix it in the upcoming releases.

[AntoineRR]

Ok I could try it like this.
If it doesn't play well, I thought it is also possible to use different structs for using in Rust and Python, and convert one into the other when required. This may be cheaper than acquiring the GIL and more similar to what we have now, i.e. using a Request object in Rust, converting it to a Hashmap to use in Python and then back to Request, except the Hashmap would be a new object, a PyRequest containing only the field we want to expose. What do you think about this alternative?

[sansyrox]

@AntoineRR , I think this approach will be faster than acquiring and releasing GIL on edits. Do you think we should start from this?

[AntoineRR]

I think it should be faster too. Let's try this idea then! I will work on this in the following days and keep you updated here.

[sansyrox]

@AntoineRR ,sounds great 😊

[sansyrox]

Hey @AntoineRR 👋

I just added this (#443) . It will now eliminate the guessing of performance 😄

[AntoineRR]

Nice, thanks! That's good to know 😉

[codspeed-hq]

CodSpeed Performance Report

Merging #397 request-response-improvments (450cad1) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 68 untouched benchmarks

🆕 6 new benchmarks
⁉️ 4 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	request-response-improvments	Change
⁉️	test_binary_output_sync	31.6 ms	N/A	N/A
🆕	test_binary_output[/sync/octet/response-sync octet response]	N/A	31.1 ms	N/A
⁉️	test_binary_output_response_sync	31.9 ms	N/A	N/A
🆕	test_binary_output[/sync/octet-sync octet]	N/A	30.8 ms	N/A
🆕	test_binary_output[/async/octet/response-async octet response]	N/A	31.2 ms	N/A
⁉️	test_binary_output_async	31.6 ms	N/A	N/A
🆕	test_middlewares[async]	N/A	30.4 ms	N/A
🆕	test_binary_output[/async/octet-async octet]	N/A	30.9 ms	N/A
🆕	test_middlewares[sync]	N/A	30.4 ms	N/A
⁉️	test_binary_output_response_async	31.9 ms	N/A	N/A

[sansyrox]

Hey @AntoineRR 👋 I hope all's good.

Is this PR up for review?

[AntoineRR]

Hey @sansyrox, no I'm still working on this. I'll assign you as a reviewer once the PR is ready

[AntoineRR]

Hey @sansyrox, I finally achieved what I wanted! The PR is ready for a first review, or you can wait until I rebased everything correctly (I see there is a new conflict with the latest commit).
With my latest commit, the request.headers field can be used as a true python dict without any issue and the request.body field is either a str or bytes depending on what you assign to it. There is no more need for the ActixBytesWrapper struct too.
The idea is that we have a Request and Response struct which are pure Rust, and have ToPyObject and FromPyObject implementations that convert them into and from PyRequest and PyResponse that are the objects used (instead of a dict currently) on Python's side.

[sansyrox]

Thank you for the PR @AntoineRR 😄

I have some initial questions. I will complete the review soon 😄

[AntoineRR]

@sansyrox I rebased the branch and applied your recommendations 🙂

[sansyrox]

Hey @AntoineRR ,

I have one more question here. I will have give it a final review tomorrow. And then we can merge it 😄

[sansyrox]

Great work @AntoineRR 😄

This PR was intense 🔥