Upgrade to ring 0.17 #1508
Upgrade to ring 0.17 #1508
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1508 +/- ##
=======================================
Coverage 96.46% 96.46%
=======================================
Files 74 74
Lines 15111 15113 +2
=======================================
+ Hits 14577 14579 +2
Misses 534 534
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@aochagavia the instruction count differences seem pretty worrying. Could you run time-based tests on your fancy new server for this PR? |
I've been focusing on #1485, so we don't have automated time-based benchmarks at the moment. I could spend some time tomorrow to create a bunch of ad-hoc criterion benchmarks and run them manually. Would that help? |
I actually have some of these on the aws-lc-rs branch -- will extract them to merge separately. |
That is #1511 -- local runs after merging that onto this gives: |
|
@briansmith are these performance changes to be expected? |
|
It looks like it is just Curve25519? The faster Curve25519 will come in ring 0.17.1. I will ping you with the PR when it is ready. |
|
@ctz what is the exact hardware you are running on? FWIW, I do think the Curve25519 regressions would be expected although I thought they would be smaller than that. There is a better-optimized Curve25519 that I brought into ring 0.17.0 as part of the BoringSSL merge but didn't enable because I wanted to do some additional review/testing. In the case of RSA, it looks like the benchmarks are noisy since we wouldn't expect PSS vs PKCS#1 to make a difference. If you could clean up the noise it would be great. BoringSSL did make a change that might have negatively affected performance slightly, and my analogous change might have affected performance slightly more than that. If so, it can be fixed. Similarly, some other algorithms will also get faster because I brought in new faster code, but didn't hook it up. However, unlike Curve25519, there weren't any major changes that would have regressed performance, at least intentionally, IIRC. |
|
I would suggest waiting a bit before doing this, until more of the perf work in 0.17.1 is done, so that we can understand how that work affects the build requirements. It might require a newer GCC and/or clang than 0.17.0 requires right now, and I'd like to get that all clarified. |
|
@ctz briansmith/ring#1688 is the PR for adding the new optimized Curve25519. |
64-bit Linux on Intel Skylake -- specifically i7-7820X. |
|
Should we hold off on merging this until the performance regresssion is dealt with? Should we hold off on releasing a 0.21.x release with this until the performance regression is dealt with? (I'm thinking maybe not and yes.) |
I would think probably not? I feel rustls 0.21 and rustls-webpki 0.101 should be maintenance/serious bug fixes only at this point.
I'd agree with that assessment. I don't see an eventuality where the first 0.22.x release goes out with ring 0.16. This PR probably also depends on (or could incorporate) taking rustls-webpki==0.102.0-alpha.4 -- see eg fb8ee75 |
|
Hmm, I think I disagree. Clearly there's a lot of excitement about ring 0.17 (given the amount of issues opened and the number of issues/PRs linking to this PR), probably because of the massively increased portability? The point of not having ring appear in the public API means that we can bump ring without this being externally visible. rustls 0.22 is probably at least a week from being released and even when we release it, it will take the ecosystem a while to update dependencies -- that means that, for the time being, downstreams might be likely to pull two versions of ring into their dependency graphs when there's really no good reason to do that. So I think we should have ring 0.17 backports for webpki 0.101 and rustls 0.21 and release them as soon as we think it's ready, but maybe after the worst of the performance regressions has been addressed. Or do you see serious risks for rustls 0.21 from our upgrade to ring 0.17 (I don't really). Plus, once we release rustls 0.22 we're on the hook to provide rustls 0.21 maintenance for a year longer -- that can be quite limited but it might still be nice if they are both on the latest ring release. |
I'm not sure we have anything that validates we've achieved this though. I just went to check 0.21 and I guess it would be interesting if
I don't see this as about risk, more about the extent to which previous release branches are active vs just maintained. |
Hmm, that's fair. Well, if we expose that probably one reason to not backport to 0.21.
That would be a really interesting test -- I suppose it would?
That's fair. I would be inclined to say that the 0.21 branch should definitely be perceived as "active" since it's what ~everyone is using right now. That doesn't mean that we backport every feature and refactoring, but for something like this which (a) should be (mostly) semver-compatible and (b) makes downstream users` life better with broader compatibility and (c) low risk, I like the cost-benefit of backporting. It's also really nice to stick with up-to-date dependencies to avoid having duplicates in dependency graphs... |
fwiw, I agree. I don't think it's something we should necessarily do for every dependency but I think for this case it makes sense. |
5305a96 to
c4a38d6
Compare
|
Started a 0.21.x backport in #1525 |
|
0.17.1 was released with the performance improvement to Ed25519. |
|
More of the performance optimizations added in recent BoringSSL changes will appear in upcoming 0.17.x releases. We are trying a new QA process for enabling these. |
|
I bumped the lockfile to 0.17.2 in #1528, but it didn't seem to help much with the performance regressions seen here. |
|
Here are the latest benchmarks: briansmith/ring#1688 (comment). The fastest combination is ring 0.17 using Clang 17. |
Question: do we want to backport this to rustls 0.21? Would enable usage of rustls a on bunch more targets.