search

karljohns0n / nginx-more

Development repository for nginx-more package
MIT License
120 stars 29 forks source link

Consider adding those 2 patches #8

Closed gecon closed 4 years ago

gecon commented 4 years ago

Great job on this build @karljohns0n! Please consider adding the 2 patches below:

  1. Cloudflare's dynamic TLS records: https://github.com/cloudflare/sslconfig/blob/master/patches/nginx__dynamic_tls_records.patch (more on https://blog.cloudflare.com/optimizing-tls-over-tcp-to-reduce-latency/)

and

  1. Cloudflare's patch for HPACK Huffman encoding for response headers: http://hg.nginx.org/nginx/rev/ba3c2ca21aa5 (more on https://blog.cloudflare.com/hpack-the-silent-killer-feature-of-http-2/)

Thank you.

karljohns0n commented 4 years ago

Thanks for the feedback!

  1. I've read about this patch a few years ago but never implemented it. I can kick a test build with it. It hasn't been maintained by Cloudflare since they open source it so I'll see if it still compile.

  2. This patch has been merged upstream in 2016, see commit. You probably mean this one?

gecon commented 4 years ago

About 2, you are right, of course full HPACK it is (the partial support only is already there).

I understand that since there is not a clear-path (well maintained patches) this might be not possible or not easy or not stable enough to consider.

FYI I see an effort here to maintain the dynamic SSL patch on centminmod's nginx for 1.15.4

karljohns0n commented 4 years ago

Centminmod took the patch from hakasenyang. There's also kn007 who is maintaining one but we will be depending on them.

I ran a test build on el7 with both patches and it compiles well. You can try the package from here however I haven't tested it at all so try this in docker / dev server.

gecon commented 4 years ago

This is good news! Excellent! I tried it and am already using it.

Here is the much better header compression in action:

h2load https://www.XXXXXXX.tld -n 4 | tail -6 |head -1
traffic: 857B (857) total, 88B (88) headers (space savings 81.67%), 648B (648) data

Will also see dynamic SSL in a while!

After applying the patches, what is your opinion, can they survive in future releases?

Thank you!!

karljohns0n commented 4 years ago

Great, thanks for trying this test build! Please share results with dynamic SSL too when you can.

I don't mind adding those patches to the tree however it can be hard to maintain them when upgrading to next nginx branch. I can keep them as long as they are supported by the two Github users above but I wont maintain them myself. I will push them to the repo later.

gecon commented 4 years ago

I confirm, dynamic SSL is working fine.

Tested with wireshark, it started at 1393 it goes to 4253 after 40 records (ssl_dyn_rec_threshold default) and aftewards jumping again to 16k (nginx default if not changed).

Length: 1393 Length: 4253 Length: 16408

To enable (with the above defaults) you need to add in nginx configuration:

ssl_dyn_rec_enable on;

ssl sizes

It will be worth to also measure the benefits and will test on production too.

Let's hope they will survive in the future.

Great job @karljohns0n thank you!

karljohns0n commented 4 years ago

I've pushed both patches to the repo, see 029b727072adc0ec53160217e8912afb0ef480ec. Thanks for trying them so quick! I have a few things to add then I'll push an official rpm in testing repo.