Skip to content

hyper: refactor, update Rust, Hyper, and deps #9720

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

Merged
merged 5 commits into from
Mar 24, 2025

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Mar 23, 2025

This is a fairly major refactoring of the hyper tests

  • Update Rust to 1.85.0
  • Update hyper to 1.6.0
  • Use socket2 instead of net2
  • Bump other dependencies
  • Remove hyper-db test. This is now provided as a route in the main test.
  • Add a new test using tokio multi-thread runtime to compare with the
    single-thread runtime. This uses the same docker image, but passes a
    command line argument to the server to use the multi-thread runtime.
  • Add a healthcheck /ping url to the dockerfile to ensure that the
    server is running before running the tests.
  • Use deadpool-postgres for connection pooling.
  • Use cached prepared statements for the queries.
  • Factor each test into its own module.
  • Simplify the previous approach that used closures into more a more
    straightforward approach that uses linear functions.
  • Add single query and multiple query tests.

- Update Rust to 1.85.0
- Update hyper to 1.6.0
- Use socket2 instead of net2
- Bump other dependencies
- Remove hyper-db test. This is now provided as a route in the main test.
- Add a new test using tokio multi-thread runtime to compare with the
  single-thread runtime. This uses the same docker image, but passes a
  command line argument to the server to use the multi-thread runtime.
- Add a healthcheck /ping url to the dockerfile to ensure that the
  server is running before running the tests.
- Use deadpool-postgres for connection pooling.
- Use cached prepared statements for the queries.
- Factor each test into its own module.
- Simplify the previous approach that used closures into more a more
  straightforward approach that uses linear functions.
- Add single query and multiple query tests.
Copy link
Contributor Author

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Self review notes to assist reviewers

@@ -0,0 +1 @@
target/
Copy link
Contributor Author

@joshka joshka Mar 23, 2025

Choose a reason for hiding this comment

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

This makes it no longer necessary to run cargo clean in the dockerfile

Comment on lines -11 to -18
[[bin]]
name = "hyper-techempower"
path = "src/main.rs"

[[bin]]
name = "hyper-db-techempower"
path = "src/main_db.rs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's just one executable now

# Disable default runtime, so that tokio single thread can be used instead.
hyper = { version = "0.14", features = ["server", "http1"], default-features = false }
hyper = { version = "1.6.0", features = [
Copy link
Contributor Author

@joshka joshka Mar 23, 2025

Choose a reason for hiding this comment

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

Dependencies use specific x.y.z versions instead of x.y versions. This is intentional as it means that the specific versions in use will be more obvious to the end user. Additionally the docker file now specifies --locked when building, which will keep the versions in use as they are specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dependencies use specific x.y.z versions instead of x.y versions. This is intentional as it means that the specific versions in use will be more obvious to the end user.

This is equivalent to "^1.6.0" and not "=1.6.0", by the way.

That said, the lockfile should keep things pinned to what you have in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm geeking out that Steve Klabnik is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dependencies use specific x.y.z versions instead of x.y versions. This is intentional as it means that the specific versions in use will be more obvious to the end user.

This is equivalent to "^1.6.0" and not "=1.6.0", by the way.

That said, the lockfile should keep things pinned to what you have in this PR.

Yup. I understand that. This is more from the perspective of users who tend to assume the latter. :)

serde_json = "1.0.140"
socket2 = { version = "0.5.8", features = ["all"] }
strum = { version = "0.27.1", features = ["derive"] }
thiserror = "2.0.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snafu might be more appropriate for the places where the errors crop up, but this will do for now.

Comment on lines +46 to +47
tracing = "0.1.41"
tracing-subscriber = "0.3.19"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including this for some nicer logging of setup information during development. This is not intended to generally be called at runtime during benchmarks.

Comment on lines +133 to +135
// spawn accept loop into a task so it is scheduled on the runtime with all the other tasks.
let accept_loop = accept_loop(handle.clone(), listener);
handle.spawn(accept_loop).await.unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO - check the perf impact of doing this vs not.

let service = service_fn(router);
loop {
let (stream, _) = listener.accept().await?;
let http = http.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO It could be worth comparing this to just creating a new builder here. Unsure if there's any impact

Comment on lines +172 to +174
// ignore errors until https://github.com/hyperium/hyper/pull/3863/ is merged
// This PR will allow us to filter out shutdown errors which are expected.
// warn!("Connection error (this may be normal during shutdown): {e}");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this occurs when the benchmarking tool just drops the connections.

///
/// This handler is used to verify that the server is running and can respond to requests. It is
/// used by the docker health check command.
fn ping() -> Result<Response<BoxBody<Bytes, Infallible>>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an incorrectly configured test and was struggling to get the benchmarks to properly connect to that one test. I wanted a way to ensure that the server was known to be up outside of the tests themselves, so added this to support the docker healthchecks.

(The underlying problem that I was hitting was caused by me returning an overly large static value for content length from the db test - 32 instead of actually calculating the length. This causes the test framework to not be able to connect to that test and also to not report which test is failing, only that it can't connect)

Comment on lines +16 to +29
#[derive(Debug, Serialize)]
pub struct World {
id: i32,
randomnumber: i32,
}

impl From<Row> for World {
fn from(row: Row) -> Self {
World {
id: row.get(0),
randomnumber: row.get(1),
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is duplicated in the two db tests. I'm mostly fine with that here as it makes the tests more self-contained when reading them for understanding, and allows for future tweaks if the tests diverge.

@joshka
Copy link
Contributor Author

joshka commented Mar 23, 2025

Pinging @seanmonstar @steveklabnik @polachok for review as authors mentioned in the cargo.toml.

@joshka
Copy link
Contributor Author

joshka commented Mar 24, 2025

Removed precomputed headers due to #5201

@msmith-techempower msmith-techempower merged commit 2875eaf into TechEmpower:master Mar 24, 2025
3 checks passed
@joshka
Copy link
Contributor Author

joshka commented Mar 24, 2025

I was a bit surprised to see this merged without a bit more review, but I guess we can see the results and work from there.
I fully anticipate that this might have some performance changes as my focus was more on making this more maintainable than it was on the pure speed aspect of things.

I suspect that the change from tokio-postgres to slapping a pool in front of it may cause a performance degredation based on #8790 (comment) (and other parts of the same issue).

All that said, it's possible that this PR provides a good enough base to add variations on top of like choice of ORM / pool etc.

@joshka joshka deleted the jm/hyper-update branch March 24, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants