Rust: ideas for reducing allocations in setting options
- Dominant language
- C#
- Stars
- 627
- Forks
- 217
- Avg merge
- 17h
- Merged PRs (30d)
- 57
Description
### What feature or improvement would you like to see?
In #4413 I lamented that passing booleans through the current API causes a lot of unnecessary allocations because they have to be converted to `String` to be passed as `OptionValue::String`.
For the same reason, FFI _must_ unconditionally copy string and binary option values to a new allocation because the API doesn't allow passing a borrowed string/slice.
# Backwards-Compatible: Add provided methods to `Optionable`
Add a new provided methods to `Optionable` that take borrowed data:
```rust
pub trait Optionable {
// ...
/// Set an option by key from a borrowed string.
///
/// This should be called when converting to [`OptionValue::String`] would require copying to a new `String`,
/// as the implementation may be able to avoid it, e.g. if parsing as a `bool` anyway.
///
/// Implementors are **recommended but not required** to override this method if they can provide
/// a more efficient implementation. Forwards to [`Self::set_option()`] by default.
fn set_option_str(&self, key: Self::Option, value: &str) -> Result<()> {
self.set_option(key, value.into())
}
/// Set an option by key from a borrowed slice.
///
/// This should be called when converting to [`OptionValue::Bytes`] would require copying to a new `Vec`,
/// as the implementation may be able to avoid it, e.g. if parsing to another type anyway.
///
/// Implementors are **recommended but not required** to override this method if they can provide
/// a more efficient implementation. Forwards to [`Self::set_option()`] by default.
fn set_option_bytes(&self, key: Self::Option, value: &[u8]) -> Result<()> {
self.set_option(key, value.into())
}
}
```
The FFI could trivially be updated to call these methods directly, which gives implementations the possibility of bypassing the allocation if they don't need it. It's a backwards-compatible addition because it falls back to the existing
required method, so implementors don't _need_ to be aware of it.
However, there's another possible source of unnecessary allocations here that bothers me, which we could eliminate if we allowed a _small_ breaking change that's unlikely to affect most downstream crates.
# Minimally Breaking: Allow Borrowed Keys in `Optionable`
For driver-specific keys, `OptionDatabase`, `OptionConnection` and `OptionStatement` all have to convert the key to an owned `String` which is another source of generally unnecessary allocations, as the key strings are often going to be hardcoded on both sides of the API.
We can adapt the above approach to also take a borrowed string for the _key_, so that driver-specific keys don't need to be copied to a new allocation:
```rust
pub trait Optionable {
// ...
fn set_option_str(&self, key: &str, value: &str) -> Result<()>;
fn set_option_bytes(&self, key: &str, value: &[u8]) -> Result<()>;
}
```
However, this cannot be a provided method, because the `Optionable::Option` associated type only requires `AsRef`, so there's no generic way for a provided method to convert `&str` to `Self::Option`.
So the possibilities are to either:
* Add these as provided methods returning `Status::NotImplemented`.
* Backwards-compatible, but very suboptimal because downstream consumers may not know if these methods are not implemented beforehand, so they risk encountering errors at runtime or having to check for the error and implement the fallback themselves.
* Add these as required methods.
* Breaking change for all implementors of `Optionable`, but loudly signals new methods.
* Add a `From<&str>` bound to `Optionable::Option`.
* Breaking change, but **unlikely** to affect downstream consumers.
The last option is the most interesting, because all the existing option types _already_ implement `From<&str>`.
It's actually only a breaking change because the `Optionable` trait is not limited to the pre-defined option types, and _theoretically_ a downstream consumer could be using a completely different type for `Self::Option` that does not implement `From<&str>`. This seems incredibly unlikely in practice, however, because there is no reason to do so: the need to implement `Optionable` is driven by the rest of the API, which also forces the pre-defined `Option*` types to be used.
So even though this is--strictly speaking--a breaking change, it would require **zero** code changes for basically all existing implementations.
We can then add provided methods to `Optionable`:
```rust
pub trait Optionable {
// ...
fn set_option_str(&self, key: &str, value: &str) -> Result<()> {
self.set_option(key.into(), value.into())
}
fn set_option_bytes(&self, key: &str, value: &[u8]) -> Result<()> {
self.set_option(key.into(), value.into())
}
}
````
So drivers can override this when the implementation doesn't need an owned `String` for the key *or* value.
In addition to this change, I would add a method like the following to all the `Option*` types:
```rust
impl OptionDatabase {
/// Parse `key` as one of the pre-defined database options, ignoring unknown or custom keys.
pub fn try_predefined(key: &str) -> Option {
match key {
ADBC_OPTION_URI => Some(Self::Uri),
ADBC_OPTION_USERNAME => Some(Self::Username),
ADBC_OPTION_PASSWORD => Some(Self::Password),
_ => None,
}
}
}
```
This serves as a convenient building block that allows drivers to parse `Option*` types while bypassing the allocation for custom keys. I chose to return `Option` instead of `Result` to avoid allocating `message: String` in `Error`, and because it's not strictly an error as it's up to the caller to decide whether the key is actually invalid or not.
Contributor guide
Research direction
Start at the Optionable trait and the OptionDatabase, OptionConnection, and OptionStatement types named in the issue; inspect their current key/value conversions and existing From<&str> implementations. Compare the proposed provided methods and key-bound choices with current implementors and FFI call sites. Done means a selected API direction is agreed and the affected option paths can avoid unnecessary key and value allocations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design, performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100