perf: coalesce buffer instead of calling SSL_write() for each slice.

[PiotrSikora]

This results in significant performance gains, because it avoids:

1. emitting a lot of tiny TLS records, which results in less overhead,
2. calling write() for each slice.

Fixes #2667.

[PiotrSikora]

Let's wait for @htuch to confirm that this clears all the issues he observed in his load tests, but it should fix at least some of them.

cc @alyssawilk @ggreenway @ldemailly @lizan

[mattklein123]

stupid openssl/boringssl question: I don't suppose there is some writev equivalent? Seems a shame we need to linearize here.

[alyssawilk]

I don't think these is one? Can we tag off-project people? Maybe @davidben knows

[alyssawilk]

Sanity check: if we linearize multiple times it only does the work one time, correct? I want to make sure there's no corner case where trickling bites gets super expensive.

[dvorak42]

Unfortunately boringssl doesn't have a built in writev equivalent. You could potentially create a fake FD to use writev with and then dump that into SSL_write, but that probably has more overhead/complexity than linearize.

[davidben]

We do have plans to add an in-place API so you don't need to copy things in and out of OpenSSL's goofy fd abstraction, though probably the first iteration will still require it be linearized. writev is a little tricky with block ciphers. (Though that work will make it easier to add something like writev later.)

[PiotrSikora]

@mattklein123 Done.

@alyssawilk Yes, linearization will happen only once (for the same size argument).

[mattklein123]

@PiotrSikora do you mind also fixing the comment here? https://github.com/envoyproxy/envoy/blob/master/source/common/network/connection_impl.cc#L327 It still applies I think, but I think the function name it talks about is now out of date.

[ggreenway]

Could we use mode SSL_MODE_ENABLE_PARTIAL_WRITE to make the code not have to do the whole "call SSL_write() with the exact same parameters again" thing?

[davidben]

No. That is somewhat fundamental to OpenSSL's attempt to jam TLS into a UNIX-like API. If you send half a record and then get EWOULDBLOCK, you have committed to sending that entire input, but haven't yet. That's not a state that normally exists in a TCP API because committed data goes into a buffer and the kernel wakes up to drive the state machine beyond that point. OpenSSL does not have a way to wake up to drive the state machine.

The future in-place API I mentioned is to fix that by making what's going on much more explicit.

[davidben]

(SSL_MODE_ENABLE_PARTIAL_WRITE is about what happens if it decides to construct multiple records out of your large input for whatever reason. The kernel could still only have buffer for half of a record. Records are atomic.)

[davidben]

I should note: there's no requirement that you pass in the exact same write size, only that you pass in at least the same data as before. You can pass in more if you have more available now.

You even can reallocate the buffer, but then you'll need SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER to disable the silly pointer check.

(Yeah, this API is terrible. :-( )

[davidben]

(Sorry about comment spam.)

Oh, but if you do that, note that we won't tell you when we've released partial data because, by default, OpenSSL decided their API won't return success until everything you passed in has been written. If you'd like to release stuff from your buffers earlier and have logic to tolerate partial returns, that's where that partial write toggle comes in.

[ggreenway]

@davidben So if we enable both SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER and SSL_MODE_ENABLE_PARTIAL_WRITE, could we just do this, and not treat a previous partial/zero write as a special case?

int size = min(buffer.size(), 16384);
int rc = SSL_write(ssl, buffer.linearize(size), size);
if (rc > 0) {
   buffer.removeBytesFromFront(rc);
} else {
// error handling, etc
}

[davidben]

Yeah, that should work. Just don't "change your mind" about what you're writing. (That's really all the check is looking for.)

[PiotrSikora]

@davidben, SSL_MODE_ENABLE_PARTIAL_WRITE lets SSL_write() return after successful write of a whole fragment, but we're never passing more than 16384 bytes (i.e. default max_send_fragment), so I don't think it would make any difference? Or am I misreading the code?

[davidben]

It would matter if something like this happens:

1. You call SSL_write with 1000 bytes, because that's what's in the buffer. It returns -1 because the transport can't write the entire record.
2. Meanwhile, you get another 1000 bytes to write.
3. You call SSL_write with 2000 bytes, the first 1000 of which is what you passed last time.
4. The transport now has room to write that first record, but not the next record.

With SSL_MODE_ENABLE_PARTIAL_WRITE, it will return as soon as the first record is written and tell you the first 1000 bytes may be discarded. Without it, it won't return until everything you've currently passed in is flushed, which, in the case above, means you release those first 1000 bytes a little later than necessary. Dunno if you care about that.

(Of course, in reality BoringSSL already has the ciphertext for those 1000 bytes all set up, so it doesn't really need those first 1000 anymore as soon as (1) is done. But this API is dumb and tries to look like POSIX... :-/)

[htuch]

@PiotrSikora I've been OOTO past 2 days, will rerun load tests tomorrow with this PR.

[PiotrSikora]

@davidben yeah, but that won't happen with the code as it's written now, since the retry call will pass the same amount of data as in failed call... and passing more data doesn't seem to have any benefits, since the record size is already determined by the first call, and it would require completely unnecessary linearization of data that spans multiple records.

[lizan]

@mattklein123 does this part still apply? With this PR SslSocket::doWrite doesn't depend on each element but instead linearize first bytes_to_write bytes and pass it to SSL_write. So the code doesn't assume "we never change existing write_buffer_ chain elements between calls to SSL_write()", just assuming we never drain data from the buffer between calls to SSL_write(), no?

[PiotrSikora]

You can append, but you cannot drain, which is the same as before changes in this PR, AFAIK.

[lizan]

IIUC before this PR, a buffer {"abc"}, couldn't be {"abcde"} in this method, you can only add one or more slice then make it into {"abc","de"}. With this PR, either {"abcde"} or {"abc","de"} will work since you captures bytes_to_write in SslSocket::doWrite.

[PiotrSikora]

Per @davidben, you can pass more data to the retry call (but it must start with the same data as the failed call), so {"abc"} -> {"abcde"} was also fine before this PR.

Capturing bytes_to_retry_ is really an optimization to avoid linearizing over data that's going to be sent over multiple TLS records that doesn't need to be combined together.

[lizan]

Yeah after reading @davidben's comment I noticed that {"abc"} -> {"abcde"} was also find before this PR, but I think the original intention is not to allow that case, no? @mattklein123

[davidben]

The pointer needs to be the same by default but it's just a silly sanity check. If your buffer may be reallocated or whatever, set the moving buffer option.

[davidben]

The way to think about it is it's like POSIX write where you either get told the write failed with a retryable error or it finished completely with N bytes written.

The difference is that write allows this:

write("hello") => EWOULDBLOCK
write("actually I changed my mind")

with the first input never making it on the wire. This shape of API doesn't compose well with TLS if we managed to write half a record. (Writing half a record does not mean you wrote half the plaintext. It means you half-wrote the full plaintext and can't change your mind.)

OpenSSL's API handles this case by returning EWOULDBLOCK (so it looks to you like nothing was written) and then says you are not allowed to change your mind on what you are writing. Once you pass input in, you gave committed to that input. It enforces this with some sanity checks on the length and optionally the pointer, but the actual rule is the "no rewinding" rule. You want to think about that one and not the length business.

[PiotrSikora]

@mattklein123 it doesn't have to be the same pointer and length, but the data needs to be the same, so that comment isn't completely wrong, IMHO. And the only reason why the pointer doesn't need to be the same is because SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER is set, but it doesn't look that there is a reason for it, so I'm going to remove it, which will make the comment a bit more accurate.

[mattklein123]

SGTM, as long as you checked the comment and you think it's good it's fine with me. Thank you all for the very lively and enlightening discussion on this. :)

[lizan]

SGTM.

[alyssawilk]

LGTM. Will wait on loadtest results for merging.

[htuch]

This PR fixes the original issue for small request size; the SSL penalty is now only ~8% vs. ~33% originally for 1 byte requests. For 1MB requests, the penalty is ~49%.

[ggreenway]

1 minor nit, then I think this is ready

[ggreenway]

style: while (bytes_to_write > 0)

[PiotrSikora]

Done.

[mattklein123]

This PR fixes the original issue for small request size; the SSL penalty is now only ~8% vs. ~33% originally for 1 byte requests. For 1MB requests, the penalty is ~49%.

@htuch just to confirm this doesn't make the large request penalty larger, does it?

[htuch]

@mattklein123 Correct, it actually improves it a few percent, but it's still big.

[htuch]

Thanks!