Skip to content
New issue

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

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

Already on GitHub? # to your account

Create BytesMut for Json with initial capacity #1196

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

ryanfowler
Copy link
Contributor

This PR initializes the BytesMut for serializing Json with a small initial capacity to reduce allocations as the inner Vec<u8> grows.

A recent PR removed an unnecessary copy by introducing BytesMut and switching away from serde_json::to_vec, but I noticed a small regression that I believe is due to serde_json::to_vec creating a Vec<u8> with an initial capacity (code). I figured matching the initial capacity of "128 bytes" made sense. In a simple benchmark like below (mostly pulled from this comment), I'm seeing a modest ~1% speed bump.

use axum::{routing::get, Json, Router};

#[tokio::main]
async fn main() {
    let app = Router::new().route("/json", get(json));

    axum::Server::bind(&"0.0.0.0:8000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

async fn json() -> Json<Message> {
    let message = Message {
        name: "http-benchmark",
        version: "v0.1.0",
        message: "Hello, World!",
    };
    Json(message)
}

#[derive(serde::Serialize)]
pub struct Message {
    pub name: &'static str,
    pub version: &'static str,
    pub message: &'static str,
}

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

actix-web uses an initial capacity of 8192 bytes. I'm wondering if we should benchmark a slightly bigger initial capacity as well 🤔

@ryanfowler
Copy link
Contributor Author

actix-web uses an initial capacity of 8192 bytes. I'm wondering if we should benchmark a slightly bigger initial capacity as well 🤔

Oh, interesting! I suppose it completely depends on the size of JSON being serialized. I would guess that JSON responses in real-world servers are usually >128 bytes...

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, 128 seems like a good initial capacity to me but 8k seems a little much.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep its cool with me as well. We can always tweak later.

@davidpdrsn davidpdrsn enabled auto-merge (squash) July 26, 2022 13:01
auto-merge was automatically disabled July 26, 2022 13:02

Head branch was pushed to by a user without write access

@davidpdrsn davidpdrsn enabled auto-merge (squash) July 26, 2022 13:33
@ryanfowler
Copy link
Contributor Author

Hmm it appears that the required checks for the main branch may need to be updated for this naming change made to CI

@davidpdrsn davidpdrsn disabled auto-merge July 26, 2022 14:45
@davidpdrsn davidpdrsn merged commit 7e7a2f2 into tokio-rs:main Jul 26, 2022
@davidpdrsn
Copy link
Member

Yeah I'll do that separately.

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants