Benchmark results

Instruction counts

Significant differences

⚠️ There are significant instruction count differences

Other differences

Wall-time

Significant differences

There are no significant wall-time differences

Other differences

Additional information

Historical results

Checkout details:

• Base repo: https://github.com/rustls/rustls.git
• Base branch: main (3374299)
• Candidate repo: https://github.com/rustls/rustls.git
• Candidate branch: generic-conn (58a7c0b)

Codecov Report

❌ Patch coverage is 96.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.90%. Comparing base (3374299) to head (58a7c0b).
⚠️ Report is 814 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2669      +/-   ##
==========================================
+ Coverage   94.81%   94.90%   +0.08%     
==========================================
  Files          96       96              
  Lines       21826    21710     -116     
  Branches      614      614              
==========================================
- Hits        20695    20604      -91     
+ Misses       1003      978      -25     
  Partials      128      128              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm not super sure about this. I had a look at this branch in terms of docs, autocomplete and llvm-lines output:

For docs, the first docs for the most important type in the whole library looks like:

Aside from knowing to scroll halfway down the page, and finding impl Connection<Server> etc, there is really no way to go from here to understanding what types for Data are acceptable, or where to find them, or what precisely to write to get something working (either as a value, or a type, or what constructors are available for this type.) Apparently rustdoc outputs these in the order they are encountered in, so maybe it's under our control to have impl Connection<Client> appear "above the fold"?

Autocomplete - rust-analyzer gleefully guides you to code that doesn't actually compile, like:

let mut conn = rustls::Connection::new(Arc::new(config), server_name).unwrap();

(and then leaves you to read the compiler error and follow one of the two suggestions, only one of which can ever produce code that compiles.)

For llvm-lines for the bogo runner, this grows lines from 38k to 41k, and most of that is coming from the entirely of bogo::exec being monomorphised an extra time.

Generally I think I have a visceral reaction to generics in rust, and I will avoid them at all costs, so take this with a pinch of salt.

I'm not super sure about this.

Totally fair, and I'm okay with abandoning this PR if we decide it's not an improvement. I think I still like it despite your concerns, so will argue with your points a little:

I had a look at this branch in terms of docs, autocomplete and llvm-lines output:

For docs, the first docs for the most important type in the whole library looks like:

[cut the screenshot]

Aside from knowing to scroll halfway down the page, and finding impl Connection<Server> etc, there is really no way to go from here to understanding what types for Data are acceptable, or where to find them, or what precisely to write to get something working (either as a value, or a type, or what constructors are available for this type.) Apparently rustdoc outputs these in the order they are encountered in, so maybe it's under our control to have impl Connection<Client> appear "above the fold"?

Well, the first problem here is that there is no meaningful doc string for Connection because it used to be a mostly internal type. Here's a screenshot of what it looks like when that is cleaned up a bit. Note that this renames the overly generic Data type argument to Side to make it more obvious, and adds eager bounds to make it clearer as well. (Implemented in later commits.)

I agree that construction is less ergonomic, and that this is an important entry point, and rustdoc mostly does a decent job with representing the cascade of Deref implementations from ClientConfig -> ConnectionCommon<ClientConnectionData> -> CommonState. Apart from the constructors, arguably the generic methods for Connection are more relevant to most users than the side-specific ones, so I'd consider that a small win.

Autocomplete - rust-analyzer gleefully guides you to code that doesn't actually compile, like:

let mut conn = rustls::Connection::new(Arc::new(config), server_name).unwrap();

(and then leaves you to read the compiler error and follow one of the two suggestions, only one of which can ever produce code that compiles.)

Right. As a potential workaround, we could move these methods to the Config types instead. That would have the slight advantage of enabling chaining in the case where you only need a single connection.

For llvm-lines for the bogo runner, this grows lines from 38k to 41k, and most of that is coming from the entirely of bogo::exec being monomorphised an extra time.

It feels to me like abstracting over connections to the degree that bogo does it is pretty rare and probably shouldn't be a heavily weighted factor in deciding how to proceed with this. I think making it more obvious how to abstract over both types of connections has some value that might help in downstream crates (like tokio-rustls), and generally feels nicer to me in the sense of exposing our internal abstractions. I also feel like it opens up potential avenues for finding better ways to abstract over buffered/unbuffered/QUIC connections.

Generally I think I have a visceral reaction to generics in rust, and I will avoid them at all costs, so take this with a pinch of salt.

I'd be curious if you can unpack this more. In general we're already using them a fair amount (for Deref tower), and I feel like that is largely better (mostly, easier to maintain) than duplicating wrappers all over the place -- or using macros, which I guess I have more of a visceral reaction to!

I'm not super sure about this.

Do you think we should invite the Maston/Bluesky mob to give feedback?

@djc Is this still something you want to see move forward? Should it be set as a draft?

@djc Is this still something you want to see move forward? Should it be set as a draft?

Yes, would like to revisit this before 0.24.0 after discussion in #3149 (comment).