Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
I wanted to avoid making string copies when possible. Plus, we
don't want to lowercase the URL, since that means /Foo/bar
would be the same as /FOO/Bar, which is wrong :)
Ruediger Pluem wrote:
On 10/31/2005 05:31 PM, [EMAIL PROTECTED] wrote:
Author: jim
Date: Mon Oct 31 08:31:29 2005
New Revision: 329849
URL: http://svn.apache.org/viewcvs?rev=329849view=rev
Log:
Fix a problem where we are doing a case insensitive
match between the worker and the URL. Instead, only
the scheme and hostname are insensitive, the rest
should be case sensitive.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/proxy_util.c
Thanks for looking into this. I think this is also related to PR36906.
Given the fact that the hostname and the schema already get lowercased by
ap_proxy_add_worker, wouldn't it be faster and clearer to do something like
the following (honest question, I do not know the answer and the best code
should
go in):
PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
proxy_server_conf *conf,
const char *url)
{
proxy_worker *worker;
proxy_worker *max_worker = NULL;
int max_match = 0;
int url_length;
int worker_name_length;
const char *c;
int i;
char *url_copy;
c = ap_strchr_c(url, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
return NULL;
url_copy = apr_pstrdup(p, url);
url_length = strlen(url_copy);
/*
* We need to find the start of the path and
* and lowercase all characters before.
*/
c = ap_strchr_c(c+3, '/');
if (c) {
*c = '\0';
ap_str_tolower(url_copy);
*c = '/';
}
else {
ap_str_tolower(url_copy);
}
worker = (proxy_worker *)conf-workers-elts;
/*
* Do a longest match on the worker name to find the worker that
* fits best to the URL.
*/
for (i = 0; i conf-workers-nelts; i++) {
if ( ((worker_name_length = strlen(worker-name)) = url_length)
(worker_name_length max_match)
(strncmp(url_copy, worker-name, worker_name_length) == 0)) {
max_worker = worker;
max_match = worker_name_length;
}
worker++;
}
return max_worker;
}
Regards
Rüdiger
--
===
Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/
If you can dodge a wrench, you can dodge a ball.
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
Since this happens for each request, doing a string copy seems wasteful to me; it's extra overhead that is avoided with the current impl. Instead, we have an extra assignment and check, which is less expensive. I originally toyed with doing the string copy, but instead opted for a more pointer oriented solution. Ruediger Pluem wrote: On 11/01/2005 02:05 PM, Jim Jagielski wrote: I wanted to avoid making string copies when possible. Plus, we Ok. That was one point of my question as I saw the need to copy the string as a drawback compared to your approach. I was only unsure how much this counts. don't want to lowercase the URL, since that means /Foo/bar would be the same as /FOO/Bar, which is wrong :) Sure, but I actually do not do this. I only lowercase scheme and hostname, because I temporarily terminate my copied string with \0 before I lower case it. So from the functional point of view my approach should do the same as yours. The question was more about which solution - is faster - is more easy to understand if both versions are equally fast Regards Rüdiger [..cut..] -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
On 11/01/2005 02:27 PM, Jim Jagielski wrote: Since this happens for each request, doing a string copy seems wasteful to me; it's extra overhead that is avoided with the current impl. Instead, we have an extra assignment and check, which is less expensive. This is fine. My other approach was born of the idea that we can replace strncasecmp with strncmp in the loop. Just for interest: Any idea regarding the difference in performance between both? Regards Rüdiger [..cut..]
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
Ruediger Pluem wrote: On 11/01/2005 02:27 PM, Jim Jagielski wrote: Since this happens for each request, doing a string copy seems wasteful to me; it's extra overhead that is avoided with the current impl. Instead, we have an extra assignment and check, which is less expensive. This is fine. My other approach was born of the idea that we can replace strncasecmp with strncmp in the loop. Just for interest: Any idea regarding the difference in performance between both? Certainly strncmp is quicker, since strncasecmp does an auto tolower on each char. But we are doing that in both cases, whether we're tolower'ing the string first, or whether we're doing it at comparison time. So we're not saving anything really there. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
On 11/01/2005 02:58 PM, Jim Jagielski wrote: [..cut..] Certainly strncmp is quicker, since strncasecmp does an auto tolower on each char. But we are doing that in both cases, whether we're tolower'ing the string first, or whether we're doing it at comparison time. So we're not saving anything really there. Yes, but lowering explicitly is done *outside* the for loop. strncasecmp is done *inside* the for loop. So we do this via strncasecmp multiple times on the same data. Regards Rüdiger
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
Ruediger Pluem wrote: Certainly strncmp is quicker, since strncasecmp does an auto tolower on each char. But we are doing that in both cases, whether we're tolower'ing the string first, or whether we're doing it at comparison time. So we're not saving anything really there. Yes, but lowering explicitly is done *outside* the for loop. strncasecmp is done *inside* the for loop. So we do this via strncasecmp multiple times on the same data. Whatever. Change it then. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
c = ap_strchr_c(url, ':'); if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0') return NULL; BTW, unless url is somehow limited to http and ftp, the above is bad code. The proxy should be able to handle arbitrary schemes (eventually), which means requiring scheme://host is bogus. Roy
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
Roy T. Fielding wrote: c = ap_strchr_c(url, ':'); if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0') return NULL; BTW, unless url is somehow limited to http and ftp, the above is bad code. The proxy should be able to handle arbitrary schemes (eventually), which means requiring scheme://host is bogus. The current code does make that restriction, but yes, it may be too limiting... Of course, this means that the workers would also need to be not so restricted as well :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
Ruediger, would the below appease your sensibilities :)
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 329779)
+++ modules/proxy/proxy_util.c(working copy)
@@ -1217,13 +1217,33 @@
int url_length;
int worker_name_length;
const char *c;
+char *url_copy;
int i;
c = ap_strchr_c(url, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
return NULL;
+url_copy = apr_pstrdup(p, url);
url_length = strlen(url);
+
+/*
+ * We need to find the start of the path and
+ * therefore we know the length of the scheme://hostname/
+ * part to we can force-lowercase everything up to
+ * the start of the path.
+ */
+c = ap_strchr_c(c+3, '/');
+if (c) {
+char *pathstart;
+pathstart = url_copy + (c - url);
+pathstart = '\0';
+ap_str_tolower(url_copy);
+pathstart = '/';
+} else {
+ap_str_tolower(url_copy);
+}
+
worker = (proxy_worker *)conf-workers-elts;
/*
@@ -1233,7 +1253,7 @@
for (i = 0; i conf-workers-nelts; i++) {
if ( ((worker_name_length = strlen(worker-name)) =
url_length)
(worker_name_length max_match)
-(strncasecmp(url, worker-name, worker_name_length) ==
0) ) {
+(strncmp(url_copy, worker-name, worker_name_length)
== 0) ) {
max_worker = worker;
max_match = worker_name_length;
}
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
On 11/01/2005 04:11 PM, Jim Jagielski wrote: Ruediger, would the below appease your sensibilities :) Yes, it does. I am sorry. I guess I was a little too persistent in this discussion about this patch of comparable limited influence. I did not mean to step on your toes and nerves :-). Regards Rüdiger [..cut..]
Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c
On 10/31/2005 05:31 PM, [EMAIL PROTECTED] wrote:
Author: jim
Date: Mon Oct 31 08:31:29 2005
New Revision: 329849
URL: http://svn.apache.org/viewcvs?rev=329849view=rev
Log:
Fix a problem where we are doing a case insensitive
match between the worker and the URL. Instead, only
the scheme and hostname are insensitive, the rest
should be case sensitive.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/proxy_util.c
Thanks for looking into this. I think this is also related to PR36906.
Given the fact that the hostname and the schema already get lowercased by
ap_proxy_add_worker, wouldn't it be faster and clearer to do something like
the following (honest question, I do not know the answer and the best code
should
go in):
PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
proxy_server_conf *conf,
const char *url)
{
proxy_worker *worker;
proxy_worker *max_worker = NULL;
int max_match = 0;
int url_length;
int worker_name_length;
const char *c;
int i;
char *url_copy;
c = ap_strchr_c(url, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
return NULL;
url_copy = apr_pstrdup(p, url);
url_length = strlen(url_copy);
/*
* We need to find the start of the path and
* and lowercase all characters before.
*/
c = ap_strchr_c(c+3, '/');
if (c) {
*c = '\0';
ap_str_tolower(url_copy);
*c = '/';
}
else {
ap_str_tolower(url_copy);
}
worker = (proxy_worker *)conf-workers-elts;
/*
* Do a longest match on the worker name to find the worker that
* fits best to the URL.
*/
for (i = 0; i conf-workers-nelts; i++) {
if ( ((worker_name_length = strlen(worker-name)) = url_length)
(worker_name_length max_match)
(strncmp(url_copy, worker-name, worker_name_length) == 0)) {
max_worker = worker;
max_match = worker_name_length;
}
worker++;
}
return max_worker;
}
Regards
Rüdiger
