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

Use serde_json::Value is an intermediate state, and the performance of sea-orm will be lost #993

Closed
baoyachi opened this issue Aug 25, 2022 · 6 comments · Fixed by #1249
Closed
Assignees

Comments

@baoyachi
Copy link
Contributor

baoyachi commented Aug 25, 2022

Is it possible to receive sqlx's Row object directly through the object for conversion without intervening serde_json::Value. At present, serde_json::Value is an intermediate state, and the performance of sea-orm will be lost.

The code like this:
未命名文件

@billy1624
Copy link
Member

billy1624 commented Sep 1, 2022

Hey @baoyachi, thanks for the digging!

Internally, in SeaORM, we offer an into_json() method to map sqlx::Row directly to serde_json::Value

let cake: Option<serde_json::Value> = Cake::find_by_id(1)
    .into_json()
    .one(db)
    .await?;

assert_eq!(
    cake,
    Some(serde_json::json!({
        "id": 1,
        "name": "Cheese Cake"
    }))
);

https://www.sea-ql.org/SeaORM/docs/basic-crud/json/

The source code responsible for the conversion from sqlx::Row to serde_json::Value: https://github.com/SeaQL/sea-orm/blob/master/src/query/json.rs

@billy1624
Copy link
Member

I'm not sure if I answer your question. Is this a question that asking such conversion is possible or not? Or, this is a request for optimization of existing code? If so, could you point out the source code that needs to be refactor?

@main--
Copy link

main-- commented Oct 5, 2022

I'm pretty sure I understand what @baoyachi means here (I found this issue through google).

pub trait TryGetableFromJson: Sized
where
for<'de> Self: serde::Deserialize<'de>,
{
/// Ensure the type implements this method
#[allow(unused_variables, unreachable_code)]
fn try_get_from_json(res: &QueryResult, pre: &str, col: &str) -> Result<Self, TryGetError> {
let column = format!("{}{}", pre, col);
let res: Result<_, _> = match &res.row {
#[cfg(feature = "sqlx-mysql")]
QueryResultRow::SqlxMySql(row) => {
use sqlx::Row;
row.try_get::<Option<serde_json::Value>, _>(column.as_str())
.map_err(|e| TryGetError::DbErr(crate::sqlx_error_to_query_err(e)))
.and_then(|opt| opt.ok_or(TryGetError::Null(column)))
}
#[cfg(feature = "sqlx-postgres")]
QueryResultRow::SqlxPostgres(row) => {
use sqlx::Row;
row.try_get::<Option<serde_json::Value>, _>(column.as_str())
.map_err(|e| TryGetError::DbErr(crate::sqlx_error_to_query_err(e)))
.and_then(|opt| opt.ok_or(TryGetError::Null(column)))
}
#[cfg(feature = "sqlx-sqlite")]
QueryResultRow::SqlxSqlite(row) => {
use sqlx::Row;
row.try_get::<Option<serde_json::Value>, _>(column.as_str())
.map_err(|e| TryGetError::DbErr(crate::sqlx_error_to_query_err(e)))
.and_then(|opt| opt.ok_or(TryGetError::Null(column)))
}
#[cfg(feature = "mock")]
QueryResultRow::Mock(row) => {
row.try_get::<serde_json::Value>(column.as_str())
.map_err(|e| {
debug_print!("{:#?}", e.to_string());
TryGetError::Null(column)
})
}
#[allow(unreachable_patterns)]
_ => unreachable!(),
};
res.and_then(|json| {
serde_json::from_value(json).map_err(|e| TryGetError::DbErr(DbErr::Json(e.to_string())))
})
}

The implementation of TryGetableFromJson always gets the value as Option<serde_json::Value> first, and then invokes serde_json::from_value. This is not optimal. Instead, it would be better to directly get Option<sqlx::types::Json<Self>> from the database and just unpack it. The temporary serde_json::Value is unnecessary.

For example, if I deserialize struct Foo { a: i32, b: i32 }, then serde-json could directly produce this as a plain 8-bytes value. However, deserializing it as serde_json::Value first causes String allocations for "a" and "b" as well as a BTreeMap or IndexMap to store the object. This is clearly not ideal for performance.

@baoyachi
Copy link
Contributor Author

baoyachi commented Oct 8, 2022

@main-- you are right

@billy1624
Copy link
Member

Hey @baoyachi, thanks for the clarification! Now I see what @baoyachi want to say. Please check #1249

@billy1624 billy1624 self-assigned this Nov 24, 2022
@baoyachi
Copy link
Contributor Author

Hey @baoyachi, thanks for the clarification! Now I see what @baoyachi want to say. Please check #1249

@billy1624 How much performance will be improved? Is there a numerical result comparison?

@billy1624 billy1624 moved this from Triage to Done in SeaQL Dev Tracker Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants