Hello!

On Thu, Jun 22, 2017 at 01:33:15PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora@google.com>
> # Date 1489618535 25200
> # Wed Mar 15 15:55:35 2017 -0700
> # Node ID 0637acdb51e29e1f68f5f3e762115c702cab4e72
> # Parent 068381014f256ad6e2dc490bacc2529cebbb0462
> Proxy: split configured header names and values.
>
> Previously, each configured header was represented in one of two ways,
> depending on whether or not its value included any variables.
>
> If the value didn't include any variables, then it would be represented
> as as a single script that contained complete header line with HTTP/1.1
> delimiters, i.e.:
>
> "Header: value\r\n"
>
> But if the value included any variables, then it would be represented
> as a series of three scripts: first contained header name and the ": "
> delimiter, second evaluated to header value, and third contained only
> "\r\n", i.e.:
>
> "Header: "
> "$value"
> "\r\n"
>
> This commit changes that, so that each configured header is represented
> as a series of two scripts: first contains only header name, and second
> contains (or evaluates to) only header value, i.e.:
>
> "Header"
> "$value"
>
> or
>
> "Header"
> "value"
>
> This not only makes things more consistent, but also allows header name
> and value to be accessed separately.
>
> Signed-off-by: Piotr Sikora <piotrsikora@google.com>

As suggested during previous review iteration, it would be
interesting to see some benchmarks. Here they are.

With 1000 static headers "proxy_set_header X-Foo foo;" the new
variant is slightly slower (numbers are in requests per second,
higher is better):

$ ministat t.current.n t.patched.n
x t.current.n
+ t.patched.n
+------------------------------------------------------------------------------+
|+ + + + ++ + + x x x x xx x|
| |___________A_____M_____| |_______A__M____| |
+------------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 7 793.883 820.497 811.498 807.94229 10.111563
+ 8 725.285 770.099 759.39 752.68988 15.026166
Difference at 95.0% confidence
-55.2524 +/- 14.5227
-6.83866% +/- 1.7975%
(Student's t, pooled s = 12.991)

It is considerably faster when using variables though, again with
1000 headers "proxy_set_header X-Foo $request_uri;":

$ ministat t.current.vars.n t.patched.vars.n
x t.current.vars.n
+ t.patched.vars.n
+------------------------------------------------------------------------------+
| x ++ + |
|x xxx x + + ++ + |
| |___A__| |______A_M___||
+------------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 8 365.499 415.19 385.856 386.026 14.078621
+ 8 605.499 690.492 681.396 671.80087 27.852872
Difference at 95.0% confidence
285.775 +/- 23.6679
74.03% +/- 6.13117%
(Student's t, pooled s = 22.068)

Given that using variables is much more common, overral the patch
looks clearly beneficial.

> diff -r 068381014f25 -r 0637acdb51e2 src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -1151,6 +1151,7 @@ static ngx_int_t
> ngx_http_proxy_create_request(ngx_http_request_t *r)
> {
> size_t len, uri_len, loc_len, body_len;
> + size_t key_len, val_len;

Style, should be only one type instead:

- size_t len, uri_len, loc_len, body_len;
- size_t key_len, val_len;
+ size_t len, uri_len, loc_len, body_len,
+ key_len, val_len;

[...]

Committed with the above change, thanks.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel