refactor: use cfg-aliases for feature flags #3865
Conversation
Introduces aliases for each feature flag as well as for the main combinations of http version and client/server roles. This makes it significantly easier to understand which feature flags are required on any given line as the aliases require less parsing of nested `cfg` expressions.
|
It does look cleaner, true! I'd rather not take a build-dependency, though. I tried to make things easier with a few cfg_blah macros, but they didn't work out well in practice. |
My personal calculus would be to rely on a well used crate like this (69M downloads on 6 versions, vs hyper's 278M on 244 versions), but I can see why you might want to avoid that in hyper. I will expand the macros into their effect. It's mostly just a feature gated println.
I think there's some useful ideas there in grouping code and adding doc comments, but using doc_auto_cfg is often a bit simpler unless there's specific situations where that doesn't work. |
| #[cfg(feature = "runtime")] | ||
| #[cfg(runtime)] |
There was a problem hiding this comment.
Doing some more digging, this feature doesn't exist in the cargo.toml. Should it be removed from the source or added to the features list?
|
I should have said more! Not just a build-dependency, but the build script. There's a light positive pressure generally to avoid them unless building something, because they slow down compiler parallelism. |
That sounds like a good general rule to follow, but also one which leads to negative outcomes if followed without thinking (let's not fall into a "cargo" cult mentality here). Can we measure the impact of this change on build times perhaps? I would intuitively expect that it's so negligible as to be meaningless as a blocker, but I'm often surprised when such assumptions are false. How would you confirm that that? Do you perhaps have a very low CPU system that you can point at that would show the difference like a Raspberry Pi 1 or similar? The difference in compilation times (Macbook M2) are about 150ms, which puts it in a range where it wouldn't worry me generally. I'd make the tradeoff for simplifying code like this for +150ms build in just about any situation. Do you have a threshold that differs? ~/local/hyper on master
❯ time (touch src/lib.rs && cargo b)
Compiling hyper v1.6.0 (/Users/joshka/local/hyper)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s
( touch src/lib.rs && cargo b; ) 0.09s user 0.12s system 115% cpu 0.186 total
~/local/hyper on jm/cfg-aliases
❯ time (touch src/lib.rs && cargo b)
Compiling hyper v1.6.0 (/Users/joshka/local/hyper)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.30s
( touch src/lib.rs && cargo b; ) 0.10s user 0.13s system 65% cpu 0.350 total |
|
That's the wrong benchmark: it's measuring (1) incremental builds (2) with a no-op change to the library itself but none to the build script, but (3) unnecessarily including the cost of cargo asking |
|
Can you provide something that shows the concrete impact of this problem? This is doing a full rebuild of hyper directly, and I'll add one where hyper is part of a dependency chain (if you've got a particular project you can point at where it's deep in the chain that would be great). with Edit: I picked tokio-console as it uses hyper via tonic -> axum -> reqwest -> hyper (and a variety of other paths) This shows that this presents a measurable, but not noticeable at a human scale time difference. When is that meaningful? Serious question as I don't know what downstream concerns would be affected by this. |
|
Your diff doesn't replace hyper in the dependency tree of tokio-console, it adds the local version on top (you can verify this with diff --git a/Cargo.lock b/Cargo.lock
index bc83a65..8daee0f 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -821,9 +821,8 @@ dependencies = [
[[package]]
name = "hyper"
-version = "1.5.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bbbff0a806a4728c99295b254c8838933b5b082d75e3cb70c8dab21fdfbcfa9a"
+version = "1.6.0"
+source = "git+https://github.com/joshka/hyper.git?rev=83488c120d10dfd5a92aa5f6ff7f92355a9ebba1#83488c120d10dfd5a92aa5f6ff7f92355a9ebba1"
dependencies = [
"bytes",
"futures-channel",
diff --git a/Cargo.toml b/Cargo.toml
index 1eb3e8a..4b9ff78 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -32,3 +32,6 @@ install-updater = false
[profile.dist]
inherits = "release"
lto = "thin"
+
+[patch.crates-io]
+hyper = { git = "https://github.com/joshka/hyper.git", rev = "83488c120d10dfd5a92aa5f6ff7f92355a9ebba1" }With this change, I get these numbers (your branch first, hyper 1.5.0 from crates.io second): Is this significant? It's probably hard for a human to notice because 38 seconds is way outside "I sit there watching the progress bar fill up" territory. On the other hand, if you multiply the CPU time by how many times hyper is compiled every month, it doesn't seem negligible any more. (The Rust ecosystem in general scores pretty terribly on this metric, but that's no reason to ignore it entirely.) Note that this computer has a CPU from 2017 (Intel i7-6700K) but it was high end back then and still works perfectly fine for most things -- definitely better than the newer laptop I sometimes use for coding while traveling. That aside, tokio-console has a relatively large dependency graph (245 "units" in [dependencies]
bytes = "1.8.0"
http-body-util = "0.1.2"
hyper = { version = "1.5.0", features = ["http1", "server"] }
hyper-util = { version = "0.1.10", features = ["tokio"] }
tokio = { version = "1.41.1", features = ["net", "rt-multi-thread"] }... but unfortunately your branch doesn't compile with that feature set: Regardless, if the absolute impact were to be similar to the effect I saw in tokio-console, I'd consider that noteworthy on a project that otherwise builds in 4 seconds. In this example it probably won't have the same wall time impact because there's a lot of time where everything waits on tokio and there's spare CPU cores to use for a small build script. But I have other projects (not public) that build in 10 seconds or so where I might use hyper in the future which have more dependencies and more than enough work for all my CPU cores during most of the build. |
src/error.rs
Outdated
| #[cfg(all(feature = "client", any(feature = "http1", feature = "http2")))] | ||
| #[cfg(any(http1_client, http1_server))] |
There was a problem hiding this comment.
This is wrong, it should be http_client here.
Looks like I failed on a boolean expansion somewhere (which is a pain because if I've missed one then all the expansions are now suspect). Mostly I did these by regex replacement, but it's entirely possible that I missed a bracket somewhere and broke things. If this was to be considered for merge, I'd do a comprehensive audit of the changes to ensure that this is correct. |
|
Ugh on not noticing that it was using 1.5 there. Apologies.
I fixed just the buggy enum that you mentioned (noting that I'm not sure if this problem is more widespread - I'd have to check it out fully and redo the conversions to be sure) Using the hello-world example from axum source code (@ latest commit as of now af6a595f) diff --git i/Cargo.toml w/Cargo.toml
index cfbfb615..de75834b 100644
--- i/Cargo.toml
+++ w/Cargo.toml
@@ -53,3 +53,6 @@ verbose_file_reads = "warn"
# These have been fixed in the past, but are still present in the changelog.
DefaultOnFailedUpdgrade = "DefaultOnFailedUpdgrade"
OnFailedUpdgrade = "OnFailedUpdgrade"
+
+[patch.crates-io]
+hyper = { version = "1.6.0", path = "../hyper" }
I think it would be worth setting an actual target of what's good enough performance for build rather than this being some ambiguous idea. Without this everyone's tradeoffs are invisible constraints that are impossible to meet. I'm running a max spec-ed at the time, but now 2 years old laptop here as my daily, which I acknowledge probably still puts it still well into the upper end of the performance bracket. For me personally the time vs complexity tradeoff is an obvious win on the side of simplicity here. I'd say that anything sub second is immaterial here. |
|
I'm only a user of hyper, so I can't say anything worthwhile about how to balance improved maintainability (which only matters directly for maintainers) with build time regressions. If I was a maintainer, I could look at the value this provides to me and try to come up with a number for how much build time I'm willing to trade for it. I trust that @seanmonstar and others will do this in the best way for their own health and the health of the project. But as user of hyper, there is no measurable regression I'd unconditionally accept as immaterial for me. My projects depend on many crates, not just hyper, and overall build time is influenced by all of them -- both by how long each crate takes to build in isolation and by interactions of the overall dependency graph (available parallelism, critical paths, feature unification, etc.). If every one of my 200 dependencies accepted a 5% or 0.1s build regression because none of them are noticeable for humans, build times for my project would get very noticeably slower. Naturally, this doesn't apply equally to all dependencies: it's more effective to spend effort on keeping build times low for crates that are compiled often and have non-trivial impact on downstream build times. Hyper scores relatively high on both metrics because of its popularity and because of its position in the dependency graph (downstream of tokio, upstream of most of the HTTP-related Rust ecosystem). That's why I thought it was worth my time to engage here in the first place. But again, that's just my priorities as a user of hyper: weighing that against maintainability of the code base is the maintainers' job. |
Introduces aliases for each feature flag as well as for the main
combinations of http version and client/server roles. This makes it
significantly easier to understand which feature flags are required on
any given line as the aliases require less parsing of nested
cfgexpressions.