Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take advantage of borrowing for Database Object + Diesel #1236

Open
insipx opened this issue Nov 8, 2024 · 1 comment
Open

Take advantage of borrowing for Database Object + Diesel #1236

insipx opened this issue Nov 8, 2024 · 1 comment

Comments

@insipx
Copy link
Contributor

insipx commented Nov 8, 2024

We deal exclusively with owned database objects like StoredGroup or StoredGroupMessage. We can instead borrow it directly from what the SQLite library returns from the database, avoiding unnecessary allocations. Furthermore, when inserting into the database we can use borrowed variants of our schema to avoid extra allocations before inserting.

There are an abundance of examples around the codebase, but load_identity_updates is a fairly well self-contained example:

/// For the given list of `inbox_id`s get all updates from the network that are newer than the last known `sequence_id`,
/// write them in the db, and return the updates
#[tracing::instrument(level = "trace", skip_all)]
pub async fn load_identity_updates<ApiClient: XmtpApi>(
    api_client: &ApiClientWrapper<ApiClient>,
    conn: &DbConnection,
    inbox_ids: &[&str],
) -> Result<HashMap<String, Vec<InboxUpdate>>, ClientError> {
    if inbox_ids.is_empty() {
        return Ok(HashMap::new());
    }
    tracing::debug!("Fetching identity updates for: {:?}", inbox_ids);

    let existing_sequence_ids = conn.get_latest_sequence_id(inbox_ids)?;
    let filters: Vec<GetIdentityUpdatesV2Filter> = inbox_ids
        .iter()
        .map(|inbox_id| GetIdentityUpdatesV2Filter {
            sequence_id: existing_sequence_ids.get(*inbox_id).map(|i| *i as u64),
            inbox_id: inbox_id.to_string(),
        })
        .collect();

    let updates = api_client.get_identity_updates_v2(filters).await?;

    let to_store = updates
        .iter()
        .flat_map(|(inbox_id, updates)| {
            updates.iter().map(|update| StoredIdentityUpdate {
                inbox_id: inbox_id.clone(),
                sequence_id: update.sequence_id as i64,
                server_timestamp_ns: update.server_timestamp_ns as i64,
                payload: update.update.clone().into(),
            })
        })
        .collect::<Vec<StoredIdentityUpdate>>();

    conn.insert_or_ignore_identity_updates(&to_store)?;
    Ok(updates)
}

we could instead insert a StoredIdentityUpdateRef<'_> that borrows inbox_id, and payload

For this one example it doesn't matter much, but multiplied over everywhere we load & store in the codebase, it could have a noticeable effect on perf.

@insipx insipx added this to libxmtp Nov 8, 2024
@insipx insipx converted this from a draft issue Nov 8, 2024
@neekolas
Copy link
Contributor

neekolas commented Nov 9, 2024

Love this idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

2 participants