Add base REST Client and CLI setup.#6
Add base REST Client and CLI setup.#6G8XSU merged 10 commits intolightningdevkit:mainfrom G8XSU:cli-base-setup
Conversation
G8XSU
commented
Sep 6, 2024
- Add base REST client setup.
- Add base cli setup.
- Add initial set of api methods to rest-client.
| async fn post_request<Rq: Message, Rs: Message + Default>( | ||
| &self, request: &Rq, url: &str, | ||
| ) -> Result<Rs, LdkNodeServerError> { | ||
| let request_body = request.encode_to_vec(); |
There was a problem hiding this comment.
Instead of allocating a Vec, should we instead use something like async_prost (https://docs.rs/async-prost/latest/async_prost/) or prost_stream (https://docs.rs/prost-stream/latest/prost_stream/) with Body::wrap_stream (https://docs.rs/reqwest/latest/reqwest/struct.Body.html#method.wrap_stream)? Not sure how reliable those are or if we want to take on the dependencies, though I suppose we could implement TryStream ourselves.
There was a problem hiding this comment.
Both of libs aren't maintained at all, prost is maintained by tokio.
Imo, we should stick with the popular and well-used lib for now and piggyback on them improving performance for having future optimizations like this.
Since both encode and decode are internal details, we can change them at any time without any breakage, so i am not too worried about this optimization for now.
Also, currently our messages have limited size and don't really contain any big blobs, so its upside is limited.
-
However it did motivate me to look into some other optimization which is to replace all vec with Bytes, which is easily doable and could avoid some vec clones at some of the places.
-
While in decode, prost has special handling for ref-counted byte buffers, it will not clone them, will return a ref to slice instead. Reqwest already returns ref-counted
Bytesas response. -
Also, I am not sure how much of improvement it is if any, we can send reqwest a Bytes object instead of vec and it can create body from that. In the current version of reqwest that we are using, stream trait for body isn't stable iiuc.
It mentions "// TheStreamtrait isn't stable, so the impl isn't public.".
There was a problem hiding this comment.
Both of libs aren't maintained at all, prost is maintained by tokio. Imo, we should stick with the popular and well-used lib for now and piggyback on them improving performance for having future optimizations like this. Since both encode and decode are internal details, we can change them at any time without any breakage, so i am not too worried about this optimization for now. Also, currently our messages have limited size and don't really contain any big blobs, so its upside is limited.
Fair enough! Thanks for investigating though.
- However it did motivate me to look into some other optimization which is to replace all vec with Bytes, which is easily doable and could avoid some vec clones at some of the places.
Which clones are avoided? Or are you saying in another PR?
- While in decode, prost has special handling for ref-counted byte buffers, it will not clone them, will return a ref to slice instead. Reqwest already returns ref-counted
Bytesas response.
Not sure how this is relevant. Aren't we talking about encoding objects that we've created in memory?
- Also, I am not sure how much of improvement it is if any, we can send reqwest a Bytes object instead of vec and it can create body from that.
Isn't that just a wrapper around a Vec in how we are using it? Nothing needs to be shared, IIUC.
In the current version of reqwest that we are using, stream trait for body isn't stable iiuc.
It mentions "// TheStreamtrait isn't stable, so the impl isn't public.".
Hmm... I see it as an optional feature not unstable.
There was a problem hiding this comment.
Which clones are avoided? Or are you saying in another PR?
Yes, mostly while creating these proto objects if we already have access to a byte slice or buffer.
Not sure how this is relevant. Aren't we talking about encoding objects that we've created in memory?
Reqwest returns Bytes, it will read raw bytes from wire and place them on buffer. if we change generated protos to use Bytes, it can refer to a slice from that buffer, whereas if we use vec it has to clone that part of memory and allocate while converting from slice iiuc.
Isn't that just a wrapper around a Vec in how we are using it? Nothing needs to be shared, IIUC.
Yeah this doesn't change much of anything. It is mostly some minor optimizations that bytes might do internally like buffer allocation/reuse or optimizing storage of small Bytes objects.
Hmm... I see it as an optional feature not unstable.
It was mentioned in code: https://github.com/seanmonstar/reqwest/blame/cf69fd4bfe22855d576497eb94e9eb549e742475/src/async_impl/body.rs#L22
Iiuc, support for streaming as public api was only exposed last week: seanmonstar/reqwest#2255
| async fn post_request<Rq: Message, Rs: Message + Default>( | ||
| &self, request: &Rq, url: &str, | ||
| ) -> Result<Rs, LdkNodeServerError> { | ||
| let request_body = request.encode_to_vec(); |
There was a problem hiding this comment.
Both of libs aren't maintained at all, prost is maintained by tokio. Imo, we should stick with the popular and well-used lib for now and piggyback on them improving performance for having future optimizations like this. Since both encode and decode are internal details, we can change them at any time without any breakage, so i am not too worried about this optimization for now. Also, currently our messages have limited size and don't really contain any big blobs, so its upside is limited.
Fair enough! Thanks for investigating though.
- However it did motivate me to look into some other optimization which is to replace all vec with Bytes, which is easily doable and could avoid some vec clones at some of the places.
Which clones are avoided? Or are you saying in another PR?
- While in decode, prost has special handling for ref-counted byte buffers, it will not clone them, will return a ref to slice instead. Reqwest already returns ref-counted
Bytesas response.
Not sure how this is relevant. Aren't we talking about encoding objects that we've created in memory?
- Also, I am not sure how much of improvement it is if any, we can send reqwest a Bytes object instead of vec and it can create body from that.
Isn't that just a wrapper around a Vec in how we are using it? Nothing needs to be shared, IIUC.
In the current version of reqwest that we are using, stream trait for body isn't stable iiuc.
It mentions "// TheStreamtrait isn't stable, so the impl isn't public.".
Hmm... I see it as an optional feature not unstable.
|
Rebased on main and squashed. |