Description
Currently CachingSession::execute
takes impl Into<Query>
, but internally it only ever uses functions/methods that take &Query
.
I ran into this because I was trying to pool my service's allocations for Query but realized it was a waste since the clone happens anyways.
So I started by tracking the lifetime of a Query passed into CachingSession::execute
CachingSession::execute
takesimpl Into<Query>
CachingSession::execute
callsCachingSession::add_prepared_statement
, which takesimpl Into<&Query>
CachingSession::add_prepared_statement
calls.clone()
on the query when it prepares it viaSession::prepare
Session::prepare
takesimpl Into<Query>
, but internally the query is only ever used via&Arc<Connection>::prepare
, which itself takes &Query
All the way down to (and including) the SerializedRequest::make, it seems that ownership of the Query contents is unnecessary.
A bunch of allocations can be avoided by refactoring things a bit:
CachingSession::execute
does not require ownership since, as far as I can tell, there's no unconditional point in the call graph where ownership of the Query string is requiredCachingSession::add_prepared_statement
could avoid the clone when preparing the session ifSession::prepare
didn't requireimpl Into<Query>
Session::query_paged
doesn't need Query, which has implications for a whole bunch of apis triggering unnecessary allocations
There are a few options to fix this.
- Take
impl AsRef<Query>
- Take
&Query
- Refactor Query to use
Cow<str>
instead of String - Create a new type,
QueryRef
Both (1) and (2) share the same problems. They're breaking API changes and they break code like:
session.execute("myquery", &());
I imagine that the ability to provide static strings like that is pretty desirable, so I think it's fair to discount those approaches.
Approach (3) would allow Query to be generic over an owned vs unowned string. Query would become:
#[derive(Clone)]
pub struct Query<'a> {
pub(crate) config: StatementConfig,
pub contents: std::cow::Cow<'a, String>,
page_size: Option<i32>,
}
A helper method to create a Query<'static>
would probably be useful.
The main problem I see with this approach is the API breakage would permeate through every API where a Query is taken, and users who are working with Query structs directly will have to add a lifetime (although they could do so with 'static
pretty easily).
I actually think this is still the best approach despite that as it lets the caller manage the buffer.
(4) Is probably the best in terms of not having any API breakage
A new type would be created, QueryRef
,
#[derive(Clone)]
pub struct QueryRef<'a> {
pub(crate) config: StatementConfig,
pub contents: &'a str,
page_size: Option<i32>,
}
There'd be impls for AsRef<QueryRef> for Query
and From<QueryRef> for Query
.
Initially I thought this would be the ideal approach but I'm not sure.
Or something else entirely, idk.