Various improvements around the index method #318
Conversation
✅ Deploy Preview for robyn canceled.
|
|
Thank you for the PR @AntoineRR 😄 Looks great at the first glance. I will give it a thorough review by tomorrow evening. 😄 |
| request.insert("params", route_params.to_object(py)); | ||
| request.insert("queries", queries.to_object(py)); |
There was a problem hiding this comment.
@AntoineRR , I have one question - What benefit does to_object bring over into_py?
There was a problem hiding this comment.
From the docs of pyo3 here:
ToPyObject is a conversion trait that allows various objects to be converted into PyObject. IntoPy serves the same purpose, except that it consumes self.
I used to_object because it doesn't consume self. This means I was able to use references instead of taking ownership of the object, and I didn't need to clone the object to use it twice in the code.
There was a problem hiding this comment.
Ah, alrighty. Thank you! :D
I have a feeling that this will result in some performance increase!
There was a problem hiding this comment.
It may lead to performance increase, but I am not sure about it because the Rust compiler is smart enough to remove unnecessary clone calls most of the time ^^
Btw how do you test the performances of Robyn ? For testing the Rust code it is possible to write benchmarks using the criterion crate, I could try to do it if you are interested :)
There was a problem hiding this comment.
@AntoineRR , I just compile it and run it through oha. (https://github.com/hatoo/oha)
I had a look at criterion before but was unable to figure out how would I bench the requests. What approach would you take for this?
There was a problem hiding this comment.
Ok, I used oha to run some performance tests too, and saw no difference on my machine sadly :( But I think it still makes the code more readable ^^
To benchmark with criterion, we would have to first add routes to the routers, then generate a few requests we should test, and I guess we could benchmark the index method synchronously by sending all the requests to it? If there is enough variations in the requests it would be representative of the average speed of the Rust core of Robyn.
There was a problem hiding this comment.
@AntoineRR , that sounds complicated. What do you think about python/bash script that runs a dummy app and runs oha on it.
Your approach will give us more granular results, but then I have a question how much of it can we actually control? As we use pyo3 and then the next steps would be writing unsafe CFFI ourselves.
Let me know what you think of it. And feel free to correct me if I am wrong or very wrong 😆
There was a problem hiding this comment.
Adding a python or bash script to run performance tests on Robyn as a whole would be a good idea I think, it would let us easily check if a change in the code has an impact on performances!
I don't think it would require that much work to write a benchmark with criterion, but my pyo3 knowledge is quite limited for now. I may try it and submit a PR if I find it useful enough tho ^^
For now a script running oha is surely good enough, and easier to setup! I can open an issue for this if you are interested
There was a problem hiding this comment.
For now a script running oha is surely good enough, and easier to setup! I can open an issue for this if you are interested
Yes, please. Thank you! :D
I don't think it would require that much work to write a benchmark with criterion, but my pyo3 knowledge is quite limited for now. I may try it and submit a PR if I find it useful enough tho ^^
Perfect. Do lmk if I can be of any help here. :D
There was a problem hiding this comment.
Thanks, I'll let you know if I got something to work with criterion ;)
Here is the issue for a follow up on the oha benchmark: #320
Description
Hello, I'm back with a new suggestion for the Rust code of Robyn.
I tried to simplify the code around the
indexfunction inserver.rs:Rc<RefCell<>>clone()calls (this is mostly possible due to the use of theto_objectmethod, which takes a reference to the object, instead of theinto_pymethod which takes ownership of the object)Let me know if this is ok for you 🙂