Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Michal Karm]

On 11/17/2015 02:28 PM, Jim Jagielski wrote:

Agreed... if we should optimize, then focusing on ap_proxy_port_of_scheme(),
which is part of the actual API, is likely best.


On Nov 17, 2015, at 8:20 AM, Yann Ylavic  wrote:

On Tue, Nov 17, 2015 at 2:12 PM, Jim Jagielski  wrote:

I would propose that if the below is NOT the cause, then the
old version remain. There is a lot to be said for simplicity
and clarity.

There is still a (per request) call to ap_proxy_port_of_scheme() in
ap_proxy_determine_connection() we can't avoid (AFAICT), so it is
worth optimize it anyway IMO.


Plus, the whole reason for ap_proxy_port_of_scheme() was
to avoid the sorts of special numbers the below "hides"
in various locations.

Agreed, the optimization in ap_proxy_port_of_scheme() only is probably better.




Hi guys, FYI,
the patch suggested by Yenn [1][2] did not help the performance
results in any substantial capacity. The difference between having and not 
having

 if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
 uri.port = 0;
 }

in proxy_util.c is still at least 30% performance loss, gain respectively.

The test case is having hundreds of different application contexts
being accessed by hundreds of concurrent clients.

Thanks for comments.

Cheers

[1] 
http://mail-archives.apache.org/mod_mbox/httpd-dev/201511.mbox/%3CCAKQ1sVP-ZO%3DC8kUt_%2BtW7%2Bh5A%3DyjDuKhEE9ah9shF1xRQVciRw%40mail.gmail.com%3E
[2] 
http://mail-archives.apache.org/mod_mbox/httpd-dev/201511.mbox/%3CCAKQ1sVO_-Shud7EXrCBn-KhjdPdNVk96Npwxeeb6Lk5n0TE1KA%40mail.gmail.com%3E


Michal Karm Babacek

--
Sent from my Hosaka Ono-Sendai Cyberspace 7

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Yann Ylavic]

On Wed, Nov 18, 2015 at 1:58 PM, Michal Karm  wrote:
>
> the patch suggested by Yenn [1][2] did not help the performance
> results in any substantial capacity. The difference between having and not
> having
>
>  if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
>  uri.port = 0;
>  }
>
> in proxy_util.c is still at least 30% performance loss, gain respectively.

The code above is load time only (i.e. not per request), so it's not
this particular ap_proxy_port_of_scheme() which eats cycles, but
rather the fact that the proxy worker URL (name) is now canonicalized
at startup (not including the port if it is well known) and thus when
that URL is parsed/used at runtime (per request) in
ap_proxy_determine_connection(), the backend's port needs to be
determined using ap_proxy_port_of_scheme().

I thought this latter call was the culprit for the increased cycles,
but it seems not (the new version should be quite more efficient than
the previous code to find a know port associated to a scheme).
So I'm a bit lost here...

Can you confirm that reverting this only commit (r1650655) from 2.4.17
fixes the performance loss?

Also, which proxy module(s) is(are) used, http/ajp/...?
How are the backends ports declared in ProxyPass/BalancerMember
directives, using the default port for the scheme (ie. 80/443 for
http/s, 8009 for ajp, ...) or with a custom port or else no port at
all?

Regards,
Yann.

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Jim Jagielski]

> On Nov 16, 2015, at 12:38 PM, Yann Ylavic  wrote:
> 
> On Mon, Nov 16, 2015 at 5:53 PM, jean-frederic clere  
> wrote:
>> On 01/09/2015 09:37 PM, jaillet...@apache.org wrote:
>>> 
>>> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1650655=1650654=1650655=diff
>>> ==
>>> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
>>> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Jan  9 
>>> 20:37:50 2015
>>> @@ -1683,6 +1683,9 @@ PROXY_DECLARE(char *) ap_proxy_define_wo
>>> 
>>> memset(wshared, 0, sizeof(proxy_worker_shared));
>>> 
>>> +if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
>>> +uri.port = 0;
>>> +}
>> 
>> 
>> I must be doing something wrong but the above seems to hurt by 30% more
>> CPU one for the tests we are doing, any hints?
> 
> Looks like you are quite heavy testing this path...
> 
> Could you please try the following patch and see if it helps?
> 
> Index: modules/proxy/proxy_util.c
> ===
> --- modules/proxy/proxy_util.c(revision 1714617)
> +++ modules/proxy/proxy_util.c(working copy)
> @@ -3663,35 +3663,51 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade(apr_bucke
> return OK;
> }
> 
> -/* Fill in unknown schemes from apr_uri_port_of_scheme() */
> 
> -typedef struct proxy_schemes_t {
> -const char *name;
> -apr_port_t default_port;
> -} proxy_schemes_t ;
> -
> -static proxy_schemes_t pschemes[] =
> -{
> -{"fcgi", 8000},
> -{"ajp",  AJP13_DEF_PORT},
> -{"scgi", SCGI_DEF_PORT},
> -{ NULL, 0x } /* unknown port */
> -};
> -
> PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
> {
> if (scheme) {
> -apr_port_t port;
> -if ((port = apr_uri_port_of_scheme(scheme)) != 0) {
> -return port;
> -} else {
> -proxy_schemes_t *pscheme;
> -for (pscheme = pschemes; pscheme->name != NULL; ++pscheme) {
> -if (strcasecmp(scheme, pscheme->name) == 0) {
> -return pscheme->default_port;
> +/* Fill in unknown schemes from apr_uri_port_of_scheme(),
> + * and try a fast path for common schemes.
> + */
> +switch (ap_tolower(*scheme)) {
> +case 'a':
> +if (strcasecmp(scheme + 1, "jp") == 0) {
> +return AJP13_DEF_PORT;
> +}
> +break;
> +case 'f':
> +if (strcasecmp(scheme + 1, "cgi") == 0) {
> +return 8000;
> +}
> +if (strcasecmp(scheme + 1, "tp") == 0) {
> +return APR_URI_FTP_DEFAULT_PORT;
> +}
> +break;
> +case 'h':
> +if (strncasecmp(scheme + 1, "ttp", 4) == 0) {

Should be 3, shouldn't it?

> +int s4 = ap_tolower(scheme[4]);
> +if (!s4) {
> +return APR_URI_HTTP_DEFAULT_PORT;
> }
> +else if (s4 == 's' && !scheme[5]) {
> +return APR_URI_HTTPS_DEFAULT_PORT;
> +}
> }

If we are really trying to optimize the heck out of this, certainly
we should not ap_tolower s4 until we know it's not '\0'.

> +break;
> +case 'n':
> +if (strcasecmp(scheme + 1, "ntp") == 0) {
> +return APR_URI_NNTP_DEFAULT_PORT;
> +}
> +break;
> +case 's':
> +if (strcasecmp(scheme + 1, "cgi") == 0) {
> +return SCGI_DEF_PORT;
> +}
> +break;
> }
> +
> +return apr_uri_port_of_scheme(scheme);
> }
> return 0;
> }
> --
> 
> Regards,
> Yann.

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Yann Ylavic]

On Tue, Nov 17, 2015 at 1:34 PM, Jim Jagielski  wrote:
>
>> On Nov 16, 2015, at 12:38 PM, Yann Ylavic  wrote:
>>
>> +case 'h':
>> +if (strncasecmp(scheme + 1, "ttp", 4) == 0) {
>
> Should be 3, shouldn't it?

Yes, I corrected this already in a previous message.

>
>> +int s4 = ap_tolower(scheme[4]);
>> +if (!s4) {
>> +return APR_URI_HTTP_DEFAULT_PORT;
>> }
>> +else if (s4 == 's' && !scheme[5]) {
>> +return APR_URI_HTTPS_DEFAULT_PORT;
>> +}
>> }
>
> If we are really trying to optimize the heck out of this, certainly
> we should not ap_tolower s4 until we know it's not '\0'.

Agreed, I have a final patch already which addresses this, just wanted
to see (quickly) if that was the real issue.

The patch I'd propose to commit is something like:

Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1714617)
+++ modules/proxy/proxy_util.c(working copy)
@@ -3663,37 +3663,61 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade(apr_bucke
 return OK;
 }

-/* Fill in unknown schemes from apr_uri_port_of_scheme() */

-typedef struct proxy_schemes_t {
-const char *name;
-apr_port_t default_port;
-} proxy_schemes_t ;
-
-static proxy_schemes_t pschemes[] =
+PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
 {
-{"fcgi", 8000},
-{"ajp",  AJP13_DEF_PORT},
-{"scgi", SCGI_DEF_PORT},
-{ NULL, 0x } /* unknown port */
-};
+if (!scheme) {
+return 0;
+}

-PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
-{
-if (scheme) {
-apr_port_t port;
-if ((port = apr_uri_port_of_scheme(scheme)) != 0) {
-return port;
-} else {
-proxy_schemes_t *pscheme;
-for (pscheme = pschemes; pscheme->name != NULL; ++pscheme) {
-if (strcasecmp(scheme, pscheme->name) == 0) {
-return pscheme->default_port;
-}
+/* Fill in unknown schemes from apr_uri_port_of_scheme(),
+ * and try a fast path for common ones.
+ */
+switch (*scheme) {
+case 'a':
+case 'A':
+if (strcasecmp(scheme + 1, "jp") == 0) {
+return AJP13_DEF_PORT;
+}
+break;
+
+case 'f':
+case 'F':
+if (strcasecmp(scheme + 1, "cgi") == 0) {
+return AP_FCGI_DEF_PORT;
+}
+if (strcasecmp(scheme + 1, "tp") == 0) {
+return APR_URI_FTP_DEFAULT_PORT;
+}
+break;
+
+case 'h':
+case 'H':
+if (strncasecmp(scheme + 1, "ttp", 3) == 0) {
+if (!scheme[4]) {
+return APR_URI_HTTP_DEFAULT_PORT;
 }
+else if (ap_tolower(scheme[4]) == 's' && !scheme[5]) {
+return APR_URI_HTTPS_DEFAULT_PORT;
+}
 }
+break;
+
+case 'n':
+case 'N':
+if (strcasecmp(scheme + 1, "ntp") == 0) {
+return APR_URI_NNTP_DEFAULT_PORT;
+}
+break;
+
+case 's':
+case 'S':
+if (strcasecmp(scheme + 1, "cgi") == 0) {
+return SCGI_DEF_PORT;
+}
+break;
 }
-return 0;
+return apr_uri_port_of_scheme(scheme);
 }

 void proxy_util_register_hooks(apr_pool_t *p)
Index: include/util_fcgi.h
===
--- include/util_fcgi.h(revision 1714617)
+++ include/util_fcgi.h(working copy)
@@ -72,6 +72,11 @@ typedef struct {
 #define AP_FCGI_VERSION_1 1

 /**
+ * Default port for FastCGI backends.
+ */
+#define AP_FCGI_DEF_PORT 8000
+
+/**
  * Possible values for the type field of ap_fcgi_header
  */
 #define AP_FCGI_BEGIN_REQUEST   1
Index: modules/proxy/mod_proxy_ajp.c
===
--- modules/proxy/mod_proxy_ajp.c(revision 1714617)
+++ modules/proxy/mod_proxy_ajp.c(working copy)
@@ -48,7 +48,7 @@ static int proxy_ajp_canon(request_rec *r, char *u
  * do syntactic check.
  * We break the URL into host, port, path, search
  */
-port = def_port = ap_proxy_port_of_scheme("ajp");
+port = def_port = AJP13_DEF_PORT;

 err = ap_proxy_canon_netloc(r->pool, , NULL, NULL, , );
 if (err) {
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c(revision 1714617)
+++ modules/proxy/mod_proxy_fcgi.c(working copy)
@@ -46,7 +46,7 @@ static int proxy_fcgi_canon(request_rec *r, char *
 return DECLINED;
 }

-port = def_port = ap_proxy_port_of_scheme("fcgi");
+port = def_port = AP_FCGI_DEF_PORT;

 ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
  "canonicalising URL %s", url);
Index: 

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Yann Ylavic]

On Tue, Nov 17, 2015 at 2:12 PM, Jim Jagielski  wrote:
> I would propose that if the below is NOT the cause, then the
> old version remain. There is a lot to be said for simplicity
> and clarity.

There is still a (per request) call to ap_proxy_port_of_scheme() in
ap_proxy_determine_connection() we can't avoid (AFAICT), so it is
worth optimize it anyway IMO.

>
> Plus, the whole reason for ap_proxy_port_of_scheme() was
> to avoid the sorts of special numbers the below "hides"
> in various locations.

Agreed, the optimization in ap_proxy_port_of_scheme() only is probably better.

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Jim Jagielski]

Agreed... if we should optimize, then focusing on ap_proxy_port_of_scheme(),
which is part of the actual API, is likely best.

> On Nov 17, 2015, at 8:20 AM, Yann Ylavic  wrote:
> 
> On Tue, Nov 17, 2015 at 2:12 PM, Jim Jagielski  wrote:
>> I would propose that if the below is NOT the cause, then the
>> old version remain. There is a lot to be said for simplicity
>> and clarity.
> 
> There is still a (per request) call to ap_proxy_port_of_scheme() in
> ap_proxy_determine_connection() we can't avoid (AFAICT), so it is
> worth optimize it anyway IMO.
> 
>> 
>> Plus, the whole reason for ap_proxy_port_of_scheme() was
>> to avoid the sorts of special numbers the below "hides"
>> in various locations.
> 
> Agreed, the optimization in ap_proxy_port_of_scheme() only is probably better.

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Jim Jagielski]

I would propose that if the below is NOT the cause, then the
old version remain. There is a lot to be said for simplicity
and clarity.

Plus, the whole reason for ap_proxy_port_of_scheme() was
to avoid the sorts of special numbers the below "hides"
in various locations.

> On Nov 17, 2015, at 7:41 AM, Yann Ylavic  wrote:
> 
> On Tue, Nov 17, 2015 at 1:34 PM, Jim Jagielski  wrote:
>> 
>>> On Nov 16, 2015, at 12:38 PM, Yann Ylavic  wrote:
>>> 
>>> +case 'h':
>>> +if (strncasecmp(scheme + 1, "ttp", 4) == 0) {
>> 
>> Should be 3, shouldn't it?
> 
> Yes, I corrected this already in a previous message.
> 
>> 
>>> +int s4 = ap_tolower(scheme[4]);
>>> +if (!s4) {
>>> +return APR_URI_HTTP_DEFAULT_PORT;
>>>}
>>> +else if (s4 == 's' && !scheme[5]) {
>>> +return APR_URI_HTTPS_DEFAULT_PORT;
>>> +}
>>>}
>> 
>> If we are really trying to optimize the heck out of this, certainly
>> we should not ap_tolower s4 until we know it's not '\0'.
> 
> Agreed, I have a final patch already which addresses this, just wanted
> to see (quickly) if that was the real issue.
> 
> The patch I'd propose to commit is something like:
> 
> Index: modules/proxy/proxy_util.c
> ===
> --- modules/proxy/proxy_util.c(revision 1714617)
> +++ modules/proxy/proxy_util.c(working copy)
> @@ -3663,37 +3663,61 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade(apr_bucke
> return OK;
> }
> 
> -/* Fill in unknown schemes from apr_uri_port_of_scheme() */
> 
> -typedef struct proxy_schemes_t {
> -const char *name;
> -apr_port_t default_port;
> -} proxy_schemes_t ;
> -
> -static proxy_schemes_t pschemes[] =
> +PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
> {
> -{"fcgi", 8000},
> -{"ajp",  AJP13_DEF_PORT},
> -{"scgi", SCGI_DEF_PORT},
> -{ NULL, 0x } /* unknown port */
> -};
> +if (!scheme) {
> +return 0;
> +}
> 
> -PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
> -{
> -if (scheme) {
> -apr_port_t port;
> -if ((port = apr_uri_port_of_scheme(scheme)) != 0) {
> -return port;
> -} else {
> -proxy_schemes_t *pscheme;
> -for (pscheme = pschemes; pscheme->name != NULL; ++pscheme) {
> -if (strcasecmp(scheme, pscheme->name) == 0) {
> -return pscheme->default_port;
> -}
> +/* Fill in unknown schemes from apr_uri_port_of_scheme(),
> + * and try a fast path for common ones.
> + */
> +switch (*scheme) {
> +case 'a':
> +case 'A':
> +if (strcasecmp(scheme + 1, "jp") == 0) {
> +return AJP13_DEF_PORT;
> +}
> +break;
> +
> +case 'f':
> +case 'F':
> +if (strcasecmp(scheme + 1, "cgi") == 0) {
> +return AP_FCGI_DEF_PORT;
> +}
> +if (strcasecmp(scheme + 1, "tp") == 0) {
> +return APR_URI_FTP_DEFAULT_PORT;
> +}
> +break;
> +
> +case 'h':
> +case 'H':
> +if (strncasecmp(scheme + 1, "ttp", 3) == 0) {
> +if (!scheme[4]) {
> +return APR_URI_HTTP_DEFAULT_PORT;
> }
> +else if (ap_tolower(scheme[4]) == 's' && !scheme[5]) {
> +return APR_URI_HTTPS_DEFAULT_PORT;
> +}
> }
> +break;
> +
> +case 'n':
> +case 'N':
> +if (strcasecmp(scheme + 1, "ntp") == 0) {
> +return APR_URI_NNTP_DEFAULT_PORT;
> +}
> +break;
> +
> +case 's':
> +case 'S':
> +if (strcasecmp(scheme + 1, "cgi") == 0) {
> +return SCGI_DEF_PORT;
> +}
> +break;
> }
> -return 0;
> +return apr_uri_port_of_scheme(scheme);
> }
> 
> void proxy_util_register_hooks(apr_pool_t *p)
> Index: include/util_fcgi.h
> ===
> --- include/util_fcgi.h(revision 1714617)
> +++ include/util_fcgi.h(working copy)
> @@ -72,6 +72,11 @@ typedef struct {
> #define AP_FCGI_VERSION_1 1
> 
> /**
> + * Default port for FastCGI backends.
> + */
> +#define AP_FCGI_DEF_PORT 8000
> +
> +/**
>  * Possible values for the type field of ap_fcgi_header
>  */
> #define AP_FCGI_BEGIN_REQUEST   1
> Index: modules/proxy/mod_proxy_ajp.c
> ===
> --- modules/proxy/mod_proxy_ajp.c(revision 1714617)
> +++ modules/proxy/mod_proxy_ajp.c(working copy)
> @@ -48,7 +48,7 @@ static int proxy_ajp_canon(request_rec *r, char *u
>  * do syntactic check.
>  * We break the URL into host, port, path, search
>  */
> -port = def_port = ap_proxy_port_of_scheme("ajp");
> +port = def_port = 

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[jean-frederic clere]

On 01/09/2015 09:37 PM, jaillet...@apache.org wrote:
> Author: jailletc36
> Date: Fri Jan  9 20:37:50 2015
> New Revision: 1650655
> 
> URL: http://svn.apache.org/r1650655
> Log:
> Merge r1644503 from trunk
> 
>* mod_proxy_ajp: Fix handling of the default port (8009) in the
>  ProxyPass and  configurations.  PR 57259.
> 
> Submitted by: ylavic
> Reviewed by: ylavic, jim, covener
> Backported by: jailletc36
> 
> Modified:
> httpd/httpd/branches/2.4.x/CHANGES
> httpd/httpd/branches/2.4.x/STATUS
> httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> 
> Modified: httpd/httpd/branches/2.4.x/CHANGES
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1650655=1650654=1650655=diff
> ==
> --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
> +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Fri Jan  9 20:37:50 2015
> @@ -22,6 +22,9 @@ Changes with Apache 2.4.11
>   request headers earlier.  Adds "MergeTrailers" directive to restore
>   legacy behavior.  [Edward Lu, Yann Ylavic, Joe Orton, Eric Covener]
>  
> +  *) mod_proxy_ajp: Fix handling of the default port (8009) in the
> + ProxyPass and  configurations.  PR 57259.  [Yann Ylavic].
> +
>*) mpm_event: Avoid a possible use after free when notifying the end of
>   connection during lingering close.  PR 57268.  [Eric Covener, Yann 
> Ylavic]
>  
> 
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1650655=1650654=1650655=diff
> ==
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Fri Jan  9 20:37:50 2015
> @@ -104,12 +104,6 @@ RELEASE SHOWSTOPPERS:
>  PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>[ start all new proposals below, under PATCHES PROPOSED. ]
>  
> -   * mod_proxy_ajp: Fix handling of the default port (8009) in the
> - ProxyPass and  configurations.  PR 57259.
> - trunk patch: http://svn.apache.org/r1644503
> - 2.4.x patch: trunk works (module CHANGES)
> - +1: ylavic, jim, covener
> -
> * mod_ssl: Check if we are having an SSL connection before looking up SSL
>related variables during expression evaluation to avoid a 
> crash.
>If not return NULL as ssl_var_lookup_ssl does by default.  PR 
> 57070
> 
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1650655=1650654=1650655=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Jan  9 20:37:50 
> 2015
> @@ -1683,6 +1683,9 @@ PROXY_DECLARE(char *) ap_proxy_define_wo
>  
>  memset(wshared, 0, sizeof(proxy_worker_shared));
>  
> +if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
> +uri.port = 0;
> +}


I must be doing something wrong but the above seems to hurt by 30% more
CPU one for the tests we are doing, any hints?

Cheers

Jean-Frederic

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Jim Jagielski]

Are you logging DEBUG?

> On Nov 16, 2015, at 11:53 AM, jean-frederic clere  wrote:
> 
> On 01/09/2015 09:37 PM, jaillet...@apache.org wrote:
>> Author: jailletc36
>> Date: Fri Jan  9 20:37:50 2015
>> New Revision: 1650655
>> 
>> URL: http://svn.apache.org/r1650655
>> Log:
>> Merge r1644503 from trunk
>> 
>>   * mod_proxy_ajp: Fix handling of the default port (8009) in the
>> ProxyPass and  configurations.  PR 57259.
>> 
>> Submitted by: ylavic
>> Reviewed by: ylavic, jim, covener
>> Backported by: jailletc36
>> 
>> Modified:
>>httpd/httpd/branches/2.4.x/CHANGES
>>httpd/httpd/branches/2.4.x/STATUS
>>httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>> 
>> Modified: httpd/httpd/branches/2.4.x/CHANGES
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1650655=1650654=1650655=diff
>> ==
>> --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
>> +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Fri Jan  9 20:37:50 2015
>> @@ -22,6 +22,9 @@ Changes with Apache 2.4.11
>>  request headers earlier.  Adds "MergeTrailers" directive to restore
>>  legacy behavior.  [Edward Lu, Yann Ylavic, Joe Orton, Eric Covener]
>> 
>> +  *) mod_proxy_ajp: Fix handling of the default port (8009) in the
>> + ProxyPass and  configurations.  PR 57259.  [Yann Ylavic].
>> +
>>   *) mpm_event: Avoid a possible use after free when notifying the end of
>>  connection during lingering close.  PR 57268.  [Eric Covener, Yann 
>> Ylavic]
>> 
>> 
>> Modified: httpd/httpd/branches/2.4.x/STATUS
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1650655=1650654=1650655=diff
>> ==
>> --- httpd/httpd/branches/2.4.x/STATUS (original)
>> +++ httpd/httpd/branches/2.4.x/STATUS Fri Jan  9 20:37:50 2015
>> @@ -104,12 +104,6 @@ RELEASE SHOWSTOPPERS:
>> PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>>   [ start all new proposals below, under PATCHES PROPOSED. ]
>> 
>> -   * mod_proxy_ajp: Fix handling of the default port (8009) in the
>> - ProxyPass and  configurations.  PR 57259.
>> - trunk patch: http://svn.apache.org/r1644503
>> - 2.4.x patch: trunk works (module CHANGES)
>> - +1: ylavic, jim, covener
>> -
>>* mod_ssl: Check if we are having an SSL connection before looking up SSL
>>   related variables during expression evaluation to avoid a 
>> crash.
>>   If not return NULL as ssl_var_lookup_ssl does by default.  PR 
>> 57070
>> 
>> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1650655=1650654=1650655=diff
>> ==
>> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Jan  9 
>> 20:37:50 2015
>> @@ -1683,6 +1683,9 @@ PROXY_DECLARE(char *) ap_proxy_define_wo
>> 
>> memset(wshared, 0, sizeof(proxy_worker_shared));
>> 
>> +if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
>> +uri.port = 0;
>> +}
> 
> 
> I must be doing something wrong but the above seems to hurt by 30% more
> CPU one for the tests we are doing, any hints?
> 
> Cheers
> 
> Jean-Frederic

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Yann Ylavic]

On Mon, Nov 16, 2015 at 6:38 PM, Yann Ylavic  wrote:
>
> Could you please try the following patch and see if it helps?
>
> Index: modules/proxy/proxy_util.c
> ===
> --- modules/proxy/proxy_util.c(revision 1714617)
> +++ modules/proxy/proxy_util.c(working copy)
[]
>  PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
>  {
>  if (scheme) {
[]
> +switch (ap_tolower(*scheme)) {
[]
> +case 'h':
> +if (strncasecmp(scheme + 1, "ttp", 4) == 0) {

Actually this should be strncasecmp(scheme + 1, "ttp", 3)...

> +int s4 = ap_tolower(scheme[4]);
> +if (!s4) {
> +return APR_URI_HTTP_DEFAULT_PORT;
>  }
> +else if (s4 == 's' && !scheme[5]) {
> +return APR_URI_HTTPS_DEFAULT_PORT;
> +}
>  }
> +break;

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[Yann Ylavic]

On Mon, Nov 16, 2015 at 5:53 PM, jean-frederic clere  wrote:
> On 01/09/2015 09:37 PM, jaillet...@apache.org wrote:
>>
>> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1650655=1650654=1650655=diff
>> ==
>> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Jan  9 
>> 20:37:50 2015
>> @@ -1683,6 +1683,9 @@ PROXY_DECLARE(char *) ap_proxy_define_wo
>>
>>  memset(wshared, 0, sizeof(proxy_worker_shared));
>>
>> +if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
>> +uri.port = 0;
>> +}
>
>
> I must be doing something wrong but the above seems to hurt by 30% more
> CPU one for the tests we are doing, any hints?

Looks like you are quite heavy testing this path...

Could you please try the following patch and see if it helps?

Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1714617)
+++ modules/proxy/proxy_util.c(working copy)
@@ -3663,35 +3663,51 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade(apr_bucke
 return OK;
 }

-/* Fill in unknown schemes from apr_uri_port_of_scheme() */

-typedef struct proxy_schemes_t {
-const char *name;
-apr_port_t default_port;
-} proxy_schemes_t ;
-
-static proxy_schemes_t pschemes[] =
-{
-{"fcgi", 8000},
-{"ajp",  AJP13_DEF_PORT},
-{"scgi", SCGI_DEF_PORT},
-{ NULL, 0x } /* unknown port */
-};
-
 PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme)
 {
 if (scheme) {
-apr_port_t port;
-if ((port = apr_uri_port_of_scheme(scheme)) != 0) {
-return port;
-} else {
-proxy_schemes_t *pscheme;
-for (pscheme = pschemes; pscheme->name != NULL; ++pscheme) {
-if (strcasecmp(scheme, pscheme->name) == 0) {
-return pscheme->default_port;
+/* Fill in unknown schemes from apr_uri_port_of_scheme(),
+ * and try a fast path for common schemes.
+ */
+switch (ap_tolower(*scheme)) {
+case 'a':
+if (strcasecmp(scheme + 1, "jp") == 0) {
+return AJP13_DEF_PORT;
+}
+break;
+case 'f':
+if (strcasecmp(scheme + 1, "cgi") == 0) {
+return 8000;
+}
+if (strcasecmp(scheme + 1, "tp") == 0) {
+return APR_URI_FTP_DEFAULT_PORT;
+}
+break;
+case 'h':
+if (strncasecmp(scheme + 1, "ttp", 4) == 0) {
+int s4 = ap_tolower(scheme[4]);
+if (!s4) {
+return APR_URI_HTTP_DEFAULT_PORT;
 }
+else if (s4 == 's' && !scheme[5]) {
+return APR_URI_HTTPS_DEFAULT_PORT;
+}
 }
+break;
+case 'n':
+if (strcasecmp(scheme + 1, "ntp") == 0) {
+return APR_URI_NNTP_DEFAULT_PORT;
+}
+break;
+case 's':
+if (strcasecmp(scheme + 1, "cgi") == 0) {
+return SCGI_DEF_PORT;
+}
+break;
 }
+
+return apr_uri_port_of_scheme(scheme);
 }
 return 0;
 }
--

Regards,
Yann.

Re: svn commit: r1650655 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS modules/proxy/proxy_util.c

[jean-frederic clere]

On 11/16/2015 06:04 PM, Jim Jagielski wrote:
> Are you logging DEBUG?

No, I also though that was the problem and checked it ;-)

Cheers

Jean-Frederic

> 
>> On Nov 16, 2015, at 11:53 AM, jean-frederic clere  wrote:
>>
>> On 01/09/2015 09:37 PM, jaillet...@apache.org wrote:
>>> Author: jailletc36
>>> Date: Fri Jan  9 20:37:50 2015
>>> New Revision: 1650655
>>>
>>> URL: http://svn.apache.org/r1650655
>>> Log:
>>> Merge r1644503 from trunk
>>>
>>>   * mod_proxy_ajp: Fix handling of the default port (8009) in the
>>> ProxyPass and  configurations.  PR 57259.
>>>
>>> Submitted by: ylavic
>>> Reviewed by: ylavic, jim, covener
>>> Backported by: jailletc36
>>>
>>> Modified:
>>>httpd/httpd/branches/2.4.x/CHANGES
>>>httpd/httpd/branches/2.4.x/STATUS
>>>httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>>>
>>> Modified: httpd/httpd/branches/2.4.x/CHANGES
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1650655=1650654=1650655=diff
>>> ==
>>> --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
>>> +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Fri Jan  9 20:37:50 2015
>>> @@ -22,6 +22,9 @@ Changes with Apache 2.4.11
>>>  request headers earlier.  Adds "MergeTrailers" directive to restore
>>>  legacy behavior.  [Edward Lu, Yann Ylavic, Joe Orton, Eric Covener]
>>>
>>> +  *) mod_proxy_ajp: Fix handling of the default port (8009) in the
>>> + ProxyPass and  configurations.  PR 57259.  [Yann Ylavic].
>>> +
>>>   *) mpm_event: Avoid a possible use after free when notifying the end of
>>>  connection during lingering close.  PR 57268.  [Eric Covener, Yann 
>>> Ylavic]
>>>
>>>
>>> Modified: httpd/httpd/branches/2.4.x/STATUS
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1650655=1650654=1650655=diff
>>> ==
>>> --- httpd/httpd/branches/2.4.x/STATUS (original)
>>> +++ httpd/httpd/branches/2.4.x/STATUS Fri Jan  9 20:37:50 2015
>>> @@ -104,12 +104,6 @@ RELEASE SHOWSTOPPERS:
>>> PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>>>   [ start all new proposals below, under PATCHES PROPOSED. ]
>>>
>>> -   * mod_proxy_ajp: Fix handling of the default port (8009) in the
>>> - ProxyPass and  configurations.  PR 57259.
>>> - trunk patch: http://svn.apache.org/r1644503
>>> - 2.4.x patch: trunk works (module CHANGES)
>>> - +1: ylavic, jim, covener
>>> -
>>>* mod_ssl: Check if we are having an SSL connection before looking up SSL
>>>   related variables during expression evaluation to avoid a 
>>> crash.
>>>   If not return NULL as ssl_var_lookup_ssl does by default.  PR 
>>> 57070
>>>
>>> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1650655=1650654=1650655=diff
>>> ==
>>> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
>>> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Jan  9 
>>> 20:37:50 2015
>>> @@ -1683,6 +1683,9 @@ PROXY_DECLARE(char *) ap_proxy_define_wo
>>>
>>> memset(wshared, 0, sizeof(proxy_worker_shared));
>>>
>>> +if (uri.port && uri.port == ap_proxy_port_of_scheme(uri.scheme)) {
>>> +uri.port = 0;
>>> +}
>>
>>
>> I must be doing something wrong but the above seems to hurt by 30% more
>> CPU one for the tests we are doing, any hints?
>>
>> Cheers
>>
>> Jean-Frederic
> 
>