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

fix: allow response headers and fix headers not working in const requests #331

Merged
merged 5 commits into from
Dec 14, 2022

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Dec 8, 2022

Description
fix: allow response headers and fix headers not working in const requests
This PR fixes #325 , #323

@netlify
Copy link

netlify bot commented Dec 8, 2022

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 8e5cee7
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63987b83dc146c0008ffd1db

@sansyrox sansyrox marked this pull request as ready for review December 8, 2022 21:18
@sansyrox
Copy link
Member Author

@AntoineRR @cirospaciari , do you folks want to give it a final green light before we release a new version?

I really appreciate all your help 😄

@sansyrox sansyrox changed the title fix: allow response headers fix: allow response headers and fix headers not working in const requests Dec 13, 2022
@cirospaciari
Copy link

cirospaciari commented Dec 13, 2022

@AntoineRR @cirospaciari , do you folks want to give it a final green light before we release a new version?

I really appreciate all your help smile

I think will work.
But my issue is with jsonify basically this is serializing and deserializing the headers without any need, in CPython this is really slow, even if Robyn can get all raw power of actix web (3.5 mi req/s in TechEmPower) will be limited to JSON serializer performance, the better JSON serializer performs about 1.4 mi req/s in TechEmPower, also is adding a lot of CPU usage, copying, and dynamic allocations. Rewriting now will be better than adding this to the main branch and causing incompatibility issues in the future.

JSON data:
https://www.techempower.com/benchmarks/#section=test&runid=124fd2af-3030-4315-8876-e1db1fa91193&test=json

Plaintext data:
https://www.techempower.com/benchmarks/#section=test&runid=124fd2af-3030-4315-8876-e1db1fa91193&test=plaintext

Take a look in:
https://docs.rs/pyo3/0.2.7/pyo3/struct.PyDict.html
And in:
https://pyo3.rs/main/doc/pyo3/types/struct.pytuple

@AntoineRR
Copy link
Collaborator

I agree with @cirospaciari, we shouldn't use jsonify for the headers.
Although I think it is already handled like this in Robyn currently (see this test route from the main branch for example) so there will already be an incompatibility issue. Can you confirm this @sansyrox ?
Also, refactoring to allow something else than a string may be a big refactor for issue #336, maybe it would be good to merge this and start a new PR to remove jsonify ?

@cirospaciari
Copy link

I agree with @cirospaciari, we shouldn't use jsonify for the headers.
Although I think it is already handled like this in Robyn currently (see this test route from the main branch for example) so there will already be an incompatibility issue. Can you confirm this @sansyrox ?
Also, refactoring to allow something else than a string may be a big refactor for issue #336, maybe it would be good to merge this and start a new PR to remove jsonify ?

If already will be an incompatible issue so, merging and fix in a new PR is acceptable, but refactoring should be priority, I need to benchmark it first but the performance penalty will be huge without refactoring.

@sansyrox
Copy link
Member Author

but refactoring should be priority, I need to benchmark it first but the performance penalty will be huge without refactoring.

Agreed.

Although I think it is already handled like this in Robyn currently (see this test route from the main branch for example) so there will already be an incompatibility issue.

Yep, yep.

Also, refactoring to allow something else than a string may be a big refactor for issue #336, maybe it would be good to merge this and start a new PR to remove jsonify ?

I have been working this for a few hours now. It is indeed a big refactor. :(

I think it makes sense to merge this one and refactor based on a priority.

@sansyrox sansyrox merged commit 61942c6 into main Dec 14, 2022
@sansyrox sansyrox deleted the fix/headers branch April 9, 2023 05:22
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 this pull request may close these issues.

[Feature Request] Allow the ability of sending the headers from the same route
3 participants