Hello! On Sat, Jun 03, 2017 at 08:04:00PM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch > # User Piotr Sikora <piotrsik...@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 <piotrsik...@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