Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nghttpx: dynamic TLS record size behaviour configuration #393

Closed
LPardue opened this issue Oct 16, 2015 · 28 comments
Closed

nghttpx: dynamic TLS record size behaviour configuration #393

LPardue opened this issue Oct 16, 2015 · 28 comments

Comments

@LPardue
Copy link
Contributor

LPardue commented Oct 16, 2015

During performance testing of nghttpx I identified that the dynamic TLS record size behavior implemented for HTTP/2 can reduce overall throughput in certain cases. During that activity I tested increasing the time out period before the record size decreased. More details are available on (this blog)[http://www.bbc.co.uk/rd/blog/2015/07/performance-testing-results-of-adaptive-media-streaming-over-http].

The change was implemented by modifying code and recompiling, which is not a general solution. Therefore I would like to propose the addition of a new nghttpx option, perhaps --tls-warmup-threshold that takes a DURATION. This would allow a user to select a timeout appropriate for their use case.

It looks like the implementation of this behaviour has changed since I last looked at it. Previously I had modified ClientHandler::get_write_limit(). However there is now a single reference to this function (in a comment block) so I need to investigate some more before being able to start on a pull request.

@tatsuhiro-t
Copy link
Member

We have 2 criteria to enable full record size transfer: timeout and bytes written to socket without hitting timeout.

The timeout calculation is done at https://github.com/tatsuhiro-t/nghttp2/blob/5d002ff6caddd3283b6b01efd5a20382e8df6866/src/shrpx_connection.cc#L500

Warmup size check is done a bit below, its size is hard coded value SHRPX_WARMUP_THRESHOLD, which is 1MiB.

If you mean --tls-warmup-threshold is warm up byte size, then it should not accept DURATION, since it uses units for time duration.
On the other hand, if it means a timeout value, then the name should be renamed to reflect that.

Either way, we can add additional parameters to Connection constructor and refer them in the code.

@LPardue
Copy link
Contributor Author

LPardue commented Oct 17, 2015

I think I probably mean both :)

Thanks for the tips i will take a look at this and prepare a PR in the
coming days.
On Oct 17, 2015 3:17 AM, "Tatsuhiro Tsujikawa" notifications@github.com
wrote:

We have 2 criteria to enable full record size transfer: timeout and bytes
written to socket without hitting timeout.

The timeout calculation is done at
https://github.com/tatsuhiro-t/nghttp2/blob/5d002ff6caddd3283b6b01efd5a20382e8df6866/src/shrpx_connection.cc#L500

Warmup size check is done a bit below, its size is hard coded value
SHRPX_WARMUP_THRESHOLD, which is 1MiB.

If you mean --tls-warmup-threshold is warm up byte size, then it should
not accept DURATION, since it uses units for time duration.
On the other hand, if it means a timeout value, then the name should be
renamed to reflect that.

Either way, we can add additional parameters to Connection constructor and
refer them in the code.


Reply to this email directly or view it on GitHub
#393 (comment)
.

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

Can I check the default idle timeout period (can't locate the documentation for this capability at present but will add this to help output). Code suggests it is 1 second, is that correct?

@tatsuhiro-t
Copy link
Member

Yes, 1 second is the default. I heard that it is used in google server.

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

Thanks for the clarification.

Sorry for a similar question about the record size, I remember this being 16K but the function get_tls_write_limit returns std::numeric_limits<ssize_t>::max() which comes out as on my machine as 9223372036854775807. I think I'm missing something obvious here.

@tatsuhiro-t
Copy link
Member

That is intentional. It means "unlimited".

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

That is intentional. It means "unlimited".

So is the 16K limit enforced by SSL library?

@tatsuhiro-t
Copy link
Member

Yes. We take min(the input lengh, get_tls_write_limit()), and pass the data with that length to SSL_write(). SSL_write() will split passed data into multiple TLS records if necessary.

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

OK thanks for the clarification, hope to get the PR up within the next day!

@tatsuhiro-t
Copy link
Member

I'm ok to return 16KiB if it is more readable. I don't think there is performance hit by doing so, but should check it.

@tatsuhiro-t
Copy link
Member

Thank you!

@ghost
Copy link

ghost commented Oct 20, 2015

Hi,

I wanted to ask about this some time ago, where the tls record size / tls buffer size is defined in nghttpx, my guess was shrpx.cc / mod_config()->downstream_[request|response]_buffer_size = 16_k, but changing this value did not make any difference. Would it be possible to specify the buffer size in nghttpx manually via config (--tls-buffer-size=[1400|16k] for example), and, maybe as a bonus (not quite sure if this needed nowadays), specify "tls-false-start=[0|1]", according to https://www.igvita.com/2013/12/16/optimizing-nginx-tls-time-to-first-byte/ and https://www.igvita.com/2013/10/24/optimizing-tls-record-size-and-buffering-latency/ ?

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

So as I understand (and correct me if I am wrong Tatsuhiro) nghttpx supports TLS dynamic record size behaviour for HTTP/2 on the frontend only. This behavior is implemented in line with Ilya's blog recommendations.

So at the moment, the record size flips between 1300 B and 16KiB according to write thresholds and idle timeouts. I'm working on adding configurability of write thresholds and idle timeouts.

It sounds like you would like an option to define min/max TLS record sizes, with whatever values you want (within reason). Is that right? This is something I actually thought about today while working in this area, its certainly possible but not sure if Tatsuhiro thinks its appropriate.

False start I know nothing about sorry!

@ghost
Copy link

ghost commented Oct 20, 2015

If the record size flips dynamicly (based on...?), then I guess there won't be any need to set it manually. I just wasn't quite sure if the record size is fixed to 16k, and if so if it would be possible to lower that value manually w/o modifying the source.
False start is basicly a client thing, but depends on ALPN/NPN on server side, so this should be enabled by default, but confirmation from tatsuhiro would be appreciated ;)

@LPardue
Copy link
Contributor Author

LPardue commented Oct 20, 2015

If the record size flips dynamicly (based on...?), then I guess there won't be any need to set it manually. I just wasn't quite sure if the record size is fixed to 16k, and if so if it would be possible to lower that value manually w/o modifying the source.

For default nghttpx behavior, Record size starts at 1300 B, after 1MiB of data is sent the size is increased to 16KiB. If there is a period of inactivity greater than 1 second the record size drops down to 1300 B.

For experimentation purposes it might be useful be able to adjust the record size values.

@tatsuhiro-t
Copy link
Member

I wanted to ask about this some time ago, where the tls record size / tls buffer size is defined in nghttpx, my guess was shrpx.cc / mod_config()->downstream_[request|response]_buffer_size = 16_k, but

downstream_[request|response]_buffer_size is not buffer size for TLS records. They are buffer size to store request body/response body.

So as I understand (and correct me if I am wrong Tatsuhiro) nghttpx supports TLS dynamic record size behaviour for HTTP/2 on the frontend only. This behavior is implemented in line with Ilya's blog recommendations.

dynamic record size is enabled for both frontend and backend TLS connections.
We use same class for both. And yes, the behavior is from Ilya's blog.

It sounds like you would like an option to define min/max TLS record sizes, with whatever values you want (within reason). Is that right? This is something I actually thought about today while working in this area, its certainly possible but not sure if Tatsuhiro thinks its appropriate.

If the record size flips dynamicly (based on...?), then I guess there won't be any need to set it manually. I just wasn't quite sure if the record size is fixed to 16k, and if so if it would be possible to lower that value manually w/o modifying the source.

The parameter values are from google's experiment. 16K size makes sense because smaller record size increases overhead (actually consumes more CPU), and makes more packets (we disabled Nagl's algorithm). I have no strong objection to change this value, but I wonder it adds any value.

False start is basicly a client thing, but depends on ALPN/NPN on server side, so this should be enabled by default, but confirmation from tatsuhiro would be appreciated ;)

We support ALPN/NPN, so it should be enabled, but I don't know how to know it is used actually. Does openssl s_client show something useful?

@ghost
Copy link

ghost commented Oct 20, 2015

Wow, thank you for clearing that up!

downstream_[request|response]_buffer_size is not buffer size for TLS records. They are buffer
size to store request body/response body.

I was grepping through the source looking for implicitly set buffer/record size values, this was the only one I found. Thing is, I have an outstanding issue with every 1 or 2 out of 10 requests stalling at connection initiation state for up to 1,5s (under high traffic), even when the connection has already been established (just following a link to another page on the site). This is definititly a TCP based issue, since no request to the backend has been made yet. So I was looking to squeeze a little bit more performance out of nghttpx, but I guess there really is no way for that, seems to be TOO perfect (except for TCP_FASTOPEN, which is not available in the kernel in use :().

For dynamic record sizes - is this behaviour defined in shrpx_connection.cc / Connection::get_tls_write_limit()?

We support ALPN/NPN, so it should be enabled, but I don't know how to know it is used actually.
Does openssl s_client show something useful?

Unfortunately not. It just says "No ALPN negotiated" (with ECDHE-RSA). False-start was a chrome/SPDY thing, and was dropped in 2012 (?, see https://www.imperialviolet.org/2012/04/11/falsestart.html), but reappeared in 2014 (see https://src.chromium.org/viewvc/chrome/trunk/src/net/socket/ssl_client_socket_openssl.cc?pathrev=265685). I was looking for a way to check in chrome if it's being used or not, couldn't find anything in chrome://net-internals so far :-/

@tatsuhiro-t
Copy link
Member

Thing is, I have an outstanding issue with every 1 or 2 out of 10 requests stalling at connection initiation state for up to 1,5s (under high traffic), even when the connection has already been established (just following a link to another page on the site). This is definititly a TCP based issue, since no request to the backend has been made yet.

Interesting. Do you see HTTP request is also delayed or it is already arrived in nghttpx, but the backend connection is delayed? I know this is a bit hard to know, since it requires INFO level log, and it is too verbose for high traffic site.
Browser may connect to the site speculatively for faster request later?

For dynamic record sizes - is this behaviour defined in shrpx_connection.cc / Connection::get_tls_write_limit()?

Yes

@LPardue
Copy link
Contributor Author

LPardue commented Oct 22, 2015

In the end we added:

  • --tls-dyn-rec-warmup-threshold=<SIZE> that controls size of write buffer before record size increases to 16 KB. Default 1M.
  • --tls-dyn-rec-idle-timeout=<DURATION> that controls idle timeout period after which the record size decreases to 1300 bytes. Default 1 second.

This was merged to master as of 7b35c28

@LPardue LPardue closed this as completed Oct 22, 2015
@ghost
Copy link

ghost commented Oct 23, 2015

Interesting. Do you see HTTP request is also delayed or it is already arrived in nghttpx, but the
backend connection is delayed? I know this is a bit hard to know, since it requires INFO level log,
and it is too verbose for high traffic site.

I was not quite sure where to start. I have re-taken control over maintaining my own nginx debian packages, lightweight and with aio threads + sendfile enabled, since I first suspected i/o blocking in nginx.
Now on my tiny dev-vm I'm running h2load against the the sitemap containing about 14k pages with a 500ms delay. initial connect took it's time, but now h2load is fetching one page every 500ms with return access code 200, so no problem. But when I try to connect to the site with chrome, TTFB goes up to about 20s, and that goes for every page request.

Sample:
image

But when the connection was once established, content download is then done within 20-40ms. nghttpx is build with libevent and threading too. Next step, tshark. :-/

@tatsuhiro-t
Copy link
Member

So only the connection time is affected somehow. Request sent is 0.17ms, so chrome thought it dispatched request. Checking nghttpx INFO log may tell us something. Does chrome always suffer this problem without any load on server?

@kg
Copy link

kg commented Aug 17, 2017

I think I'm hitting a variation on that delayed request problem when using Chrome to connect to nghttpx. I have nghttpx sitting in front of a Squid proxy and periodically, a single request will be stalled in the 'waiting' state for multiple seconds, sometimes as long as 20 seconds. After it 'wakes up' it completes immediately. Most requests complete very quickly with consistent timing. I have a Chrome bug filed about it at https://bugs.chromium.org/p/chromium/issues/detail?id=756063 that has more details. The Chrome developers say the browser is "stalled waiting to receive the body from the server". Is it possible that when the record size rises to 16KB, nghttpx waits until it has 16KB worth of data to forward it to the client? These response bodies are very small, so if it does that, the connection would stall indefinitely unless another request is issued over the connection.

@tatsuhiro-t
Copy link
Member

As far as I know, nghttpx does not wait for such a condition.

@kg
Copy link

kg commented Aug 17, 2017

Thanks. I figured that couldn't be the case. Is there any way I could get nghttpx to generate a detailed diagnostic log? I didn't see one when I skimmed the documentation.

@tatsuhiro-t
Copy link
Member

Try -LINFO, or log-level=INFO in configuration file.
--frontend-frame-debug may help for http/2 protocol level debugging but you have to disable multi threading because it scrambles outputs.

@tatsuhiro-t
Copy link
Member

@kg Do you still experience stalling issue? Which version are you using?

@kg
Copy link

kg commented Aug 24, 2017

I built the latest version from source, because Ubuntu's apt packages are very old. I still have the issue, but I'm waiting to see what the Chrome people think about it. When I get some time I may try some protocol level debugging like you suggested.

@tatsuhiro-t
Copy link
Member

I recommend to remove standard nghttpx package from ubuntu completely to avoid to accidentally use the old nghttpx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants