Skip to content

Add base REST Client and CLI setup.#6

Merged
G8XSU merged 10 commits intolightningdevkit:mainfrom
G8XSU:cli-base-setup
Sep 12, 2024
Merged

Add base REST Client and CLI setup.#6
G8XSU merged 10 commits intolightningdevkit:mainfrom
G8XSU:cli-base-setup

Conversation

@G8XSU
Copy link
Copy Markdown
Contributor

@G8XSU G8XSU commented Sep 6, 2024

  • Add base REST client setup.
  • Add base cli setup.
  • Add initial set of api methods to rest-client.

@G8XSU G8XSU changed the title [Draft PR] Add base REST Client and CLI setup. Add base REST Client and CLI setup. Sep 10, 2024
@G8XSU G8XSU requested a review from jkczyz September 10, 2024 22:12
async fn post_request<Rq: Message, Rs: Message + Default>(
&self, request: &Rq, url: &str,
) -> Result<Rs, LdkNodeServerError> {
let request_body = request.encode_to_vec();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Bytes as 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 "// The Stream trait isn't stable, so the impl isn't public.".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Bytes as 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 "// The Stream trait isn't stable, so the impl isn't public.".

Hmm... I see it as an optional feature not unstable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Bytes as 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 "// The Stream trait isn't stable, so the impl isn't public.".

Hmm... I see it as an optional feature not unstable.

@G8XSU G8XSU requested a review from jkczyz September 12, 2024 17:58
Copy link
Copy Markdown
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash.

@G8XSU
Copy link
Copy Markdown
Contributor Author

G8XSU commented Sep 12, 2024

Rebased on main and squashed.

@G8XSU G8XSU merged commit 06f986c into lightningdevkit:main Sep 12, 2024
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.

3 participants