Hello!

On Sat, Jun 03, 2017 at 08:04:00PM -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 ff79d6887fc92d0344eac3e87339583265241e36
> # Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49
> 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

Nitpicking: the ": " delimiter.

> "\r\n", i.e.:
>
> "Header:"

Same here, missed space.

> "$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>

Note that current code is written this way to minimize runtime
processing costs: if we know a header contents in advance, we
simply generate a static string with the header value. If
contents of the header are not known, it is represented so it will
be possible to test if it is empty or not at runtime. All
additional syntax constructions, such as ": " and CRLFs, are
included in the overall script so that required buffer size can be
estimated simply by executing all the len scripts, and the
complete request header can be generated with minimal additional
processing.

I'm not really object the change though, as current implementation
looks overcomplicated, and I'm not really sure the optimization in
question makes any real difference (and if it is, if the
difference is positive). It would be interesting to do some
benchmarking of the old and new variants.

Below some comments about the code. Generic recommendation is to
keep the code similar to the fastcgi/uwsgi/scgi modules.

>
> diff -r 716852cce913 -r ff79d6887fc9 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
> @@ -1144,6 +1144,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;
> uintptr_t escape;
> ngx_buf_t *b;
> ngx_str_t method;
> @@ -1258,10 +1259,17 @@ ngx_http_proxy_create_request(ngx_http_r
> le.flushed = 1;
>
> while (*(uintptr_t *) le.ip) {
> - while (*(uintptr_t *) le.ip) {
> + lcode = *(ngx_http_script_len_code_pt *) le.ip;
> + key_len = lcode(&le);
> +
> + for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
> lcode = *(ngx_http_script_len_code_pt *) le.ip;
> - len += lcode(&le);
> }
> +
> + if (val_len) {
> + len += key_len + sizeof(": ") - 1 + val_len + sizeof(CRLF) - 1;
> + }
> +
> le.ip += sizeof(uintptr_t);
> }
>

Re-shaping this cycle to look more like the one in other modules
would be a plus,

while (*(uintptr_t *) le.ip) {

lcode = *(ngx_http_script_len_code_pt *) le.ip;
key_len = lcode(&le);

for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
lcode = *(ngx_http_script_len_code_pt *) le.ip;
}
le.ip += sizeof(uintptr_t);

if (val_len == 0) {
continue;
}

len += key_len + sizeof(": ") - 1 + val_len + sizeof(CRLF) - 1;
}

> @@ -1363,28 +1371,32 @@ ngx_http_proxy_create_request(ngx_http_r
>
> while (*(uintptr_t *) le.ip) {
> lcode = *(ngx_http_script_len_code_pt *) le.ip;
> -
> - /* skip the header line name length */
> (void) lcode(&le);
>
> - if (*(ngx_http_script_len_code_pt *) le.ip) {
> -
> - for (len = 0; *(uintptr_t *) le.ip; len += lcode(&le)) {
> - lcode = *(ngx_http_script_len_code_pt *) le.ip;
> - }
> -
> - e.skip = (len == sizeof(CRLF) - 1) ? 1 : 0;
> -
> - } else {
> - e.skip = 0;
> + for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
> + lcode = *(ngx_http_script_len_code_pt *) le.ip;
> }
>
> le.ip += sizeof(uintptr_t);
>
> + e.skip = (val_len == 0) ? 1 : 0;
> +
> + code = *(ngx_http_script_code_pt *) e.ip;
> + code((ngx_http_script_engine_t *) &e);
> +
> + if (!e.skip) {
> + *e.pos++ = ':'; *e.pos++ = ' ';
> + }
> +
> while (*(uintptr_t *) e.ip) {
> code = *(ngx_http_script_code_pt *) e.ip;
> code((ngx_http_script_engine_t *) &e);
> }
> +
> + if (!e.skip) {
> + *e.pos++ = CR; *e.pos++ = LF;
> + }
> +
> e.ip += sizeof(uintptr_t);
> }

Same here,

while (*(uintptr_t *) le.ip) {

lcode = *(ngx_http_script_len_code_pt *) le.ip;
(void) lcode(&le);

for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
lcode = *(ngx_http_script_len_code_pt *) le.ip;
}
le.ip += sizeof(uintptr_t);

if (val_len == 0) {
e.skip = 1;

while (*(uintptr_t *) e.ip) {
code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);
}
e.ip += sizeof(uintptr_t);

e.skip = 0;

continue;
}

code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);

*e.pos++ = ':'; *e.pos++ = ' ';

while (*(uintptr_t *) e.ip) {
code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);
}
e.ip += sizeof(uintptr_t);

*e.pos++ = CR; *e.pos++ = LF;
}

>
> @@ -3498,6 +3510,30 @@ ngx_http_proxy_init_headers(ngx_conf_t *
> continue;
> }
>
> + copy = ngx_array_push_n(headers->lengths,
> + sizeof(ngx_http_script_copy_code_t));
> + if (copy == NULL) {
> + return NGX_ERROR;
> + }
> +
> + copy->code = (ngx_http_script_code_pt) ngx_http_script_copy_len_code;
> + copy->len = src[i].key.len;
> +
> + size = (sizeof(ngx_http_script_copy_code_t)
> + + src[i].key.len + sizeof(uintptr_t) - 1)
> + & ~(sizeof(uintptr_t) - 1);

The last line is indented as if "&" operation is in parentheses.
It would be better to use the same form as in the fastcgi module:

size = (sizeof(ngx_http_script_copy_code_t)
+ src[i].key.len + sizeof(uintptr_t) - 1)
& ~(sizeof(uintptr_t) - 1);

> +
> + copy = ngx_array_push_n(headers->values, size);
> + if (copy == NULL) {
> + return NGX_ERROR;
> + }
> +
> + copy->code = ngx_http_script_copy_code;
> + copy->len = src[i].key.len;
> +
> + p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
> + ngx_memcpy(p, src[i].key.data, src[i].key.len);
> +
> if (ngx_http_script_variables_count(&src[i].value) == 0) {
> copy = ngx_array_push_n(headers->lengths,
> sizeof(ngx_http_script_copy_code_t));
> @@ -3507,14 +3543,10 @@ ngx_http_proxy_init_headers(ngx_conf_t *
>
> copy->code = (ngx_http_script_code_pt)
> ngx_http_script_copy_len_code;
> - copy->len = src[i].key.len + sizeof(": ") - 1
> - + src[i].value.len + sizeof(CRLF) - 1;
> -
> + copy->len = src[i].value.len;
>
> size = (sizeof(ngx_http_script_copy_code_t)
> - + src[i].key.len + sizeof(": ") - 1
> - + src[i].value.len + sizeof(CRLF) - 1
> - + sizeof(uintptr_t) - 1)
> + + src[i].value.len + sizeof(uintptr_t) - 1)
> & ~(sizeof(uintptr_t) - 1);
>
> copy = ngx_array_push_n(headers->values, size);
> @@ -3523,45 +3555,12 @@ ngx_http_proxy_init_headers(ngx_conf_t *
> }
>
> copy->code = ngx_http_script_copy_code;
> - copy->len = src[i].key.len + sizeof(": ") - 1
> - + src[i].value.len + sizeof(CRLF) - 1;
> + copy->len = src[i].value.len;
>
> p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
> -
> - p = ngx_cpymem(p, src[i].key.data, src[i].key.len);
> - *p++ = ':'; *p++ = ' ';
> - p = ngx_cpymem(p, src[i].value.data, src[i].value.len);
> - *p++ = CR; *p = LF;
> + ngx_memcpy(p, src[i].value.data, src[i].value.len);
>

It makes a little sense to preserve special processing for the "no
variables" case here with . Just calling the ngx_http_script_compile()
function as in the "else" clause will do the same.

> } else {
> - copy = ngx_array_push_n(headers->lengths,
> - sizeof(ngx_http_script_copy_code_t));
> - if (copy == NULL) {
> - return NGX_ERROR;
> - }
> -
> - copy->code = (ngx_http_script_code_pt)
> - ngx_http_script_copy_len_code;
> - copy->len = src[i].key.len + sizeof(": ") - 1;
> -
> -
> - size = (sizeof(ngx_http_script_copy_code_t)
> - + src[i].key.len + sizeof(": ") - 1 + sizeof(uintptr_t) - 1)
> - & ~(sizeof(uintptr_t) - 1);
> -
> - copy = ngx_array_push_n(headers->values, size);
> - if (copy == NULL) {
> - return NGX_ERROR;
> - }
> -
> - copy->code = ngx_http_script_copy_code;
> - copy->len = src[i].key.len + sizeof(": ") - 1;
> -
> - p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
> - p = ngx_cpymem(p, src[i].key.data, src[i].key.len);
> - *p++ = ':'; *p = ' ';
> -
> -
> ngx_memzero(&sc, sizeof(ngx_http_script_compile_t));
>
> sc.cf = cf;
> @@ -3573,33 +3572,6 @@ ngx_http_proxy_init_headers(ngx_conf_t *
> if (ngx_http_script_compile(&sc) != NGX_OK) {
> return NGX_ERROR;
> }
> -
> -
> - copy = ngx_array_push_n(headers->lengths,
> - sizeof(ngx_http_script_copy_code_t));
> - if (copy == NULL) {
> - return NGX_ERROR;
> - }
> -
> - copy->code = (ngx_http_script_code_pt)
> - ngx_http_script_copy_len_code;
> - copy->len = sizeof(CRLF) - 1;
> -
> -
> - size = (sizeof(ngx_http_script_copy_code_t)
> - + sizeof(CRLF) - 1 + sizeof(uintptr_t) - 1)
> - & ~(sizeof(uintptr_t) - 1);
> -
> - copy = ngx_array_push_n(headers->values, size);
> - if (copy == NULL) {
> - return NGX_ERROR;
> - }
> -
> - copy->code = ngx_http_script_copy_code;
> - copy->len = sizeof(CRLF) - 1;
> -
> - p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
> - *p++ = CR; *p = LF;
> }
>
> code = ngx_array_push_n(headers->lengths, sizeof(uintptr_t));

Full patch with the above comments below:

diff --git a/src/http/modules/ngx_http_proxy_module.c b/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
@@ -1259,18 +1259,20 @@ ngx_http_proxy_create_request(ngx_http_r
le.flushed = 1;

while (*(uintptr_t *) le.ip) {
+
lcode = *(ngx_http_script_len_code_pt *) le.ip;
key_len = lcode(&le);

for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
lcode = *(ngx_http_script_len_code_pt *) le.ip;
}
-
- if (val_len) {
- len += key_len + sizeof(": ") - 1 + val_len + sizeof(CRLF) - 1;
+ le.ip += sizeof(uintptr_t);
+
+ if (val_len == 0) {
+ continue;
}

- le.ip += sizeof(uintptr_t);
+ len += key_len + sizeof(": ") - 1 + val_len + sizeof(CRLF) - 1;
}


@@ -1370,34 +1372,41 @@ ngx_http_proxy_create_request(ngx_http_r
le.ip = headers->lengths->elts;

while (*(uintptr_t *) le.ip) {
+
lcode = *(ngx_http_script_len_code_pt *) le.ip;
(void) lcode(&le);

for (val_len = 0; *(uintptr_t *) le.ip; val_len += lcode(&le)) {
lcode = *(ngx_http_script_len_code_pt *) le.ip;
}
-
le.ip += sizeof(uintptr_t);

- e.skip = (val_len == 0) ? 1 : 0;
+ if (val_len == 0) {
+ e.skip = 1;
+
+ while (*(uintptr_t *) e.ip) {
+ code = *(ngx_http_script_code_pt *) e.ip;
+ code((ngx_http_script_engine_t *) &e);
+ }
+ e.ip += sizeof(uintptr_t);
+
+ e.skip = 0;
+
+ continue;
+ }

code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);

- if (!e.skip) {
- *e.pos++ = ':'; *e.pos++ = ' ';
- }
+ *e.pos++ = ':'; *e.pos++ = ' ';

while (*(uintptr_t *) e.ip) {
code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);
}
-
- if (!e.skip) {
- *e.pos++ = CR; *e.pos++ = LF;
- }
-
e.ip += sizeof(uintptr_t);
+
+ *e.pos++ = CR; *e.pos++ = LF;
}

b->last = e.pos;
@@ -3521,7 +3530,7 @@ ngx_http_proxy_init_headers(ngx_conf_t *

size = (sizeof(ngx_http_script_copy_code_t)
+ src[i].key.len + sizeof(uintptr_t) - 1)
- & ~(sizeof(uintptr_t) - 1);
+ & ~(sizeof(uintptr_t) - 1);

copy = ngx_array_push_n(headers->values, size);
if (copy == NULL) {
@@ -3534,44 +3543,16 @@ ngx_http_proxy_init_headers(ngx_conf_t *
p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
ngx_memcpy(p, src[i].key.data, src[i].key.len);

- if (ngx_http_script_variables_count(&src[i].value) == 0) {
- copy = ngx_array_push_n(headers->lengths,
- sizeof(ngx_http_script_copy_code_t));
- if (copy == NULL) {
- return NGX_ERROR;
- }
-
- copy->code = (ngx_http_script_code_pt)
- ngx_http_script_copy_len_code;
- copy->len = src[i].value.len;
-
- size = (sizeof(ngx_http_script_copy_code_t)
- + src[i].value.len + sizeof(uintptr_t) - 1)
- & ~(sizeof(uintptr_t) - 1);
-
- copy = ngx_array_push_n(headers->values, size);
- if (copy == NULL) {
- return NGX_ERROR;
- }
-
- copy->code = ngx_http_script_copy_code;
- copy->len = src[i].value.len;
-
- p = (u_char *) copy + sizeof(ngx_http_script_copy_code_t);
- ngx_memcpy(p, src[i].value.data, src[i].value.len);
-
- } else {
- ngx_memzero(&sc, sizeof(ngx_http_script_compile_t));
-
- sc.cf = cf;
- sc.source = &src[i].value;
- sc.flushes = &headers->flushes;
- sc.lengths = &headers->lengths;
- sc.values = &headers->values;
-
- if (ngx_http_script_compile(&sc) != NGX_OK) {
- return NGX_ERROR;
- }
+ ngx_memzero(&sc, sizeof(ngx_http_script_compile_t));
+
+ sc.cf = cf;
+ sc.source = &src[i].value;
+ sc.flushes = &headers->flushes;
+ sc.lengths = &headers->lengths;
+ sc.values = &headers->values;
+
+ if (ngx_http_script_compile(&sc) != NGX_OK) {
+ return NGX_ERROR;
}

code = ngx_array_push_n(headers->lengths, sizeof(uintptr_t));

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