Re: [PATCH] HTTP/2: add support for HPACK encoding

2017-07-24 Thread Valentin V. Bartenev 
On Monday 24 July 2017 12:33:55 Vlad Krasnov via nginx-devel wrote:
[..]
> >> +ngx_http_v2_hpack_enc_entry_thtable[HPACK_ENC_HTABLE_SZ];
> >> +ngx_http_v2_hpack_name_entry_t   heads[HPACK_ENC_DYNAMIC_KEY_TBL_SZ];
> >> +u_char   
> >> storage[NGX_HTTP_V2_MAX_HPACK_TABLE_SIZE +
> >> + HPACK_ENC_MAX_ENTRY];
> >> +} ngx_http_v2_hpack_enc_t;
> >> +#endif
> >> +
> >> +
> > 
> > Well, it means that even idle connection will consume 18+ KB more
> > memory than before.
> > 
> > That doesn't look like a generic solution suitable for most of our users.
> > It should be at least configurable.
> > 
> 
> I can make it configurable fairly easily, but then it will require an extra 
> allocation.
> 

With your current patch ngx_http_v2_connection_s doesn't fit into
memory pool anymore and requires an extra allocation anyway.

  wbr, Valentin V. Bartenev

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: add support for HPACK encoding

2017-07-24 Thread Vlad Krasnov via nginx-devel 

> On Jul 24, 2017, at 11:42 AM, Valentin V. Bartenev  wrote:
> 
> On Thursday 22 June 2017 18:23:46 Vlad Krasnov via nginx-devel wrote:
>> # HG changeset patch
>> # User Vlad Krasnov mailto:v...@cloudflare.com>>
>> # Date 1498167669 25200
>> #  Thu Jun 22 14:41:09 2017 -0700
>> # Node ID 895cea03ac21fb18d2c2ba32389cd67dc74ddbd0
>> # Parent  a39bc74873faf9e5bea616561b43f6ecc55229f9
>> HTTP/2: add support for HPACK encoding
>> 
>> Add support for full HPACK encoding as per RFC7541.
>> This modification improves header compression ratio by 5-10% for the first
>> response, and by 40-95% for consequential responses on the connection.
>> The implementation is similar to the one used by Cloudflare.
>> 
> 
> First of all, there's no way for a patch to be committed with so
> many style issues.
> 
> Please, examine how existing nginx sources are formatted and mimic
> this style.

OK.

> 
> 
> 
>> diff -r a39bc74873fa -r 895cea03ac21 auto/modules
>> --- a/auto/modules   Mon Jun 19 14:25:42 2017 +0300
>> +++ b/auto/modules   Thu Jun 22 14:41:09 2017 -0700
>> @@ -436,6 +436,10 @@
>> . auto/module
>> fi
>> 
>> +if [ $HTTP_V2_HPACK_ENC = YES ]; then
>> +have=NGX_HTTP_V2_HPACK_ENC . auto/have
>> +fi
>> +
>> if :; then
>> ngx_module_name=ngx_http_static_module
>> ngx_module_incs=
>> diff -r a39bc74873fa -r 895cea03ac21 auto/options
>> --- a/auto/options   Mon Jun 19 14:25:42 2017 +0300
>> +++ b/auto/options   Thu Jun 22 14:41:09 2017 -0700
>> @@ -59,6 +59,7 @@
>> HTTP_GZIP=YES
>> HTTP_SSL=NO
>> HTTP_V2=NO
>> +HTTP_V2_HPACK_ENC=NO
>> HTTP_SSI=YES
>> HTTP_POSTPONE=NO
>> HTTP_REALIP=NO
>> @@ -221,6 +222,7 @@
>> 
>> --with-http_ssl_module)  HTTP_SSL=YES   ;;
>> --with-http_v2_module)   HTTP_V2=YES;;
>> +--with-http_v2_hpack_enc)HTTP_V2_HPACK_ENC=YES  ;;
> 
> What's the reason for conditional compilation?

Internal reasons, but I decided to keep it in the patch, because maybe not 
everyone wants that overhead.

> 
> 
>> --with-http_realip_module)   HTTP_REALIP=YES;;
>> --with-http_addition_module) HTTP_ADDITION=YES  ;;
>> --with-http_xslt_module) HTTP_XSLT=YES  ;;
>> @@ -430,6 +432,7 @@
>> 
>>   --with-http_ssl_module enable ngx_http_ssl_module
>>   --with-http_v2_module  enable ngx_http_v2_module
>> +  --with-http_v2_hpack_enc   enable ngx_http_v2_hpack_enc
>>   --with-http_realip_module  enable ngx_http_realip_module
>>   --with-http_addition_moduleenable ngx_http_addition_module
>>   --with-http_xslt_moduleenable ngx_http_xslt_module
>> diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.c
>> --- a/src/core/ngx_murmurhash.c  Mon Jun 19 14:25:42 2017 +0300
>> +++ b/src/core/ngx_murmurhash.c  Thu Jun 22 14:41:09 2017 -0700
>> @@ -50,3 +50,63 @@
>> 
>> return h;
>> }
>> +
>> +
>> +uint64_t
>> +ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed)
>> +{
>> +uint64_t  h, k;
>> +
>> +h = seed ^ len;
>> +
>> +while (len >= 8) {
>> +k  = data[0];
>> +k |= data[1] << 8;
>> +k |= data[2] << 16;
>> +k |= data[3] << 24;
>> +k |= (uint64_t)data[4] << 32;
>> +k |= (uint64_t)data[5] << 40;
>> +k |= (uint64_t)data[6] << 48;
>> +k |= (uint64_t)data[7] << 56;
>> +
>> +k *= 0xc6a4a7935bd1e995ull;
>> +k ^= k >> 47;
>> +k *= 0xc6a4a7935bd1e995ull;
>> +
>> +h ^= k;
>> +h *= 0xc6a4a7935bd1e995ull;
>> +
>> +data += 8;
>> +len -= 8;
>> +}
>> +
>> +switch (len) {
>> +case 7:
>> +h ^= (uint64_t)data[6] << 48;
>> +/* fall through */
>> +case 6:
>> +h ^= (uint64_t)data[5] << 40;
>> +/* fall through */
>> +case 5:
>> +h ^= (uint64_t)data[4] << 32;
>> +/* fall through */
>> +case 4:
>> +h ^= data[3] << 24;
>> +/* fall through */
>> +case 3:
>> +h ^= data[2] << 16;
>> +/* fall through */
>> +case 2:
>> +h ^= data[1] << 8;
>> +/* fall through */
>> +case 1:
>> +h ^= data[0];
>> +h *= 0xc6a4a7935bd1e995ull;
>> +}
>> +
>> +h ^= h >> 47;
>> +h *= 0xc6a4a7935bd1e995ull;
>> +h ^= h >> 47;
>> +
>> +return h;
>> +}
>> diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.h
>> --- a/src/core/ngx_murmurhash.h  Mon Jun 19 14:25:42 2017 +0300
>> +++ b/src/core/ngx_murmurhash.h  Thu Jun 22 14:41:09 2017 -0700
>> @@ -15,5 +15,7 @@
>> 
>> uint32_t ngx_murmur_hash2(u_char *data, size_t len);
>> 
>> +uint64_t ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed);
>> +
>> 
>> #endif /* _NGX_MURMURHASH_H_INCLUDED_ */
>> diff -r a39bc74873fa -r 895cea03ac21 src/http/v2/ngx_http_v2.c
>> --- a/src/http/v2/ngx_http_v2.c  Mon Jun 19 14:25:42 2017 +0300
>> +++ b/src/http/v2

Re: [PATCH] HTTP/2: add support for HPACK encoding

2017-07-24 Thread Valentin V. Bartenev 
On Thursday 22 June 2017 18:23:46 Vlad Krasnov via nginx-devel wrote:
> # HG changeset patch
> # User Vlad Krasnov 
> # Date 1498167669 25200
> #  Thu Jun 22 14:41:09 2017 -0700
> # Node ID 895cea03ac21fb18d2c2ba32389cd67dc74ddbd0
> # Parent  a39bc74873faf9e5bea616561b43f6ecc55229f9
> HTTP/2: add support for HPACK encoding
> 
> Add support for full HPACK encoding as per RFC7541.
> This modification improves header compression ratio by 5-10% for the first
> response, and by 40-95% for consequential responses on the connection.
> The implementation is similar to the one used by Cloudflare.
> 

First of all, there's no way for a patch to be committed with so
many style issues.

Please, examine how existing nginx sources are formatted and mimic
this style.



> diff -r a39bc74873fa -r 895cea03ac21 auto/modules
> --- a/auto/modulesMon Jun 19 14:25:42 2017 +0300
> +++ b/auto/modulesThu Jun 22 14:41:09 2017 -0700
> @@ -436,6 +436,10 @@
>  . auto/module
>  fi
>  
> +if [ $HTTP_V2_HPACK_ENC = YES ]; then
> +have=NGX_HTTP_V2_HPACK_ENC . auto/have
> +fi
> +
>  if :; then
>  ngx_module_name=ngx_http_static_module
>  ngx_module_incs=
> diff -r a39bc74873fa -r 895cea03ac21 auto/options
> --- a/auto/optionsMon Jun 19 14:25:42 2017 +0300
> +++ b/auto/optionsThu Jun 22 14:41:09 2017 -0700
> @@ -59,6 +59,7 @@
>  HTTP_GZIP=YES
>  HTTP_SSL=NO
>  HTTP_V2=NO
> +HTTP_V2_HPACK_ENC=NO
>  HTTP_SSI=YES
>  HTTP_POSTPONE=NO
>  HTTP_REALIP=NO
> @@ -221,6 +222,7 @@
>  
>  --with-http_ssl_module)  HTTP_SSL=YES   ;;
>  --with-http_v2_module)   HTTP_V2=YES;;
> +--with-http_v2_hpack_enc)HTTP_V2_HPACK_ENC=YES  ;;

What's the reason for conditional compilation?


>  --with-http_realip_module)   HTTP_REALIP=YES;;
>  --with-http_addition_module) HTTP_ADDITION=YES  ;;
>  --with-http_xslt_module) HTTP_XSLT=YES  ;;
> @@ -430,6 +432,7 @@
>  
>--with-http_ssl_module enable ngx_http_ssl_module
>--with-http_v2_module  enable ngx_http_v2_module
> +  --with-http_v2_hpack_enc   enable ngx_http_v2_hpack_enc
>--with-http_realip_module  enable ngx_http_realip_module
>--with-http_addition_moduleenable ngx_http_addition_module
>--with-http_xslt_moduleenable ngx_http_xslt_module
> diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.c
> --- a/src/core/ngx_murmurhash.c   Mon Jun 19 14:25:42 2017 +0300
> +++ b/src/core/ngx_murmurhash.c   Thu Jun 22 14:41:09 2017 -0700
> @@ -50,3 +50,63 @@
>  
>  return h;
>  }
> +
> +
> +uint64_t
> +ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed)
> +{
> +uint64_t  h, k;
> +
> +h = seed ^ len;
> +
> +while (len >= 8) {
> +k  = data[0];
> +k |= data[1] << 8;
> +k |= data[2] << 16;
> +k |= data[3] << 24;
> +k |= (uint64_t)data[4] << 32;
> +k |= (uint64_t)data[5] << 40;
> +k |= (uint64_t)data[6] << 48;
> +k |= (uint64_t)data[7] << 56;
> +
> +k *= 0xc6a4a7935bd1e995ull;
> +k ^= k >> 47;
> +k *= 0xc6a4a7935bd1e995ull;
> +
> +h ^= k;
> +h *= 0xc6a4a7935bd1e995ull;
> +
> +data += 8;
> +len -= 8;
> +}
> +
> +switch (len) {
> +case 7:
> +h ^= (uint64_t)data[6] << 48;
> +/* fall through */
> +case 6:
> +h ^= (uint64_t)data[5] << 40;
> +/* fall through */
> +case 5:
> +h ^= (uint64_t)data[4] << 32;
> +/* fall through */
> +case 4:
> +h ^= data[3] << 24;
> +/* fall through */
> +case 3:
> +h ^= data[2] << 16;
> +/* fall through */
> +case 2:
> +h ^= data[1] << 8;
> +/* fall through */
> +case 1:
> +h ^= data[0];
> +h *= 0xc6a4a7935bd1e995ull;
> +}
> +
> +h ^= h >> 47;
> +h *= 0xc6a4a7935bd1e995ull;
> +h ^= h >> 47;
> +
> +return h;
> +}
> diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.h
> --- a/src/core/ngx_murmurhash.h   Mon Jun 19 14:25:42 2017 +0300
> +++ b/src/core/ngx_murmurhash.h   Thu Jun 22 14:41:09 2017 -0700
> @@ -15,5 +15,7 @@
>  
>  uint32_t ngx_murmur_hash2(u_char *data, size_t len);
>  
> +uint64_t ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed);
> +
>  
>  #endif /* _NGX_MURMURHASH_H_INCLUDED_ */
> diff -r a39bc74873fa -r 895cea03ac21 src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c   Mon Jun 19 14:25:42 2017 +0300
> +++ b/src/http/v2/ngx_http_v2.c   Thu Jun 22 14:41:09 2017 -0700
> @@ -245,6 +245,8 @@
>  
>  h2c->frame_size = NGX_HTTP_V2_DEFAULT_FRAME_SIZE;
>  
> +h2c->max_hpack_table_size = NGX_HTTP_V2_DEFAULT_HPACK_TABLE_SIZE;
> +
>  h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
>  
>  h2c->pool = ngx_create_

Re: [PATCH] HTTP/2: add support for HPACK encoding

2017-06-23 Thread Valentin V. Bartenev 
On Thursday 22 June 2017 18:23:46 Vlad Krasnov via nginx-devel wrote:
> # HG changeset patch
> # User Vlad Krasnov 
> # Date 1498167669 25200
> #  Thu Jun 22 14:41:09 2017 -0700
> # Node ID 895cea03ac21fb18d2c2ba32389cd67dc74ddbd0
> # Parent  a39bc74873faf9e5bea616561b43f6ecc55229f9
> HTTP/2: add support for HPACK encoding
> 
> Add support for full HPACK encoding as per RFC7541.
> This modification improves header compression ratio by 5-10% for the first
> response, and by 40-95% for consequential responses on the connection.
> The implementation is similar to the one used by Cloudflare.
> 
[..]

Thank you for the patch.

Sorry, I'm busy with other work.  I'll look on it later, as time permits.

  wbr, Valentin V. Bartenev

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH] HTTP/2: add support for HPACK encoding

2017-06-22 Thread Vlad Krasnov via nginx-devel 
# HG changeset patch
# User Vlad Krasnov 
# Date 1498167669 25200
#  Thu Jun 22 14:41:09 2017 -0700
# Node ID 895cea03ac21fb18d2c2ba32389cd67dc74ddbd0
# Parent  a39bc74873faf9e5bea616561b43f6ecc55229f9
HTTP/2: add support for HPACK encoding

Add support for full HPACK encoding as per RFC7541.
This modification improves header compression ratio by 5-10% for the first
response, and by 40-95% for consequential responses on the connection.
The implementation is similar to the one used by Cloudflare.

diff -r a39bc74873fa -r 895cea03ac21 auto/modules
--- a/auto/modules  Mon Jun 19 14:25:42 2017 +0300
+++ b/auto/modules  Thu Jun 22 14:41:09 2017 -0700
@@ -436,6 +436,10 @@
 . auto/module
 fi
 
+if [ $HTTP_V2_HPACK_ENC = YES ]; then
+have=NGX_HTTP_V2_HPACK_ENC . auto/have
+fi
+
 if :; then
 ngx_module_name=ngx_http_static_module
 ngx_module_incs=
diff -r a39bc74873fa -r 895cea03ac21 auto/options
--- a/auto/options  Mon Jun 19 14:25:42 2017 +0300
+++ b/auto/options  Thu Jun 22 14:41:09 2017 -0700
@@ -59,6 +59,7 @@
 HTTP_GZIP=YES
 HTTP_SSL=NO
 HTTP_V2=NO
+HTTP_V2_HPACK_ENC=NO
 HTTP_SSI=YES
 HTTP_POSTPONE=NO
 HTTP_REALIP=NO
@@ -221,6 +222,7 @@
 
 --with-http_ssl_module)  HTTP_SSL=YES   ;;
 --with-http_v2_module)   HTTP_V2=YES;;
+--with-http_v2_hpack_enc)HTTP_V2_HPACK_ENC=YES  ;;
 --with-http_realip_module)   HTTP_REALIP=YES;;
 --with-http_addition_module) HTTP_ADDITION=YES  ;;
 --with-http_xslt_module) HTTP_XSLT=YES  ;;
@@ -430,6 +432,7 @@
 
   --with-http_ssl_module enable ngx_http_ssl_module
   --with-http_v2_module  enable ngx_http_v2_module
+  --with-http_v2_hpack_enc   enable ngx_http_v2_hpack_enc
   --with-http_realip_module  enable ngx_http_realip_module
   --with-http_addition_moduleenable ngx_http_addition_module
   --with-http_xslt_moduleenable ngx_http_xslt_module
diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.c
--- a/src/core/ngx_murmurhash.c Mon Jun 19 14:25:42 2017 +0300
+++ b/src/core/ngx_murmurhash.c Thu Jun 22 14:41:09 2017 -0700
@@ -50,3 +50,63 @@
 
 return h;
 }
+
+
+uint64_t
+ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed)
+{
+uint64_t  h, k;
+
+h = seed ^ len;
+
+while (len >= 8) {
+k  = data[0];
+k |= data[1] << 8;
+k |= data[2] << 16;
+k |= data[3] << 24;
+k |= (uint64_t)data[4] << 32;
+k |= (uint64_t)data[5] << 40;
+k |= (uint64_t)data[6] << 48;
+k |= (uint64_t)data[7] << 56;
+
+k *= 0xc6a4a7935bd1e995ull;
+k ^= k >> 47;
+k *= 0xc6a4a7935bd1e995ull;
+
+h ^= k;
+h *= 0xc6a4a7935bd1e995ull;
+
+data += 8;
+len -= 8;
+}
+
+switch (len) {
+case 7:
+h ^= (uint64_t)data[6] << 48;
+/* fall through */
+case 6:
+h ^= (uint64_t)data[5] << 40;
+/* fall through */
+case 5:
+h ^= (uint64_t)data[4] << 32;
+/* fall through */
+case 4:
+h ^= data[3] << 24;
+/* fall through */
+case 3:
+h ^= data[2] << 16;
+/* fall through */
+case 2:
+h ^= data[1] << 8;
+/* fall through */
+case 1:
+h ^= data[0];
+h *= 0xc6a4a7935bd1e995ull;
+}
+
+h ^= h >> 47;
+h *= 0xc6a4a7935bd1e995ull;
+h ^= h >> 47;
+
+return h;
+}
diff -r a39bc74873fa -r 895cea03ac21 src/core/ngx_murmurhash.h
--- a/src/core/ngx_murmurhash.h Mon Jun 19 14:25:42 2017 +0300
+++ b/src/core/ngx_murmurhash.h Thu Jun 22 14:41:09 2017 -0700
@@ -15,5 +15,7 @@
 
 uint32_t ngx_murmur_hash2(u_char *data, size_t len);
 
+uint64_t ngx_murmur_hash2_64(u_char *data, size_t len, uint64_t seed);
+
 
 #endif /* _NGX_MURMURHASH_H_INCLUDED_ */
diff -r a39bc74873fa -r 895cea03ac21 src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c Mon Jun 19 14:25:42 2017 +0300
+++ b/src/http/v2/ngx_http_v2.c Thu Jun 22 14:41:09 2017 -0700
@@ -245,6 +245,8 @@
 
 h2c->frame_size = NGX_HTTP_V2_DEFAULT_FRAME_SIZE;
 
+h2c->max_hpack_table_size = NGX_HTTP_V2_DEFAULT_HPACK_TABLE_SIZE;
+
 h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
 
 h2c->pool = ngx_create_pool(h2scf->pool_size, h2c->connection->log);
@@ -2018,6 +2020,17 @@
 h2c->frame_size = value;
 break;
 
+case NGX_HTTP_V2_HEADER_TABLE_SIZE_SETTING:
+
+if (value > NGX_HTTP_V2_MAX_HPACK_TABLE_SIZE) {
+h2c->max_hpack_table_size = NGX_HTTP_V2_MAX_HPACK_TABLE_SIZE;
+} else {
+h2c->max_hpack_table_size = value;
+}
+
+h2c->indicate_resize = 1;
+break;
+
 default:
 break;
 }
diff -r a39bc74873fa -r 895cea03ac21 src/http/v2/ngx_http_v2.h
--- a/src/http/v