v0.21.8 preparation #1525
v0.21.8 preparation #1525
Conversation
- several `clippy::slow_vector_initialization` - one `clippy::redundant_guards`
Codecov Report
@@ Coverage Diff @@
## rel-0.21 #1525 +/- ##
=========================================
Coverage 95.81% 95.81%
=========================================
Files 61 61
Lines 14803 14851 +48
=========================================
+ Hits 14183 14229 +46
- Misses 620 622 +2
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
The plan is to hold rustls/webpki#199 and this branch for a conclusive outcome to the perf. regression discussed in #1508 |
|
Regarding the SemVer issue, my understanding is there was accidentally one API that exposed ring and that you are proposing to just go ahead and break SemVer for that one API. Is that correct? If so, maybe now would be a good time to guard that specific API with a "unstable-ring" or similarly-named feature as it is likely to come up again in the future (ring 0.18, etc.). |
I don't think we've explicitly decided how to handle this. I put a more detailed TODO in the PR description for deciding on this item. Thanks for flagging.
If we're comfortable breaking semver (I don't see how we could do this update otherwise) it might be cleaner to switch the two affected fns to be |
I'd be down with that. |
|
Yeah, seems okay. |
That sounds better to me, if we don't expect people to be consuming it intentionally. |
|
Nit: in the release notes, I'd suffix method names with |
Fixed. Also pulled in #1532 |
|
What is this currently blocked on? Is it the ring 0.17 performance regression? Maybe we should just take that as a given and move forward? It doesn't seem like any improvements are on the (near-term) horizon? |
I think so. If we decide the GCC performance regression is acceptable then we can finish up rustls/webpki#199 and get this released soon after.
Agreed, the two upstream bugs I filed aren't giving any indication they'll be resolved soon. I think it's unfortunate to have a GCC performance regression in a patch release since we can't expect all consumers to be using clang but between holding the release indefinitely or releasing with the GCC regression well documented I think the latter is probably the best trade-off. @ctz What are your feelings on the matter? |
|
It's a bit frustrating to call it a performance regression when it's actually the fastest it's ever been, if you use clang. I do think it's worth calling out in the release notes that building with clang 14+ will result in faster X25519 and Ed25519 operations while building with GCC will result in slower X25519 and Ed25519 operations, when compared with the previous release. |
Apologies, it's not my intent to frustrate and I do appreciate that the update comes with a number of advantages. I will be more clear when describing the issue to emphasize it as specific to
With the "building with GCC" caveat applied I think it's uncontroversial to describe this as a performance regression, yes? |
|
I don't think we should block this further on the performance when using GCC. But we should add the C compiler and versions thereof as additional dimensions of performance testing now we're aware it can have an unexpectedly large effect. |
@aochagavia Is this something you've given any thought to? |
|
Regarding dimensions to use in performance testing, it should be reasonably straightforward to configure the C compiler to be GCC or Clang for different runs, as long as we stick to the versions available in the system. If we want to test multiple versions of the same compiler, we would probably need something more advanced (maybe using nix, as @cpu suggested elsewhere... but that sounds like a major time investment). Going meta, I'm wondering to which extent Rustls should be responsible for tracking performance of its crypto dependencies in the first place. Do you have an opinion on this? For instance, now we have alternative crypto backends, I assume we could create a no-op crypto implementation and use it for performance testing, so we measure only Rustls performance. On the other hand, most users would probably be thankful if we benchmarked Rustls together with supported crypto libraries, which means we might want to benchmark even more dependencies. |
|
I think performance like TLS connection throughput and handshake latency is a lot more tangible for people than the number of cycles spent on key exchange (to name just one example). Also if our goal in this project is to promote adoption of rustls (for whatever reasons), and given that we're (currently) definitely not the default option for most environments, showing that our performance is competitive seems an important component of that. |
|
I think this is in a good state -- let's publish this thing? |
|
I think the scope of our performance measurements should be focussed on the crate's defaults (that is, the ring provider), then any other providers we ship inside the crate (eg, aws-lc-rs when that lands) behind feature flags. I think that ideally covers factors we know alter the performance: C compiler and its version, and CPU architecture. I think "down-stream" providers (like the impending rustls-rustcrypto crate) are out of scope, though naturally it would be great if those reuse our existing performance and protocol testing code. The idea about a do-nothing crypto provider for testing maximum theoretical performance is definitely an interesting one! |
Brings in the ring 0.17 update.
The `Tls12CipherSuite::hash_algorithm` and `Tls13CipherSuite::hash_algorithm` functions were meant to be crate internal, since their return type leaks the `ring::digest::Algorithm` type. As written today these fns make updates to `*ring*` a breaking change for the Rustls API. This commit switches the visibility of both functions to be crate-internal. Strictly speaking this is a breaking change, but we don't expect there to be consumers of these functions and it unblocks a *ring* update that would otherwise be breaking on its own.
Updated the README/lib.rs notes about *ring* platform compatibility to fold in suggestions from Brian Smith.
cpu
dismissed
briansmith’s stale review
Requested changes have been implemented in-branch since Oct 10th
This is expected based on the change in 4797228 we call out in the release notes. |
|
Description
This branch targets the
rel-0.21release branch to make a point release to upgrade to ring 0.17.This involves bringing the work from #1508 onto the rel-0.101 branch. I didn't cherry-pick the commits directly because the restructuring of the modules in
mainmade it more challenging to resolve conflicts than to re-implement the changes wholesale.We don't typically mention MSRV updates, so that change is omitted from the proposed release notes. Other commits are clippy fixes and crate internal tidying also of no interest to downstream consumers.
As discussed in the main 0.17 update PR and in comments below updating ring is a breaking change for the 0.21.x release stream because we were accidentally exposing
ring::digest::Algorithmvia theTls13CipherSuite::hash_algorithmandTls12CipherSuite::hash_algorithmfns. We believe there should be few-to-none consumers of these fns downstream, and so a small breaking change within an otherwise semver compatible patch release to make these fn's crate-internal is the best way to unblock an update to ring that would be breaking on its own based on this API exposure.Proposed Release Notes
ConnectionCommon::complete_io()to flush writers before potentially expecting a response.*ring*to 0.17 - Note: ring 0.17 when built withgccwill experience slower X25519 and Ed25519 operations compared to previous releases.rustls-webpkito 0.101.7 to match*ring*0.17 dependencyTls12CipherSuite::hash_algorithm()andTls13CipherSuite::hash_algorithm()are now crate-internal. This is a small breaking change to remove unintended exposure of underlying*ring*types in the public API.TODO
Tls13CipherSuite::hash_algorithm()andTls12CipherSuite::hash_algorithm()leakingring::digest::Algorithm.gccring 0.17 X25519/Ed25519 performance regression unresolved.rustls-webpki0.101.7 release - v0.101.7 preparation webpki#199Cargo.tomlpatch