Add rustls handshake to benchmarks #4063
Conversation
… into add-rustls-handshake
| send: Rc<RefCell<VecDeque<u8>>>, | ||
| } | ||
|
|
||
| impl ConnectedBuffer { |
There was a problem hiding this comment.
What is the point of this struct? It seems like you've already initialized the IO buffers in the individual harnesses.
There was a problem hiding this comment.
For Rustls and OpenSSL, you need a trait object implementing Read + Write, where you read and write messages to the other connection. Having ConnectedBuffer in harness.rs and not rustls.rs is mainly because OpenSSL would also need this trait impl. The IO buffers are also only ever initialized once, and we create new references to the buffers with Rc::clone().
There was a problem hiding this comment.
I would probably not put this struct in this file, since it's just for benchmarking code. It would be better to put it in some sort of common.rs file that has shared code. If you don't want to do that you could just put this in the rustls.rs file for right now, since that's where it's being used.
Actually that probably goes for anything in this file, is there a reason why you defined the CipherSuites in this file too?
There was a problem hiding this comment.
I guess I'm cheating because I have extra context, but this struct is common to multiple benchmarks (or will be once we merge in the Openssl benchmark). If we want to could break it out into a utilities module? For the time being I'm okay with it living in the same file.
I think the CipherSuites are in this file because we need some way of abstracting over the individual configuration for each implementation, because individual implementations are so different (e.g. s2n-tls is the only one with a concept of security policies)
There was a problem hiding this comment.
Since the harness would also be common to the s2n-tls and rustls impls, wouldn't the harness trait also belong in common.rs? I guess I've been treating harness.rs as common.rs, but I feel like having both might be a little redundant because both harness.rs and common.rs would have shared code.
There was a problem hiding this comment.
It's more like, there seems to be a distinction between the harness trait that each library has to implement in order to benchmark, and the code necessary to setup each library to handshake. So in my mind it makes the most sense to have those be in separate files. But it doesn't sound like anyone else thinks this distinction is important so I'm cool with approving it.
| send: Rc<RefCell<VecDeque<u8>>>, | ||
| } | ||
|
|
||
| impl ConnectedBuffer { |
There was a problem hiding this comment.
It's more like, there seems to be a distinction between the harness trait that each library has to implement in order to benchmark, and the code necessary to setup each library to handshake. So in my mind it makes the most sense to have those be in separate files. But it doesn't sound like anyone else thinks this distinction is important so I'm cool with approving it.
Description of changes:
Add Rustls to handshake benchmarking and refactor benchmark harness.
Testing:
This change doesn't change the source code for s2n-tls and only builds on top of it, so it shouldn't affect tests. There are added benchmark harness specific unit tests that test the refactored harness.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.