awslabs / aws-sdk-rust

AWS SDK for the Rust Programming Language
https://awslabs.github.io/aws-sdk-rust/
Apache License 2.0
3.02k stars 249 forks source link

[request]: Accept borrowed data types as inputs #297

Open simonvandel opened 2 years ago

simonvandel commented 2 years ago

Community Note

Tell us about your request Would it be possible to accept borrowed inputs for builders? That is, instead of always making an allocation when accepting a string, make it possible for the caller to provide a borrowed type.

The allocations may seem negligible, but as a foundational library, aws-sdk-rust might as well design its APIs to be as low-cost as possible, while still being ergonomic.

Concrete example

https://docs.rs/aws-sdk-dynamodb/0.0.25-alpha/aws_sdk_dynamodb/client/fluent_builders/struct.GetItem.html#method.table_name

Every time GetItem::table_name is called, a String allocation needs to be made. If I have a &'static str it feels wasteful to incur an allocation for a String.

Tell us about the problem you're trying to solve. What are you trying to do, and why is it hard? I would like to not pay the price of allocations when that is not needed.

AFAICT there is no inherent need for the builder to own the input data. The builder only needs read-access for constructing a request to the AWS API.

Solution ideas

I'll use &str and String as examples, but also applies for other data types.

Made a small Playground link for exploring different ideas: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e61a33a1aa8b631aa6b328ea93fd6b4b

For all the new designs, the builder struct will need a lifetime parameter, which is a bit less ergonomic versus the status-quo.

Are you currently working around this issue? The current workaround is incurring the allocation cost.

rcoh commented 2 years ago

thanks for the request! This is something we've been considering, however, we haven't tackled it yet for two reasons:

  1. The impact is quite marginal considering that you're going to eventually dispatch an HTTP request over the network
  2. It can make the builders much more cumbersome to use, especially in closures since the builders now need to carry around a lifetime.

So although this is a change we're considering, it requires very careful study of the tradeoffs. If you have performance metrics demonstrating that the extra allocations current incurred are a problem, that would also be a great data point to drive prioritizations.

For other community members

If this is important to you, please upvote this issue!

neoeinstein commented 1 year ago

I'd add my support here in something at least less allocation heavy than always a String, at least for the DynamoDB SDK. I would be happy to have the API accept impl Into<Cow<'static, str>> if there was a desire to avoid a lifetime on the fluent builders, but for heavy usage, the number of repeated tiny allocations for attribute names or expressions that are just &'static str raises my antennae. You're probably right that in the grand scheme of things, with a network call on the other side, the cost isn't huge, though.