Re: NOTICE: Intent to T 2.4.18

[Stefan Eissing]

Great! I try finish up the http2 stuff until Thursday as I will be unavailable 
next week. But everything looks good so far from my end. 

> Am 24.11.2015 um 20:09 schrieb Jim Jagielski :
> 
> Just a head's up: I intend to T 2.4.18 around
> Nov 30th...

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Graham Leggett]

On 24 Nov 2015, at 6:36 PM, Yann Ylavic  wrote:

> Sure, but my point is that the worst case is likely depend on the
> application,

It will depend on the application, yes.

> eg:
> 
>case 'm':
>case 'M':
>if (!strncmp(token, "max-age", 7)
>|| !ap_casecmpstrn(token, "max-age", 7)) {
>...
>}
>else if (!strncmp(token, "max-stale", 9)
> || !ap_casecmpstrn(token, "max-stale", 9)) {
>...
>}
>else if (!strncmp(token, "min-fresh", 9)
> || !ap_casecmpstrn(token, "min-fresh", 9)) {
>...
>}
>else if (!strcmp(token, "max-revalidate")
> || !ap_casecmpstr(token, "must-revalidate")) {

Oops - max-revalidate != must-revalidate

>...
>}
>else if ...
> 
> is going to be costly when matched against "must-revalidate", or worse
> "my-token”.

In that case make it cheaper for those cases.

Have the length handy to check for a minimum-sane-length, then do a switch on 
the 4th character.

> We could use all str[n]cmp() first, but still it's a lot of
> comparisons, and now duplicated code too.

The duplicated code is not a worry, the worry is to ensure the most common 
cases take the fastest path.

Regards,
Graham
—

Re: Better ap_casecmpstr[n]?

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 1:04 PM, Yann Ylavic  wrote:

> I tested this change with both Jim's and my versions, that's slower.
>
> The better implementation I have so far is:
>
> int ap_casecmpstr_2(const char *s1, const char *s2)
> {
> size_t i;
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
>
> for (i = 0; ; ++i) {
> const int c1 = ucharmap[ps1[i]];
> const int c2 = ucharmap[ps2[i]];
>
> if (c1 != c2) {
> return c1 - c2;
> }
> if (!c1) {
> break;
> }
> }
> return (0);
> }
>
> which is ~15% faster than the current one (and not 50% unless I remove
> the translation :)
>
> I also tried:
>
> for (i = 0; ; ++i) {
> const int c1 = ps1[i];
> const int c2 = ps2[i];
>
> if (c1 != c2 && ucharmap[c1] != ucharmap[c2]) {
> return ucharmap[c1] - ucharmap[c2];
> }
> ...
>
> but no.
>
>
> On Tue, Nov 24, 2015 at 7:56 PM, William A Rowe Jr 
> wrote:
> > For the optimization cases Graham was proposing, how does this perform on
> > your test setup? Looking for both absmatches, case mismatches and proper
> vs
> > lowercase comparisons...
> >
> > int ap_casecmpstr_2(const char *s1, const char *s2)
> > {
> > size_t i;
> > const unsigned char *ps1 = (const unsigned char *) s1;
> > const unsigned char *ps2 = (const unsigned char *) s2;
> >
> > for (i = 0; ; ++i) {
> > const int c1 = ucharmap[ps1[i]];
> > const int c2 = ucharmap[ps2[i]];
> > /* Above lookups are optimized away if first test below succeeds
> */
> >
> > if ((ps1[i] != ps2[i]) && (c1 != c2)) {
> > return c1 - c2;
> > }
> > if (!c1) {
> > break;
> > }
> > }
> > return (0);
> > }
> >
> >
> > On Tue, Nov 24, 2015 at 12:43 PM, Yann Ylavic 
> wrote:
> >>
> >> On Tue, Nov 24, 2015 at 7:39 PM, Mikhail T. 
> >> wrote:
> >> > On 24.11.2015 13:04, Yann Ylavic wrote:
> >> >
> >> > int ap_casecmpstr_2(const char *s1, const char *s2)
> >> > {
> >> > size_t i;
> >> > const unsigned char *ps1 = (const unsigned char *) s1;
> >> > const unsigned char *ps2 = (const unsigned char *) s2;
> >> >
> >> > for (i = 0; ; ++i) {
> >> > const int c1 = ps1[i];
> >> > const int c2 = ps2[i];
> >> >
> >> > if (c1 != c2) {
> >> > return c1 - c2;
> >> > }
> >> > if (!c1) {
> >> > break;
> >> > }
> >> > }
> >> > return (0);
> >> > }
> >> >
> >> > Sorry, but would not the above declare "A" and "a" to be different?
> >>
> >> Yeah, forgot the translation, I went too fast :)
> >
> >
>

Sounds like this concludes the discussion of calling strcmp, followed by
this function
for optimization benefits.

Your code is much easier to read, IMHO and I'd prefer that syntax, even if
it optimizes almost identically to the existing code.

The final question was what to call it and how to document it.  Documenting
can be iterative in svn, but do we agree that ap_strcmp_token clarifies that
this is for unambiguous token comparisons, or should we stick with something
that implies ascii (not literally true), or lc_posix (closer)?

Re: Weird interaction between mod_dir and [P] rewrites

[Jose Kahan]

Eric,

On Tue, Nov 24, 2015 at 01:37:09PM -0500, Eric Covener wrote:
> Tried DirectoryCheckHandler? It's a 2.2-vs-2.4 difference discovered a
> little too late to change by default.

Thank you very much. That fixed my issue. I wonder if it would be
interesting to add a note about this change in behavior and
the directive in [1].

Thank you again for your promt reply.

Cheers,

-jose

[1] http://httpd.apache.org/docs/2.4/upgrading.html

RE: H2 stream dependencies

[Bert Huijben]

> -Original Message-
> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Sent: vrijdag 20 november 2015 10:26
> To: dev@httpd.apache.org
> Subject: Re: H2 stream dependencies
> 
> Bert,
> 
> interesting and nice to see the progress. You probably could use priorities 
> for
> ordering, especially when using the stream dependencies. I am not certain,
> however, if this will give you totally deterministic behavior. If stream B
> depends on A, usually A will be sent out before B. However, should stream A
> become suspended - e.g. unable to progress, either because there is no data
> available which some other thread needs to produce, or because the flow
> control window has not been updated in time - the server will start sending
> data for B.
> 
> The state of the implementation is:
> 2.4.17: fully implemented priority handling on sending stream data, as
> implemented by the nghttp2 library

I'm seeing different behavior when running against my FreeBSD httpd 2.4.17 
(prefork mpm) and Windows (winnt mpm) builds.

Everything appears to work ok when sending a dependency within the headers 
frame to my FreeBSD box, while on my Windows build I get responses out of order.

Is it possible that different requests over the same connection are handled by 
different threads in this case?

Is there any logging that may help me diagnose this further?
(See log below)

The propfind is handled by mod_dav, while the report is handled by mod_dav_svn.

Bert

--
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(58): [client 
::1:15125] h2_stream(12-11): created
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_session.c(71): [client 
::1:15125] h2_session: stream(12-11): opened
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_workers.c(303): 
h2_workers: register mplx(12)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(167): [client 
::1:15125] h2_mplx(12-11): start stream, task PROPFIND 
/svn-test-work/local_tmp/repos/!svn/rev/1 (localhost:7829)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(180): [client 
::1:15125] h2_stream(12-11): closing input
[:09.091881 2015] [http2:debug] [pid 1956:tid 572] h2_workers.c(166): 
h2_worker(14): start task(12-11)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_session.c(420): [client 
::1:15125] h2_stream(12-11): input closed
[:09.091881 2015] [http2:debug] [pid 1956:tid 572] h2_task_input.c(84): [client 
::1:15125] h2_task_input(12-11): request is: 
[:09.091881 2015] [http2:debug] [pid 1956:tid 572] h2_h2.c(215): [client 
::1:15125] adding h1_to_h2_resp output filter
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(58): [client 
::1:15125] h2_stream(12-13): created
[:09.091881 2015] [ssl:debug] [pid 1956:tid 572] ssl_engine_kernel.c(238): 
[client ::1:15125] AH02034: Subsequent (No.2) HTTPS request received for child 
119652660 (server localhost:443)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_session.c(71): [client 
::1:15125] h2_session: stream(12-13): opened
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_workers.c(303): 
h2_workers: register mplx(12)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(167): [client 
::1:15125] h2_mplx(12-13): start stream, task REPORT 
/svn-test-work/local_tmp/repos/!svn/rev/1 (localhost:7829)
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_workers.c(166): 
h2_worker(4): start task(12-13)
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_stream.c(180): [client 
::1:15125] h2_stream(12-13): closing input
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(84): [client 
::1:15125] h2_task_input(12-13): request is: 
[:09.091881 2015] [http2:debug] [pid 1956:tid 908] h2_session.c(420): [client 
::1:15125] h2_stream(12-13): input closed
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_h2.c(215): [client 
::1:15125] adding h1_to_h2_resp output filter
[:09.091881 2015] [ssl:debug] [pid 1956:tid 340] ssl_engine_kernel.c(238): 
[client ::1:15125] AH02034: Subsequent (No.2) HTTPS request received for child 
56701100 (server localhost:443)
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-13): read, block=0, mode=1, readbytes=0
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-13): read, block=0, mode=0, readbytes=153
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-13): read, block=0, mode=1, readbytes=0
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-13): read, block=0, mode=1, readbytes=0
[:09.091881 2015] [http2:debug] [pid 1956:tid 340] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-13): read, block=0, mode=1, readbytes=0
[:09.092880 2015] [http2:debug] [pid 1956:tid 572] h2_task_input.c(107): 
[client ::1:15125] h2_task_input(12-11): read, block=0, 

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 10:18 AM, Graham Leggett  wrote:

> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
>
> > Not sure:
> >if (!strcmp(h, "max-age")
> >|| ap_cmpcasestr(h, "max-age"))
> > is likely to be a bit faster than a single ap_cmpcasestr() when it
> > matches, but much slower when it does not.
>
> Yep, that’s the point.
>
> The vast majority of comparisons are lowercase for tokens like this. Might
> as well test that fast path first before testing the worst case scenario.
>

Hmmm, it's our implementation, why increase the pain?  Consider the
following 'optimization' - fallthrough from identity to case insensitivity
with one wasted equality test...

AP_DECLARE(int) ap_casecmpstr(const char *s1, const char *s2)
{
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

while (*ps1 == *ps2) {
if (*ps1++ == '\0') {
return (0);
}
ps2++;
}
while (ucharmap[*ps1] == ucharmap[*ps2]) {
if (*ps1++ == '\0') {
return (0);
}
ps2++;
}
return (ucharmap[*ps1] - ucharmap[*ps2]);
}


AP_DECLARE(int) ap_casecmpstrn(const char *s1, const char *s2, apr_size_t n)
{
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;
while (n) {
if (*ps1 != *ps2) {
break;
}
if (*ps1++ == '\0') {
return (0);
}
ps2++;
n--;
}
while (n--) {
if (ucharmap[*ps1] != ucharmap[*ps2]) {
return (ucharmap[*ps1] - ucharmap[*ps2]);
}
if (*ps1++ == '\0') {
break;
}
ps2++;
}
return (0);
}

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Jim Jagielski]

It's for these types of reasons that I suggest that
instead of looking at this as some replacement for
strcasecmp, we instead look at the function on how we
actually *use* it. As far as I know, we simply use
it to see if 2 strings are equal, ignoring ASCII case.
I could be wrong, and I'm sure someone will point out
if and where I am :) But even so, if *that* is the
use case, then we are free to define the function
(and create whatever name) however we want.

> On Nov 24, 2015, at 11:11 AM, Eric Covener  wrote:
> 
> On Tue, Nov 24, 2015 at 11:07 AM, William A Rowe Jr  
> wrote:
>> Well, we are sorting the entire ASCII so I guess we can drop "for
>> alpha-numerics only".
> 
> Maybe it was fixed and I missed it, but didn't you point
> out that [] were not sorted right relative to alphas per
> POSIX strcasecmp?

Re: Better ap_casecmpstr[n]?

[Yann Ylavic]

register(s) are nicely handled by the compiler today, though locals
may help it ;)

Between 30% and 50% improvements (depending on the run, I'd have to do
an average to be more precise), is not that negligible IMHO.

On Tue, Nov 24, 2015 at 7:09 PM, Jim Jagielski  wrote:
> If we really want to squeek out optimizations, judicious use
> of 'register' might help even...
>
> But after awhile things start getting silly :)
>
>> On Nov 24, 2015, at 1:04 PM, Yann Ylavic  wrote:
>>
>> I did some testing with different implémentations and my results show
>> that fastest one is:
>>
>> int ap_casecmpstr_2(const char *s1, const char *s2)
>> {
>>size_t i;
>>const unsigned char *ps1 = (const unsigned char *) s1;
>>const unsigned char *ps2 = (const unsigned char *) s2;
>>
>>for (i = 0; ; ++i) {
>>const int c1 = ps1[i];
>>const int c2 = ps2[i];
>>
>>if (c1 != c2) {
>>return c1 - c2;
>>}
>>if (!c1) {
>>break;
>>}
>>}
>>return (0);
>> }
>>
>> int ap_casecmpstrn_2(const char *s1, const char *s2, size_t n)
>> {
>>size_t i;
>>const unsigned char *ps1 = (const unsigned char *) s1;
>>const unsigned char *ps2 = (const unsigned char *) s2;
>>
>>for (i = 0; i < n; ++i) {
>>const int c1 = ps1[i];
>>const int c2 = ps2[i];
>>
>>if (c1 != c2) {
>>return c1 - c2;
>>}
>>if (!c1) {
>>break;
>>}
>>}
>>return (0);
>> }
>>
>> Some samples (test program attached):
>>
>> $ gcc -Wall -O2 newtest.c -o newtest -lrt
>> $ for i in `seq 0 2`; do
>>./newtest $i 15000 \
>>xcxcxcxcxcxcxcxcxcxcwwaa \
>>xcxcxcxcxcxcxcxcxcxcwwaa \
>>0
>> done
>> - str[n]casecmp (nb=15000, len=0)
>> time = 8.444547186 : res = 0
>> - ap_casecmpstr[n] (nb=15000, len=0)
>> time = 8.299781468 : res = 0
>> - ap_casecmpstr[n] w/ index (nb=15000, len=0)
>> time = 6.148787259 : res = 0
>>
>> That's ~30% better.
>>
>> $ gcc -Wall -Os newtest.c -o newtest -lrt
>> $ for i in `seq 0 2`; do
>>./newtest $i 15000 \
>>xcxcxcxcxcxcxcxcxcxcwwaa \
>>xcxcxcxcxcxcxcxcxcxcwwaa \
>>0
>> done
>> - str[n]casecmp (nb=15000, len=0)
>> time = 8.528311136 : res = 0
>> - ap_casecmpstr[n] (nb=15000, len=0)
>> time = 10.150553381 : res = 0
>> - ap_casecmpstr[n] w/ index (nb=15000, len=0)
>> time = 9.758638566 : res = 0
>>
>> The string.h's str[n]casecmp beat us with -Os, still this new
>> implementation is better than the current one.
>>
>> WDYT, should I commit these new versions?
>> 
>

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 11:53 AM, Jim Jagielski  wrote:

> It's for these types of reasons that I suggest that
> instead of looking at this as some replacement for
> strcasecmp, we instead look at the function on how we
> actually *use* it. As far as I know, we simply use
> it to see if 2 strings are equal, ignoring ASCII case.
> I could be wrong, and I'm sure someone will point out
> if and where I am :) But even so, if *that* is the
> use case, then we are free to define the function
> (and create whatever name) however we want.
>


Well, there is another option, reject API bloat and insist that
users set LC_ALL="C" if they want httpd to behave correctly
and performantly.  I don't suppose you tested the performance
after turning off UTF-8 processing on your Mac?

Weird interaction between mod_dir and [P] rewrites

[Jose Kahan]

Hi, 

This is something that has changed in 2.4 compared to 2.2.

When I follow the simple scenario (attached file) in a fresh 
install of apache, mod_dir is affecting the first value of 
DirectoryIndex to my request before it is being proxied. 

In 2.2, I was used to browse "https:/example,com/foo/" and have 
this being proxied to a backend as "https://example2.com/bar/;. 
But now in 2.4.7, mod_dir is first converting "/foo/" to
"/foo/index.html" and then sending the proxy request as
"/bar/index.html".

Do you know if this is an intended feature or a bug? I would
expect the backend, and not the frontend, to decide what
it wants to add to a url ending by "/".

The code in mod_dir has changed and there's a section with a comment
saying that it steps aside when it detects it is inside a mod_rewrite
fixup. It seems to be missing a provision for proxy requests.

rbowen helped me test this against 2.4.17/fedora. I tested it against
2.4.10/debian jessie with the same result.

Thanks! Cheers.

-jose
Scenario for mod_dir and mod_rewrite bug

Tested against 2.4.17 in a fresh fedora install (Thanks rbowen!)

I have this setup

   /foo/.htaccess
   /bar/echo.php

Server runs in localhost.

1. In your apache config, make sure you have the DirectoryIndex directive
   e.g. 
DirectoryIndex index.html

2. Put the following php script under /bar/echo.php Its goal is to echo 
the request URI. You can do it with another script if you want:

[[
$ cat echo.php

]]

3. Put this .htaccess under /foo/.htaccess 
[[
$ cat .htaccess
RewriteEngine on
RewriteBase /foo
RewriteRule ^(.*)$ http://localhost/bar/echo.php/$1 [P,L]
]]

4. Try the test

   GET -Se http://localhost/foo/

If you get back: 

   Requested: ...echo.php/index.html
   # index.html is the first value of my DirectoryIndex directive

You see the bug, mod_dir added the first value of DirectoryIndex 
before doing the proxy request.

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 12:03 PM, Jim Jagielski  wrote:

>
> > On Nov 24, 2015, at 11:18 AM, Graham Leggett  wrote:
> >
> > On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
> >
> >> Not sure:
> >>   if (!strcmp(h, "max-age")
> >>   || ap_cmpcasestr(h, "max-age"))
> >> is likely to be a bit faster than a single ap_cmpcasestr() when it
> >> matches, but much slower when it does not.
> >
> > Yep, that’s the point.
> >
> > The vast majority of comparisons are lowercase for tokens like this.
> Might as well test that fast path first before testing the worst case
> scenario.
> >
>
> Is there REALLY that much of a diff between
>
> if (ucharmap[*ps1] != ucharmap[*ps2]) {
>
> and
>
> if (*ps1 != *ps2) {
>
> to muddle up the code like that though??
>

I'm wondering the other way around.  Even in Yann's latest exercise,
simply testing

   if ((*ps1 == *ps2) || (ucharmap[*ps1] != ucharmap[*ps2])) {

(or in Yann's code, use the const int lookups, considering that they
should be optimized out by the compiler if the first pattern matches).

Really we are expecting one of two things in strcmp_token(), we will
usually have an all-samecase (e.g. "GET" or "upgrade") and the
exceptions will largely be proper-case (e.g. "Upgrade").  So doing
the first char case-insensitively always seems smart and then
falling back on casematch until we don't case match.

By the time we've coded all that up, I wonder what the performance
is when simply checking equality and then the lookup match on the
character-by-character basis, for mixed vs Mixed vs MIXED.

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Yann Ylavic]

On Tue, Nov 24, 2015 at 7:24 PM, William A Rowe Jr  wrote:
>
> Well, there is another option, reject API bloat and insist that
> users set LC_ALL="C" if they want httpd to behave correctly
> and performantly.

Which API bloat? If we really want to avoid misleading anybody let's
call the function ap_tokencmp[n](), no?
(but I think casecmpstr[n]() is already quite clear about that...).

Setting the locale to "C" is not always desirable, some modules need
to work in unicode (UTF-8 or whatever), let's simply provide them
helpers, and do the right thing on the HTTP side.

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Jim Jagielski]

At this stage I'm at the point of diminishing returns...

I don't care what color we paint this bike shed.

I'm just glad I was able to come up with something for
people to nit-pick at and get all alpha-male about :P

> On Nov 24, 2015, at 1:24 PM, William A Rowe Jr  wrote:
> 
> On Tue, Nov 24, 2015 at 11:53 AM, Jim Jagielski  wrote:
> It's for these types of reasons that I suggest that
> instead of looking at this as some replacement for
> strcasecmp, we instead look at the function on how we
> actually *use* it. As far as I know, we simply use
> it to see if 2 strings are equal, ignoring ASCII case.
> I could be wrong, and I'm sure someone will point out
> if and where I am :) But even so, if *that* is the
> use case, then we are free to define the function
> (and create whatever name) however we want.
> 
> 
> Well, there is another option, reject API bloat and insist that
> users set LC_ALL="C" if they want httpd to behave correctly
> and performantly.  I don't suppose you tested the performance
> after turning off UTF-8 processing on your Mac?
> 
> 

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Jim Jagielski]

> On Nov 24, 2015, at 1:11 PM, Yann Ylavic  wrote:
> 
> On Tue, Nov 24, 2015 at 7:03 PM, Jim Jagielski  wrote:
>> 
>>> On Nov 24, 2015, at 11:18 AM, Graham Leggett  wrote:
>>> 
>>> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
>>> 
 Not sure:
  if (!strcmp(h, "max-age")
  || ap_cmpcasestr(h, "max-age"))
 is likely to be a bit faster than a single ap_cmpcasestr() when it
 matches, but much slower when it does not.
>>> 
>>> Yep, that’s the point.
>>> 
>>> The vast majority of comparisons are lowercase for tokens like this. Might 
>>> as well test that fast path first before testing the worst case scenario.
>>> 
>> 
>> Is there REALLY that much of a diff between
>> 
>>if (ucharmap[*ps1] != ucharmap[*ps2]) {
>> 
>> and
>> 
>>if (*ps1 != *ps2) {
>> 
>> to muddle up the code like that though??
> 
> The test from the other thread including str[n]cmp() says:
> 
> $ for i in `seq 0 3`; do
>./newtest $i 15000 \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>0
> done
> - str[n]casecmp (nb=15000, len=0)
> time = 8.435895804 : res = 0
> - ap_casecmpstr[n] (nb=15000, len=0)
> time = 8.207019751 : res = 0
> - ap_casecmpstr[n] w/ index (nb=15000, len=0)
> time = 4.429462481 : res = 0
> - str[n]cmp (nb=15000, len=0)
> time = 1.923039981 : res = 0
> 
> So strcmp() is really fast, since it probably advances word per word...

Yeah, likely just memcmp()

Re: Weird interaction between mod_dir and [P] rewrites

[Eric Covener]

Tried DirectoryCheckHandler? It's a 2.2-vs-2.4 difference discovered a
little too late to change by default.

Re: Better ap_casecmpstr[n]?

[Mikhail T.]

On 24.11.2015 13:04, Yann Ylavic wrote:
> int ap_casecmpstr_2(const char *s1, const char *s2)
> {
> size_t i;
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
>
> for (i = 0; ; ++i) {
> const int c1 = ps1[i];
> const int c2 = ps2[i];
>
> if (c1 != c2) {
> return c1 - c2;
> }
> if (!c1) {
> break;
> }
> }
> return (0);
> }
Sorry, but would not the above declare "A" and "a" to be different?

-mi

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 7:29 PM, William A Rowe Jr  wrote:
>
> I'm wondering the other way around.  Even in Yann's latest exercise,
> simply testing
>
>if ((*ps1 == *ps2) || (ucharmap[*ps1] != ucharmap[*ps2])) {
>
> (or in Yann's code, use the const int lookups, considering that they
> should be optimized out by the compiler if the first pattern matches).

*blink*
Actually the "indexed" function in my test does not include the
translation, just realized that while wanted to include your proposed
change.

-   const int c1 = ps1[i];
-   const int c2 = ps2[i];
+   const int c1 = ucharmap[ps1[i]];
+   const int c2 = ucharmap[ps2[i]];

So now it's just a tiny faster than Jim's, not worth the commit :) but
for -Os maybe...

BTW, I'll test your change with the original version.

Re: Better ap_casecmpstr[n]?

[Yann Ylavic]

On Tue, Nov 24, 2015 at 7:39 PM, Mikhail T.  wrote:
> On 24.11.2015 13:04, Yann Ylavic wrote:
>
> int ap_casecmpstr_2(const char *s1, const char *s2)
> {
> size_t i;
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
>
> for (i = 0; ; ++i) {
> const int c1 = ps1[i];
> const int c2 = ps2[i];
>
> if (c1 != c2) {
> return c1 - c2;
> }
> if (!c1) {
> break;
> }
> }
> return (0);
> }
>
> Sorry, but would not the above declare "A" and "a" to be different?

Yeah, forgot the translation, I went too fast :)

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 12:34 PM, Jim Jagielski  wrote:

> At this stage I'm at the point of diminishing returns...
>
> I don't care what color we paint this bike shed.
>
> I'm just glad I was able to come up with something for
> people to nit-pick at and get all alpha-male about :P
>

Sorry Jim, even board members have to refrain from ad-hominem
attacks on technical discussions, which is what everyone else
has been enjoying.  Good to see so many eyes on this optimization
even if it turned out to be a protocol correctness fix, instead!

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 5:18 PM, Graham Leggett  wrote:
> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
>
>> Not sure:
>>if (!strcmp(h, "max-age")
>>|| ap_cmpcasestr(h, "max-age"))
>> is likely to be a bit faster than a single ap_cmpcasestr() when it
>> matches, but much slower when it does not.
>
> Yep, that’s the point.
>
> The vast majority of comparisons are lowercase for tokens like this. Might as 
> well test that fast path first before testing the worst case scenario.

Sure, but my point is that the worst case is likely depend on the
application, eg:

case 'm':
case 'M':
if (!strncmp(token, "max-age", 7)
|| !ap_casecmpstrn(token, "max-age", 7)) {
...
}
else if (!strncmp(token, "max-stale", 9)
 || !ap_casecmpstrn(token, "max-stale", 9)) {
...
}
else if (!strncmp(token, "min-fresh", 9)
 || !ap_casecmpstrn(token, "min-fresh", 9)) {
...
}
else if (!strcmp(token, "max-revalidate")
 || !ap_casecmpstr(token, "must-revalidate")) {
...
}
else if ...

is going to be costly when matched against "must-revalidate", or worse
"my-token".

We could use all str[n]cmp() first, but still it's a lot of
comparisons, and now duplicated code too.

Regards,
Yann.

Re: strncasecmp

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 10:07 AM, Mikhail T. 
wrote:

> On 24.11.2015 10:08, William A Rowe Jr wrote:
>
> As long as this function is promoted for fast ASCII-specific token
> recognition and has no other unexpected equalities, it serves a useful
> purpose.
>
> Because of this, I'd suggest renaming it to something, that emphasizes it
> being ASCII-only.
>

Strictly speaking, you are referring to US-ASCII, while the actual function
including
the EBCDIC build follows LC_CTYPE/LC_COLLATE C or POSIX (equivalent names
for the same thing) and is honoring LC_COLLATE of the US-ASCII ordering.

apr_strcasecmp_lcposix or apr_strcasecmp_lcc might be a usable name.  Other
suggestions?  I was leaning toward apr_strcasecmp_token since that is a
pretty
well recognized RFC term and usually refers to the ASCII set.  For HTTPbis,
token
is defined at http://tools.ietf.org/html/rfc7230#section-3.2.6

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Eric Covener]

On Tue, Nov 24, 2015 at 11:07 AM, William A Rowe Jr  wrote:
> Well, we are sorting the entire ASCII so I guess we can drop "for
> alpha-numerics only".

Maybe it was fixed and I missed it, but didn't you point
out that [] were not sorted right relative to alphas per
POSIX strcasecmp?

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 5:09 PM, Graham Leggett  wrote:
>
> A further optimisation - in many cases the fast path is really a case 
> sensitive string comparison, given that the vast majority of the time the 
> case being used is the case quoted in the spec.
>
> In other words, while mAx-AgE is valid, it is almost always max-age.
>
> Does it make sense to do a case sensitive comparison first, and then fall 
> back to case insensitive?

Not sure:
if (!strcmp(h, "max-age")
|| ap_cmpcasestr(h, "max-age"))
is likely to be a bit faster than a single ap_cmpcasestr() when it
matches, but much slower when it does not.

Regards,
Yann.

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Graham Leggett]

On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:

> Not sure:
>if (!strcmp(h, "max-age")
>|| ap_cmpcasestr(h, "max-age"))
> is likely to be a bit faster than a single ap_cmpcasestr() when it
> matches, but much slower when it does not.

Yep, that’s the point.

The vast majority of comparisons are lowercase for tokens like this. Might as 
well test that fast path first before testing the worst case scenario.

Regards,
Graham
—

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 10:11 AM, Eric Covener  wrote:

> On Tue, Nov 24, 2015 at 11:07 AM, William A Rowe Jr 
> wrote:
> > Well, we are sorting the entire ASCII so I guess we can drop "for
> > alpha-numerics only".
>
> Maybe it was fixed and I missed it, but didn't you point
> out that [] were not sorted right relative to alphas per
> POSIX strcasecmp?
>

I was pointing out that most implementations sort [] in between upper and
lower
case letters, and in the string-folded ordering, all characters are treated
as their
lower case equivalent for collation.

Unsure how POSIX defined this.  The EBCDIC ordering table used the same
lower-case folding as ASCII, so the sortation of all POSIX characters will
be
identical between our EBCDIC (extended) and ASCII implementations.

[I only just realized that original EBCDIC didn't include all the C
characters,
I only used the code page for 15 years for document imaging but always
coded in ANSI :-]

Re: strncasecmp

[Mikhail T.]

On 24.11.2015 10:08, William A Rowe Jr wrote:
> As long as this function is promoted for fast ASCII-specific token
> recognition and has no other unexpected equalities, it serves a useful
> purpose.
Because of this, I'd suggest renaming it to something, that emphasizes
it being ASCII-only.

-mi

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Jim Jagielski]

> On Nov 24, 2015, at 11:18 AM, Graham Leggett  wrote:
> 
> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
> 
>> Not sure:
>>   if (!strcmp(h, "max-age")
>>   || ap_cmpcasestr(h, "max-age"))
>> is likely to be a bit faster than a single ap_cmpcasestr() when it
>> matches, but much slower when it does not.
> 
> Yep, that’s the point.
> 
> The vast majority of comparisons are lowercase for tokens like this. Might as 
> well test that fast path first before testing the worst case scenario.
> 

Is there REALLY that much of a diff between

if (ucharmap[*ps1] != ucharmap[*ps2]) {

and

if (*ps1 != *ps2) {

to muddle up the code like that though??

Better ap_casecmpstr[n]?

[Yann Ylavic]

I did some testing with different implémentations and my results show
that fastest one is:

int ap_casecmpstr_2(const char *s1, const char *s2)
{
size_t i;
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

for (i = 0; ; ++i) {
const int c1 = ps1[i];
const int c2 = ps2[i];

if (c1 != c2) {
return c1 - c2;
}
if (!c1) {
break;
}
}
return (0);
}

int ap_casecmpstrn_2(const char *s1, const char *s2, size_t n)
{
size_t i;
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

for (i = 0; i < n; ++i) {
const int c1 = ps1[i];
const int c2 = ps2[i];

if (c1 != c2) {
return c1 - c2;
}
if (!c1) {
break;
}
}
return (0);
}

Some samples (test program attached):

$ gcc -Wall -O2 newtest.c -o newtest -lrt
$ for i in `seq 0 2`; do
./newtest $i 15000 \
xcxcxcxcxcxcxcxcxcxcwwaa \
xcxcxcxcxcxcxcxcxcxcwwaa \
0
done
- str[n]casecmp (nb=15000, len=0)
time = 8.444547186 : res = 0
- ap_casecmpstr[n] (nb=15000, len=0)
time = 8.299781468 : res = 0
- ap_casecmpstr[n] w/ index (nb=15000, len=0)
time = 6.148787259 : res = 0

That's ~30% better.

$ gcc -Wall -Os newtest.c -o newtest -lrt
$ for i in `seq 0 2`; do
./newtest $i 15000 \
xcxcxcxcxcxcxcxcxcxcwwaa \
xcxcxcxcxcxcxcxcxcxcwwaa \
0
done
- str[n]casecmp (nb=15000, len=0)
time = 8.528311136 : res = 0
- ap_casecmpstr[n] (nb=15000, len=0)
time = 10.150553381 : res = 0
- ap_casecmpstr[n] w/ index (nb=15000, len=0)
time = 9.758638566 : res = 0

The string.h's str[n]casecmp beat us with -Os, still this new
implementation is better than the current one.

WDYT, should I commit these new versions?
#include 
#include 
#include 
#include 

#include 
#include 

/*
 * Provide our own known-fast implementation of str[n]casecmp()
 * NOTE: ASCII only!
 */
static const unsigned char ucharmap[] = {
0x0,  0x1,  0x2,  0x3,  0x4,  0x5,  0x6,  0x7,
0x8,  0x9,  0xa,  0xb,  0xc,  0xd,  0xe,  0xf,
0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f,
0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27,
0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f,
0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f,
0x40,  'a',  'b',  'c',  'd',  'e',  'f',  'g',
 'h',  'i',  'j',  'k',  'l',  'm',  'n',  'o',
 'p',  'q',  'r',  's',  't',  'u',  'v',  'w',
 'x',  'y',  'z', 0x5b, 0x5c, 0x5d, 0x5e, 0x5f,
0x60,  'a',  'b',  'c',  'd',  'e',  'f',  'g',
 'h',  'i',  'j',  'k',  'l',  'm',  'n',  'o',
 'p',  'q',  'r',  's',  't',  'u',  'v',  'w',
 'x',  'y',  'z', 0x7b, 0x7c, 0x7d, 0x7e, 0x7f,
0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87,
0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f,
0x90, 0x91, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97,
0x98, 0x99, 0x9a, 0x9b, 0x9c, 0x9d, 0x9e, 0x9f,
0xa0, 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7,
0xa8, 0xa9, 0xaa, 0xab, 0xac, 0xad, 0xae, 0xaf,
0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5, 0xb6, 0xb7,
0xb8, 0xb9, 0xba, 0xbb, 0xbc, 0xbd, 0xbe, 0xbf,
0xc0, 0xc1, 0xc2, 0xc3, 0xc4, 0xc5, 0xc6, 0xc7,
0xc8, 0xc9, 0xca, 0xcb, 0xcc, 0xcd, 0xce, 0xcf,
0xd0, 0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd6, 0xd7,
0xd8, 0xd9, 0xda, 0xdb, 0xdc, 0xdd, 0xde, 0xdf,
0xe0, 0xe1, 0xe2, 0xe3, 0xe4, 0xe5, 0xe6, 0xe7,
0xe8, 0xe9, 0xea, 0xeb, 0xec, 0xed, 0xee, 0xef,
0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7,
0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff
};

int ap_casecmpstr(const char *s1, const char *s2)
{
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

while (ucharmap[*ps1] == ucharmap[*ps2++]) {
if (*ps1++ == '\0') {
return (0);
}
}
return (ucharmap[*ps1] - ucharmap[*--ps2]);
}

int ap_casecmpstrn(const char *s1, const char *s2, size_t n)
{
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;
while (n--) {
if (ucharmap[*ps1] != ucharmap[*ps2++]) {
return (ucharmap[*ps1] - ucharmap[*--ps2]);
}
if (*ps1++ == '\0') {
break;
}
}
return (0);
}

int ap_casecmpstr_2(const char *s1, const char *s2)
{
size_t i;
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

for (i = 0; ; ++i) {
const int c1 = ps1[i];
const int c2 = ps2[i];

if (c1 != c2) {
return c1 - c2;
}
if (!c1) {
break;
}
}

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Jim Jagielski]

+1 (concept)

> On Nov 24, 2015, at 12:40 PM, William A Rowe Jr  wrote:
> 
> On Tue, Nov 24, 2015 at 10:18 AM, Graham Leggett  wrote:
> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
> 
> > Not sure:
> >if (!strcmp(h, "max-age")
> >|| ap_cmpcasestr(h, "max-age"))
> > is likely to be a bit faster than a single ap_cmpcasestr() when it
> > matches, but much slower when it does not.
> 
> Yep, that’s the point.
> 
> The vast majority of comparisons are lowercase for tokens like this. Might as 
> well test that fast path first before testing the worst case scenario.
> 
> Hmmm, it's our implementation, why increase the pain?  Consider the following 
> 'optimization' - fallthrough from identity to case insensitivity with one 
> wasted equality test...
> 
> AP_DECLARE(int) ap_casecmpstr(const char *s1, const char *s2)
> {
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
> 
> while (*ps1 == *ps2) {
> if (*ps1++ == '\0') {
> return (0);
> }
> ps2++;
> }
> while (ucharmap[*ps1] == ucharmap[*ps2]) {
> if (*ps1++ == '\0') {
> return (0);
> }
> ps2++;
> }
> return (ucharmap[*ps1] - ucharmap[*ps2]);
> }
> 
> 
> AP_DECLARE(int) ap_casecmpstrn(const char *s1, const char *s2, apr_size_t n)
> {
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
> while (n) {
> if (*ps1 != *ps2) {
> break;
> }
> if (*ps1++ == '\0') {
> return (0);
> }
> ps2++;
> n--;
> }
> while (n--) {
> if (ucharmap[*ps1] != ucharmap[*ps2]) {
> return (ucharmap[*ps1] - ucharmap[*ps2]);
> }
> if (*ps1++ == '\0') {
> break;
> }
> ps2++;
> }
> return (0);
> }
>  
>  

Re: Better ap_casecmpstr[n]?

[Jim Jagielski]

If we really want to squeek out optimizations, judicious use
of 'register' might help even...

But after awhile things start getting silly :)

> On Nov 24, 2015, at 1:04 PM, Yann Ylavic  wrote:
> 
> I did some testing with different implémentations and my results show
> that fastest one is:
> 
> int ap_casecmpstr_2(const char *s1, const char *s2)
> {
>size_t i;
>const unsigned char *ps1 = (const unsigned char *) s1;
>const unsigned char *ps2 = (const unsigned char *) s2;
> 
>for (i = 0; ; ++i) {
>const int c1 = ps1[i];
>const int c2 = ps2[i];
> 
>if (c1 != c2) {
>return c1 - c2;
>}
>if (!c1) {
>break;
>}
>}
>return (0);
> }
> 
> int ap_casecmpstrn_2(const char *s1, const char *s2, size_t n)
> {
>size_t i;
>const unsigned char *ps1 = (const unsigned char *) s1;
>const unsigned char *ps2 = (const unsigned char *) s2;
> 
>for (i = 0; i < n; ++i) {
>const int c1 = ps1[i];
>const int c2 = ps2[i];
> 
>if (c1 != c2) {
>return c1 - c2;
>}
>if (!c1) {
>break;
>}
>}
>return (0);
> }
> 
> Some samples (test program attached):
> 
> $ gcc -Wall -O2 newtest.c -o newtest -lrt
> $ for i in `seq 0 2`; do
>./newtest $i 15000 \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>0
> done
> - str[n]casecmp (nb=15000, len=0)
> time = 8.444547186 : res = 0
> - ap_casecmpstr[n] (nb=15000, len=0)
> time = 8.299781468 : res = 0
> - ap_casecmpstr[n] w/ index (nb=15000, len=0)
> time = 6.148787259 : res = 0
> 
> That's ~30% better.
> 
> $ gcc -Wall -Os newtest.c -o newtest -lrt
> $ for i in `seq 0 2`; do
>./newtest $i 15000 \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>xcxcxcxcxcxcxcxcxcxcwwaa \
>0
> done
> - str[n]casecmp (nb=15000, len=0)
> time = 8.528311136 : res = 0
> - ap_casecmpstr[n] (nb=15000, len=0)
> time = 10.150553381 : res = 0
> - ap_casecmpstr[n] w/ index (nb=15000, len=0)
> time = 9.758638566 : res = 0
> 
> The string.h's str[n]casecmp beat us with -Os, still this new
> implementation is better than the current one.
> 
> WDYT, should I commit these new versions?
> 

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Graham Leggett]

On 24 Nov 2015, at 5:21 PM, Jim Jagielski  wrote:

> For these types of paths, where we use a switch/case against
> the 1st char of the token, the real reason why we used that
> impl was to avoid the (expected) expensive string comparison.
> 
> This is really no longer an issue. Sure, we could still use
> this "fast path" short-circuit, but even that is non-optimal.
> For the below, what we really should be testing is:
> 
>   if (!ap_casecmpstr(token+1, "ublic")) {
> 
> for example.
> 
> So the question is: Do we remove these silly fast-paths
> that no longer make sense[1] or optimize for the "we know the
> 1st char" case ala the above?

A further optimisation - in many cases the fast path is really a case sensitive 
string comparison, given that the vast majority of the time the case being used 
is the case quoted in the spec.

In other words, while mAx-AgE is valid, it is almost always max-age.

Does it make sense to do a case sensitive comparison first, and then fall back 
to case insensitive?

Regards,
Graham
—

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 7:03 PM, Jim Jagielski  wrote:
>
>> On Nov 24, 2015, at 11:18 AM, Graham Leggett  wrote:
>>
>> On 24 Nov 2015, at 6:15 PM, Yann Ylavic  wrote:
>>
>>> Not sure:
>>>   if (!strcmp(h, "max-age")
>>>   || ap_cmpcasestr(h, "max-age"))
>>> is likely to be a bit faster than a single ap_cmpcasestr() when it
>>> matches, but much slower when it does not.
>>
>> Yep, that’s the point.
>>
>> The vast majority of comparisons are lowercase for tokens like this. Might 
>> as well test that fast path first before testing the worst case scenario.
>>
>
> Is there REALLY that much of a diff between
>
> if (ucharmap[*ps1] != ucharmap[*ps2]) {
>
> and
>
> if (*ps1 != *ps2) {
>
> to muddle up the code like that though??

The test from the other thread including str[n]cmp() says:

$ for i in `seq 0 3`; do
./newtest $i 15000 \
xcxcxcxcxcxcxcxcxcxcwwaa \
xcxcxcxcxcxcxcxcxcxcwwaa \
0
done
- str[n]casecmp (nb=15000, len=0)
time = 8.435895804 : res = 0
- ap_casecmpstr[n] (nb=15000, len=0)
time = 8.207019751 : res = 0
- ap_casecmpstr[n] w/ index (nb=15000, len=0)
time = 4.429462481 : res = 0
- str[n]cmp (nb=15000, len=0)
time = 1.923039981 : res = 0

So strcmp() is really fast, since it probably advances word per word...

Re: Better ap_casecmpstr[n]?

[William A Rowe Jr]

For the optimization cases Graham was proposing, how does this perform
on your test setup? Looking for both absmatches, case mismatches and
proper vs lowercase comparisons...

int ap_casecmpstr_2(const char *s1, const char *s2)
{
size_t i;
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

for (i = 0; ; ++i) {
const int c1 = ucharmap[ps1[i]];
const int c2 = ucharmap[ps2[i]];
/* Above lookups are optimized away if first test below succeeds */

if ((ps1[i] != ps2[i]) && (c1 != c2)) {
return c1 - c2;
}
if (!c1) {
break;
}
}
return (0);
}


On Tue, Nov 24, 2015 at 12:43 PM, Yann Ylavic  wrote:

> On Tue, Nov 24, 2015 at 7:39 PM, Mikhail T. 
> wrote:
> > On 24.11.2015 13:04, Yann Ylavic wrote:
> >
> > int ap_casecmpstr_2(const char *s1, const char *s2)
> > {
> > size_t i;
> > const unsigned char *ps1 = (const unsigned char *) s1;
> > const unsigned char *ps2 = (const unsigned char *) s2;
> >
> > for (i = 0; ; ++i) {
> > const int c1 = ps1[i];
> > const int c2 = ps2[i];
> >
> > if (c1 != c2) {
> > return c1 - c2;
> > }
> > if (!c1) {
> > break;
> > }
> > }
> > return (0);
> > }
> >
> > Sorry, but would not the above declare "A" and "a" to be different?
>
> Yeah, forgot the translation, I went too fast :)
>

Re: Better ap_casecmpstr[n]?

[Yann Ylavic]

I tested this change with both Jim's and my versions, that's slower.

The better implementation I have so far is:

int ap_casecmpstr_2(const char *s1, const char *s2)
{
size_t i;
const unsigned char *ps1 = (const unsigned char *) s1;
const unsigned char *ps2 = (const unsigned char *) s2;

for (i = 0; ; ++i) {
const int c1 = ucharmap[ps1[i]];
const int c2 = ucharmap[ps2[i]];

if (c1 != c2) {
return c1 - c2;
}
if (!c1) {
break;
}
}
return (0);
}

which is ~15% faster than the current one (and not 50% unless I remove
the translation :)

I also tried:

for (i = 0; ; ++i) {
const int c1 = ps1[i];
const int c2 = ps2[i];

if (c1 != c2 && ucharmap[c1] != ucharmap[c2]) {
return ucharmap[c1] - ucharmap[c2];
}
...

but no.


On Tue, Nov 24, 2015 at 7:56 PM, William A Rowe Jr  wrote:
> For the optimization cases Graham was proposing, how does this perform on
> your test setup? Looking for both absmatches, case mismatches and proper vs
> lowercase comparisons...
>
> int ap_casecmpstr_2(const char *s1, const char *s2)
> {
> size_t i;
> const unsigned char *ps1 = (const unsigned char *) s1;
> const unsigned char *ps2 = (const unsigned char *) s2;
>
> for (i = 0; ; ++i) {
> const int c1 = ucharmap[ps1[i]];
> const int c2 = ucharmap[ps2[i]];
> /* Above lookups are optimized away if first test below succeeds */
>
> if ((ps1[i] != ps2[i]) && (c1 != c2)) {
> return c1 - c2;
> }
> if (!c1) {
> break;
> }
> }
> return (0);
> }
>
>
> On Tue, Nov 24, 2015 at 12:43 PM, Yann Ylavic  wrote:
>>
>> On Tue, Nov 24, 2015 at 7:39 PM, Mikhail T. 
>> wrote:
>> > On 24.11.2015 13:04, Yann Ylavic wrote:
>> >
>> > int ap_casecmpstr_2(const char *s1, const char *s2)
>> > {
>> > size_t i;
>> > const unsigned char *ps1 = (const unsigned char *) s1;
>> > const unsigned char *ps2 = (const unsigned char *) s2;
>> >
>> > for (i = 0; ; ++i) {
>> > const int c1 = ps1[i];
>> > const int c2 = ps2[i];
>> >
>> > if (c1 != c2) {
>> > return c1 - c2;
>> > }
>> > if (!c1) {
>> > break;
>> > }
>> > }
>> > return (0);
>> > }
>> >
>> > Sorry, but would not the above declare "A" and "a" to be different?
>>
>> Yeah, forgot the translation, I went too fast :)
>
>

NOTICE: Intent to T 2.4.18

[Jim Jagielski]

Just a head's up: I intend to T 2.4.18 around
Nov 30th...

Re: strncasecmp

[Yann Ylavic]

On Tue, Nov 24, 2015 at 4:12 AM, Mikhail T.  wrote:
> On 23.11.2015 19:43, Yann Ylavic wrote:
>
>> That's expected (or at least no cared about in this test case). We simply
>> want res to not be optimized out, so print it before leaving, without any
>> particular relevance for its value (string.h and optimized versions should
>> return the same res with the same args, ascii strings only, though).
>
> Yes, we do want the value printed -- such as to catch the kind of errors I
> reported earlier. But my question was, why does the value change depending
> on the number of iterations?

Hmm, res is not reset (to 0) in each loop, so I first thought it might
well end up having a different value when ORed more times, but that
makes no sense...

Actually the "evil" is in (S1[0])++, (S2[0])++ in the for loop, which
kills some compiler optimizations in main() but also the comparison
once we leave alpha characters range for those :)
Let's say that for this test to be relevant, the first char of S1 and
S2 must be the same (including case) ;)

Regards,
Yann.

Re: strncasecmp

[Yann Ylavic]

On Tue, Nov 24, 2015 at 6:10 AM, Mikhail T.  wrote:
>
> Attached is the same program with one more pair of
> functions added (and an easy way to add more "candidates" to the
> main-driver). I changed the FOR-loop define to obtain repeatable results:

This test program kills str[n]casecmp()'s inlining with the
indirections (function pointers), so it's not really fair wrt
string.h's versions.
That's not really (often) what we'll find in real world apps.

>
> The new pair (method 2) does not use the static table, which is likely to
> benefit from CPU-cache unfairly in repetitive benchmarks.  It is slower than
> the table-using method 1 functions. But the two pairs might be comparable --
> or even faster -- in real life.

Possibly, this version may also be more "respective" of the cacheline
(the 256 bytes table may evict hot things), yet it is slower here...

Regards,
Yann.

Re: strncasecmp

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 6:40 AM, Jim Jagielski  wrote:

> It really depends on the OS and the version of the OS. In
> my test cases on OSX and CentOS5 and centOS6, I see
> measurable improvements.
>

Part of the reason for your differences... on this console here I have;
$ set | grep -E "^L[AC]"
LANG=en_US.UTF-8

Knock that back to a SBCS en_US.ISO-8859-1 and you'll see an
improvement, not that the stdc library implementation was inefficient
(it wasn't), but that UTF-8 causes strcmp to fall over into MBCS character
handling.

As long as this function is promoted for fast ASCII-specific token
recognition and has no other unexpected equalities, it serves a useful
purpose.

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 6:35 AM, Jim Jagielski  wrote:

> Yeah, but not, afaict, EBCDIC.


It would help for you to review the sources.  It wasn't a correct alpha sort
until I put in the EBCDIC code table this weekend, now EBCDIC chars
collate in ASCII order under this function.


> And in our use case, we
> don't care (and never use) the greater/less-than functionality,
>

s/our/my/;s/we/I/; - projecting much?  Designing utility functions is about
designing for utility, not one case.  If it is one case don't abstract it.
That's
why util[_string].c has a lot of apparently duplicate functionality that we
don't generally need under stdc '89 and APR, but we just hadn't gone back
and mopped up yet.


> just the equal to. This allows for possible other improvements/
> enhancements which might "break" the >< but doesn't affect
> how *we* use it.


Why would we break this?

Right now you can presume that if you are looking strictly for some
encoding tokens 'chunked' 'deflate' 'gzip', you can actually walk the
token list in alpha order and find out what was missing or unexpected
in a slightly more efficient way.

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Jim Jagielski]

For these types of paths, where we use a switch/case against
the 1st char of the token, the real reason why we used that
impl was to avoid the (expected) expensive string comparison.

This is really no longer an issue. Sure, we could still use
this "fast path" short-circuit, but even that is non-optimal.
For the below, what we really should be testing is:

if (!ap_casecmpstr(token+1, "ublic")) {

for example.

So the question is: Do we remove these silly fast-paths
that no longer make sense[1] or optimize for the "we know the
1st char" case ala the above?


1. eg:

while (token) {
if (!ap_casecmpstrn(token, "no-cache", 8)) {
if (token[8] == '=') {
cc->no_cache_header = 1;
}
else if (!token[8]) {
cc->no_cache = 1;
}
}
else if (!ap_casecmpstr(token, "no-store")) {
cc->no_store = 1;
}
else if (!ap_casecmpstr(token, "no-transform")) {
cc->no_transform = 1;
}
else if (!ap_casecmpstrn(token, "max-age", 7)) {
if (token[7] == '='
&& !apr_strtoff(, token + 8, , 10)
&& endp > token + 8 && !*endp) {
cc->max_age = 1;
cc->max_age_value = offt;
}
}
else if (!ap_casecmpstr(token, "must-revalidate")) {
cc->must_revalidate = 1;
}
else if (!ap_casecmpstrn(token, "max-stale", 9)) {
if (token[9] == '='
&& !apr_strtoff(, token + 10, , 10)
&& endp > token + 10 && !*endp) {
cc->max_stale = 1;
cc->max_stale_value = offt;
}
else if (!token[9]) {
cc->max_stale = 1;
cc->max_stale_value = -1;
}
}
else if (!ap_casecmpstrn(token, "min-fresh", 9)) {
if (token[9] == '='
&& !apr_strtoff(, token + 10, , 10)
&& endp > token + 10 && !*endp) {
cc->min_fresh = 1;
cc->min_fresh_value = offt;
}
}

...

> On Nov 23, 2015, at 3:05 PM, yla...@apache.org wrote:
> 
> Author: ylavic
> Date: Mon Nov 23 20:05:16 2015
> New Revision: 1715938
> 
> URL: http://svn.apache.org/viewvc?rev=1715938=rev
> Log:
> Follow up to r1715876: fix typo.
> 
> Modified:
>httpd/httpd/trunk/modules/cache/cache_util.c
> 
> Modified: httpd/httpd/trunk/modules/cache/cache_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=1715938=1715937=1715938=diff
> ==
> --- httpd/httpd/trunk/modules/cache/cache_util.c (original)
> +++ httpd/httpd/trunk/modules/cache/cache_util.c Mon Nov 23 20:05:16 2015
> @@ -1074,7 +1074,8 @@ int ap_cache_control(request_rec *r, cac
> }
> break;
> }
> -case 'p': {
> +case 'p':
> +case 'P': {
> if (!ap_casecmpstr(token, "public")) {
> cc->public = 1;
> }
> 
> 

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Eric Covener]

On Tue, Nov 24, 2015 at 10:21 AM, Jim Jagielski  wrote:
> For these types of paths, where we use a switch/case against
> the 1st char of the token, the real reason why we used that
> impl was to avoid the (expected) expensive string comparison.

Maybe naively, I thought we were saving some potential function
call overhead here rather than some strcasecmp overhead.

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Jim Jagielski]

if the current EBCDIC validates the >< lex, then by all means
update the docs to reflect that. You specifically said that
"The implementation does provide ASCII numeric-alpha ordering."

The main reason I was suggesting this was that, really,
it would make sense for the function to return true if
they compare and 0 if not, which is, of course, not
how strncasecmp does it (which is why we have the weird
looking !strcasecmp() for code paths where they DO compare :) ).
So by allowing that casecmpstr was not, in fact, a *replacement*
implementation of strcasecmp(), we could define it to be more
sensible ;)

Like you said, we aren't creating a replacement clib, so creating
funcs for our own specific usecase does make some sense.
> On Nov 24, 2015, at 10:15 AM, William A Rowe Jr  wrote:
> 
> On Tue, Nov 24, 2015 at 6:35 AM, Jim Jagielski  wrote:
> Yeah, but not, afaict, EBCDIC.
> 
> It would help for you to review the sources.  It wasn't a correct alpha sort
> until I put in the EBCDIC code table this weekend, now EBCDIC chars
> collate in ASCII order under this function.
>  
> And in our use case, we
> don't care (and never use) the greater/less-than functionality,
> 
> s/our/my/;s/we/I/; - projecting much?  Designing utility functions is about
> designing for utility, not one case.  If it is one case don't abstract it.  
> That's
> why util[_string].c has a lot of apparently duplicate functionality that we
> don't generally need under stdc '89 and APR, but we just hadn't gone back
> and mopped up yet.
>  
> just the equal to. This allows for possible other improvements/
> enhancements which might "break" the >< but doesn't affect
> how *we* use it.
> 
> Why would we break this?
> 
> Right now you can presume that if you are looking strictly for some
> encoding tokens 'chunked' 'deflate' 'gzip', you can actually walk the 
> token list in alpha order and find out what was missing or unexpected 
> in a slightly more efficient way. 
> 
> 

Re: svn commit: r1715869 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

[Jim Jagielski]

Most of those were 100% fine... I was referring to the
apr_atoi64() stuff :)

> On Nov 23, 2015, at 11:28 AM, yla...@apache.org wrote:
> 
> Author: ylavic
> Date: Mon Nov 23 16:28:36 2015
> New Revision: 1715869
> 
> URL: http://svn.apache.org/viewvc?rev=1715869=rev
> Log:
> Revert r1715789: will re-commit without spurious functional changes.
> 
> Modified:
>httpd/httpd/trunk/modules/cache/cache_util.c
>httpd/httpd/trunk/modules/filters/mod_deflate.c
>httpd/httpd/trunk/modules/filters/mod_include.c
>httpd/httpd/trunk/modules/filters/mod_proxy_html.c
>httpd/httpd/trunk/modules/generators/mod_autoindex.c
>httpd/httpd/trunk/modules/generators/mod_cgi.c
>httpd/httpd/trunk/modules/generators/mod_cgid.c
>httpd/httpd/trunk/modules/http/byterange_filter.c
>httpd/httpd/trunk/modules/http/http_filters.c
>httpd/httpd/trunk/modules/http2/h2_from_h1.c
>httpd/httpd/trunk/modules/loggers/mod_log_config.c
>httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>httpd/httpd/trunk/modules/metadata/mod_cern_meta.c
>httpd/httpd/trunk/modules/metadata/mod_headers.c
>httpd/httpd/trunk/modules/proxy/ajp_header.c
>httpd/httpd/trunk/modules/proxy/mod_proxy.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_scgi.c
>httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
>httpd/httpd/trunk/modules/proxy/mod_serf.c
>httpd/httpd/trunk/modules/proxy/proxy_util.c
>httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
>httpd/httpd/trunk/modules/test/mod_policy.c
>httpd/httpd/trunk/server/mpm_unix.c
>httpd/httpd/trunk/server/protocol.c
>httpd/httpd/trunk/server/util.c
>httpd/httpd/trunk/server/util_expr_eval.c
>httpd/httpd/trunk/server/util_script.c
> 
> Modified: httpd/httpd/trunk/modules/cache/cache_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=1715869=1715868=1715869=diff
> ==
> --- httpd/httpd/trunk/modules/cache/cache_util.c (original)
> +++ httpd/httpd/trunk/modules/cache/cache_util.c Mon Nov 23 16:28:36 2015
> @@ -594,12 +594,7 @@ int cache_check_freshness(cache_handle_t
> }
> 
> if ((agestr = apr_table_get(h->resp_hdrs, "Age"))) {
> -char *endp;
> -apr_off_t offt;
> -if (!apr_strtoff(, agestr, , 10)
> -&& endp > agestr && !*endp) {
> -age_c = offt;
> -}
> +age_c = apr_atoi64(agestr);
> }
> 
> /* calculate age of object */
> @@ -999,7 +994,12 @@ int ap_cache_control(request_rec *r, cac
> char *header = apr_pstrdup(r->pool, pragma_header);
> const char *token = cache_strqtok(header, CACHE_SEPARATOR, );
> while (token) {
> -if (!ap_casecmpstr(token, "no-cache")) {
> +/* handle most common quickest case... */
> +if (!strcmp(token, "no-cache")) {
> +cc->no_cache = 1;
> +}
> +/* ...then try slowest case */
> +else if (!strcasecmp(token, "no-cache")) {
> cc->no_cache = 1;
> }
> token = cache_strqtok(NULL, CACHE_SEPARATOR, );
> @@ -1008,15 +1008,21 @@ int ap_cache_control(request_rec *r, cac
> }
> 
> if (cc_header) {
> -char *endp;
> -apr_off_t offt;
> char *header = apr_pstrdup(r->pool, cc_header);
> const char *token = cache_strqtok(header, CACHE_SEPARATOR, );
> while (token) {
> switch (token[0]) {
> case 'n':
> case 'N': {
> -if (!ap_casecmpstrn(token, "no-cache", 8)) {
> +/* handle most common quickest cases... */
> +if (!strcmp(token, "no-cache")) {
> +cc->no_cache = 1;
> +}
> +else if (!strcmp(token, "no-store")) {
> +cc->no_store = 1;
> +}
> +/* ...then try slowest cases */
> +else if (!strncasecmp(token, "no-cache", 8)) {
> if (token[8] == '=') {
> cc->no_cache_header = 1;
> }
> @@ -1025,63 +1031,73 @@ int ap_cache_control(request_rec *r, cac
> }
> break;
> }
> -else if (!ap_casecmpstr(token, "no-store")) {
> +else if (!strcasecmp(token, "no-store")) {
> cc->no_store = 1;
> }
> -else if (!ap_casecmpstr(token, "no-transform")) {
> +else if (!strcasecmp(token, "no-transform")) {
> 

Re: svn commit: r1715789 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

[Jim Jagielski]

To be clear, I was only semi-joking about the code
changes (eg: apr_atoi64()) which did not involve
ap_casecmpstr() at all :) :)


> On Nov 23, 2015, at 11:50 AM, Yann Ylavic  wrote:
> 
> On Mon, Nov 23, 2015 at 3:05 PM, Jim Jagielski  wrote:
>> 
>> That's cheating :)
> 
> Yeah, reverted (r1715869) and re-committed (r1715876) with no functional 
> change.
> Thanks for catching!

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Jim Jagielski]

Yeah, but not, afaict, EBCDIC. And in our use case, we
don't care (and never use) the greater/less-than functionality,
just the equal to. This allows for possible other improvements/
enhancements which might "break" the >< but doesn't affect
how *we* use it.
> On Nov 23, 2015, at 12:01 PM, William A Rowe Jr  wrote:
> 
> On Mon, Nov 23, 2015 at 9:58 AM,  wrote:
> Author: jim
> Date: Mon Nov 23 15:58:25 2015
> New Revision: 1715859
> 
> URL: http://svn.apache.org/viewvc?rev=1715859=rev
> Log:
> we just worry about "equality" with this implementation...
> So it's not a "real" strcasecmp replacement.
> 
> Modified:
> httpd/httpd/trunk/include/httpd.h
> 
> Modified: httpd/httpd/trunk/include/httpd.h
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1715859=1715858=1715859=diff
> ==
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Mon Nov 23 15:58:25 2015
> @@ -2442,9 +2442,8 @@ AP_DECLARE(int) ap_array_str_contains(co
>   * Known-fast version of strcasecmp(): ASCII case-folding, POSIX compliant
>   * @param s1 The 1st string to compare
>   * @param s2 The 2nd string to compare
> - * @return integer greater than, equal to, or less than 0, depending on
> - * if s1 is lexicographically greater than, equal to, or less
> - * than s2 ignoring case.
> + * @return 0 if s1 is lexicographically equal to s2 ignoring case;
> + * non-0 otherwise.
>   */
>  AP_DECLARE(int) ap_casecmpstr(const char *s1, const char *s2);
> 
> @@ -2453,9 +2452,8 @@ AP_DECLARE(int) ap_casecmpstr(const char
>   * @param s1 The 1st string to compare
>   * @param s2 The 2nd string to compare
>   * @param n  Maximum number of characters in the strings to compare
> - * @return integer greater than, equal to, or less than 0, depending on
> - * if s1 is lexicographically greater than, equal to, or less
> - * than s2 ignoring case.
> + * @return 0 if s1 is lexicographically equal to s2 ignoring case;
> + * non-0 otherwise.
>   */
>  AP_DECLARE(int) ap_casecmpstrn(const char *s1, const char *s2, apr_size_t n);
> 
> Howso?  The implementation does provide ASCII numeric-alpha ordering.

Re: strncasecmp

[Jim Jagielski]

It really depends on the OS and the version of the OS. In
my test cases on OSX and CentOS5 and centOS6, I see
measurable improvements.
> On Nov 23, 2015, at 3:12 PM, Christophe JAILLET 
>  wrote:
> 
> Hi Jim,
> 
> Do you have done some benchmark and have results to share?
> 
> I tried to do some but the benefit of the optimized version is not that 
> clear, at least on my system:
>   gcc 5.2.1
>   Linux linux 4.2.0-18-generic #22-Ubuntu SMP Fri Nov 6 18:25:50 UTC 2015 
> x86_64 x86_64 x86_64 GNU/Linux
> 
> I've tried to trick gcc in order to avoid some optimization. (gcc does its 
> best to remove/inline/remove from loop known standard C functions)
> I've check asm output to be more confident on the code generated by gcc.
> 
> Based on my own results only, and without any becnhmark, I would -1 a 
> backport.
> For small strings (below identical char), your implementation looks faster, 
> but above 4 identical chars it looks slower
> 
> I can provide my test program if interested.
> 
> 
> best regards,
> CJ
> 
> 
> Le 20/11/2015 19:53, Jim Jagielski a écrit :
>> Implemented in r1715401
>> 
>> If people want to nit-pick about naming and wish to
>> rename it to something else, be my guest.
>> 
>>> On Nov 20, 2015, at 1:03 PM, Yann Ylavic  wrote:
>>> 
>>> +1
>>> 
>>> On Fri, Nov 20, 2015 at 6:17 PM, Jim Jagielski  wrote:
 We use str[n]casecmp quite a bit. The rub is that some
 platforms use a sensible implementation (such as OSX) which
 uses a upper-lowercase map and is v. fast, and others
 use a brain-dead version which does an actual tolower() of
 each char in the string as it tests. We actually try to
 handle this in many cases by doing a switch/case test on the
 1st char to fast path the strncasecmp, resulting in ugly code.
 
 This is crazy.
 
 I propose a ap_strncasecmp/ap_strcasecmp which we should use.
 Ideally, it would be in apr but no need to wait for that
 to happen :)
 
 Unless people have heartburn about this, I will add next week.
> 

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

[Jim Jagielski]

> On Nov 23, 2015, at 3:00 PM, Marion & Christophe JAILLET 
>  wrote:
> 
> Hi,
> 
> 1 typo below.
> 
> Moreover, this kind of patch is a good candidate for backport as it 
> introduces many small differences between 2.4 and trunk.
> Without a backport, backporting future patches may become a nightmare.
> 

++1. I always anticipated it being backported.

Re: svn commit: r1715789 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

[Yann Ylavic]

I think that's what I understood :)
I prefered reverting the whole and re-committing without the unrelated
changes (the three apr_atoi64() in cache_util.c), and then commit
those apr_atoi64() changes separately (r1715886) to propose a specific
backport...

On Tue, Nov 24, 2015 at 1:37 PM, Jim Jagielski  wrote:
> To be clear, I was only semi-joking about the code
> changes (eg: apr_atoi64()) which did not involve
> ap_casecmpstr() at all :) :)
>
>
>> On Nov 23, 2015, at 11:50 AM, Yann Ylavic  wrote:
>>
>> On Mon, Nov 23, 2015 at 3:05 PM, Jim Jagielski  wrote:
>>>
>>> That's cheating :)
>>
>> Yeah, reverted (r1715869) and re-committed (r1715876) with no functional 
>> change.
>> Thanks for catching!
>

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 4:28 PM, Eric Covener  wrote:
> On Tue, Nov 24, 2015 at 10:21 AM, Jim Jagielski  wrote:
>> For these types of paths, where we use a switch/case against
>> the 1st char of the token, the real reason why we used that
>> impl was to avoid the (expected) expensive string comparison.
>
> Maybe naively, I thought we were saving some potential function
> call overhead here rather than some strcasecmp overhead.

I agree, the switch'ed code is likely to be more efficient because of
that (unless we inline casecmpstr[n], but that's not a good idea
IMHO).

I'll modify Chritophe test a bit (more) to check that...

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Jim Jagielski]

> On Nov 24, 2015, at 10:28 AM, Eric Covener  wrote:
> 
> On Tue, Nov 24, 2015 at 10:21 AM, Jim Jagielski  wrote:
>> For these types of paths, where we use a switch/case against
>> the 1st char of the token, the real reason why we used that
>> impl was to avoid the (expected) expensive string comparison.
> 
> Maybe naively, I thought we were saving some potential function
> call overhead here rather than some strcasecmp overhead.

Yeah... you are likely right. In which case, we should adjust
ala the suggested

if (!ap_casecmpstr(token+1, "ublic")) {

implementation. Yeah, that's ugly, I agree.

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 9:28 AM, Jim Jagielski  wrote:

> if the current EBCDIC validates the >< lex, then by all means
> update the docs to reflect that. You specifically said that
> "The implementation does provide ASCII numeric-alpha ordering."
>

You are right, calling out "This implementation provides ASCII-ordering
of strings (EBCDIC text is also ASCII ordered on those platforms)"
might have been clearer?  Eric, what phrasing would make sense to you
as a user/maintainer of the httpd EBCDIC port?


> The main reason I was suggesting this was that, really,
> it would make sense for the function to return true if
> they compare and 0 if not, which is, of course, not
> how strncasecmp does it (which is why we have the weird
> looking !strcasecmp() for code paths where they DO compare :) ).
> So by allowing that casecmpstr was not, in fact, a *replacement*
> implementation of strcasecmp(), we could define it to be more
> sensible ;)
>

That's possible, although I'm -.5 on losing the functionality for the
earlier reasons stated, and your interpretation is incorrect.  "cmp"
implies compare, lt, 0, gt valuations. "eq" implies equality tests.

Like you said, we aren't creating a replacement clib, so creating
> funcs for our own specific usecase does make some sense.
>

Sure, and the use case(s) can be broad enough to serve more than
one purpose.

If you really need a strict equality test, isn't

#define ap_strcaseequals(s1, s2) (!ap_strcasecmp(s1, s2)) sufficient?

(or choose match or identity but not cmp, please :)

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[Eric Covener]

On Tue, Nov 24, 2015 at 10:41 AM, William A Rowe Jr  wrote:
> You are right, calling out "This implementation provides ASCII-ordering
> of strings (EBCDIC text is also ASCII ordered on those platforms)"
> might have been clearer?  Eric, what phrasing would make sense to you
> as a user/maintainer of the httpd EBCDIC port?

"Provides C/POSIX strcasecmp ordering, for alpha-numerics only"?

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 9:28 AM, Eric Covener  wrote:

> On Tue, Nov 24, 2015 at 10:21 AM, Jim Jagielski  wrote:
> > For these types of paths, where we use a switch/case against
> > the 1st char of the token, the real reason why we used that
> > impl was to avoid the (expected) expensive string comparison.
>
> Maybe naively, I thought we were saving some potential function
> call overhead here rather than some strcasecmp overhead.
>

In part, but also in this specific case;

while (token) {
***
A switch here short circuits three function calls;
if (!ap_casecmpstrn(token, "no-cache", 8)) {
if (token[8] == '=') {
cc->no_cache_header = 1;
}
else if (!token[8]) {
cc->no_cache = 1;
}
}
else if (!ap_casecmpstr(token, "no-store")) {
cc->no_store = 1;
}
else if (!ap_casecmpstr(token, "no-transform")) {
cc->no_transform = 1;
}
***
A switch here short circuits four function calls;
else if (!ap_casecmpstrn(token, "max-age", 7)) {
if (token[7] == '='
&& !apr_strtoff(, token + 8, , 10)
&& endp > token + 8 && !*endp) {
cc->max_age = 1;
cc->max_age_value = offt;
}
}
else if (!ap_casecmpstr(token, "must-revalidate")) {
cc->must_revalidate = 1;
}
else if (!ap_casecmpstrn(token, "max-stale", 9)) {
if (token[9] == '='
&& !apr_strtoff(, token + 10, , 10)
&& endp > token + 10 && !*endp) {
cc->max_stale = 1;
cc->max_stale_value = offt;
}
else if (!token[9]) {
cc->max_stale = 1;
cc->max_stale_value = -1;
}
}
else if (!ap_casecmpstrn(token, "min-fresh", 9)) {
if (token[9] == '='
&& !apr_strtoff(, token + 10, , 10)
&& endp > token + 10 && !*endp) {
cc->min_fresh = 1;
cc->min_fresh_value = offt;
}
}
***

Plus the overhead if each function call itself, it is still sensible to
narrow out
several groups of tokens (could even use if s[0]=='n' s[1]=='o' tests)

Does it make sense to expose the ap_tolower_ascii() function?  We could
call it ap_tolower_plaintext() or something else to distinguish that it
isn't
lowercasing extended characters?  Or is the case 'N': case 'n': syntax
easier to follow?

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

[Yann Ylavic]

On Tue, Nov 24, 2015 at 5:28 AM, William A Rowe Jr  wrote:
>
> I didn't have a chance to review the patch, but all of the instances that 1.
> map to an RFC protocol string comparison and 2. occur in 2.4 code base
> *should* be coalesced into one patch.  I'm +1 for every such change.  The
> rest of the RFC token comparisons can be added individually for later
> merging with their associated bug or enhancement backports.

The goal of the changes I committed (and reverted) so far is to
address the cases where we really expect ASCII comparison (ie. don't
want locale to make us think the compared string matches whereas it is
not, eg. "Connection: clôse").
This mainly concerns header names, header values (known tokens), schemes, ...
Hopefully I reverted the other/abusive changes already.

There remain these changes to mod_proxy and mod_rewrite (revert patch
attached, not applied, yet?), which I think makes sense for
performance reasons, but are not strictly required since the compared
strings come from the configuration file (though mod_proxy params may
be configured via the balancer manager).
I whish I could let them applied because of the (huge/costly)
if-else-if-else-if-... pattern, and we do compare real (ours) tokens
here...
Index: modules/mappers/mod_rewrite.c
===
--- modules/mappers/mod_rewrite.c	(revision 1716159)
+++ modules/mappers/mod_rewrite.c	(working copy)
@@ -3477,13 +3477,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
 switch (*key++) {
 case 'b':
 case 'B':
-if (!*key || !ap_casecmpstr(key, "ackrefescaping")) {
+if (!*key || !strcasecmp(key, "ackrefescaping")) {
 cfg->flags |= RULEFLAG_ESCAPEBACKREF;
 if (val && *val) { 
 cfg->escapes = val;
 }
 }
-else if (!ap_casecmpstr(key, "NP") || !ap_casecmpstr(key, "ackrefernoplus")) { 
+else if (!strcasecmp(key, "NP") || !strcasecmp(key, "ackrefernoplus")) { 
 cfg->flags |= RULEFLAG_ESCAPENOPLUS;
 }
 else {
@@ -3492,11 +3492,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'c':
 case 'C':
-if (!*key || !ap_casecmpstr(key, "hain")) {   /* chain */
+if (!*key || !strcasecmp(key, "hain")) {   /* chain */
 cfg->flags |= RULEFLAG_CHAIN;
 }
 else if (((*key == 'O' || *key == 'o') && !key[1])
- || !ap_casecmpstr(key, "ookie")) {   /* cookie */
+ || !strcasecmp(key, "ookie")) {   /* cookie */
 data_item *cp = cfg->cookie;
 
 if (!cp) {
@@ -3519,13 +3519,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'd':
 case 'D':
-if (!*key || !ap_casecmpstr(key, "PI") || !ap_casecmpstr(key,"iscardpath")) {
+if (!*key || !strcasecmp(key, "PI") || !strcasecmp(key,"iscardpath")) {
 cfg->flags |= (RULEFLAG_DISCARDPATHINFO);
 }
 break;
 case 'e':
 case 'E':
-if (!*key || !ap_casecmpstr(key, "nv")) { /* env */
+if (!*key || !strcasecmp(key, "nv")) { /* env */
 data_item *cp = cfg->env;
 
 if (!cp) {
@@ -3542,7 +3542,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 cp->next = NULL;
 cp->data = val;
 }
-else if (!ap_casecmpstr(key, "nd")) {/* end */
+else if (!strcasecmp(key, "nd")) {/* end */
 cfg->flags |= RULEFLAG_END;
 }
 else {
@@ -3552,7 +3552,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'f':
 case 'F':
-if (!*key || !ap_casecmpstr(key, "orbidden")) {   /* forbidden */
+if (!*key || !strcasecmp(key, "orbidden")) {   /* forbidden */
 cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
 cfg->forced_responsecode = HTTP_FORBIDDEN;
 }
@@ -3563,7 +3563,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'g':
 case 'G':
-if (!*key || !ap_casecmpstr(key, "one")) {/* gone */
+if (!*key || !strcasecmp(key, "one")) {/* gone */
 cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
 cfg->forced_responsecode = HTTP_GONE;
 }
@@ -3574,7 +3574,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'h':
 case 'H':
-if (!*key || !ap_casecmpstr(key, "andler")) { /* handler */
+if (!*key || !strcasecmp(key, "andler")) { /* handler */
 cfg->forced_handler = val;
 }
 else {
@@ -3583,7 +3583,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'l':
 case 'L':
-if (!*key || !ap_casecmpstr(key, "ast")) {/* last */
+if (!*key || !strcasecmp(key, 

Re: svn commit: r1715938 - /httpd/httpd/trunk/modules/cache/cache_util.c

[Yann Ylavic]

On Tue, Nov 24, 2015 at 4:49 PM, William A Rowe Jr  wrote:
>
> Does it make sense to expose the ap_tolower_ascii() function?

+1, possibly an inline one or a macro...

> We could call it ap_tolower_plaintext()

I prefer ascii().

Re: svn commit: r1715859 - /httpd/httpd/trunk/include/httpd.h

[William A Rowe Jr]

On Tue, Nov 24, 2015 at 9:46 AM, Eric Covener  wrote:

> On Tue, Nov 24, 2015 at 10:41 AM, William A Rowe Jr 
> wrote:
> > You are right, calling out "This implementation provides ASCII-ordering
> > of strings (EBCDIC text is also ASCII ordered on those platforms)"
> > might have been clearer?  Eric, what phrasing would make sense to you
> > as a user/maintainer of the httpd EBCDIC port?
>
> "Provides C/POSIX strcasecmp ordering, for alpha-numerics only"?
>

Well, we are sorting the entire ASCII so I guess we can drop "for
alpha-numerics only".

What we want to warn the user is that the sortation of extended characters
outside of
the normal C/POSIX range is undefined (not rejected), and that extended
characters
are not compared case-insensitively.

For extra confusion note the function says next to nothing in the XL
reference;

https://www-01.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.bpxbd00/rsrncp.htm?lang=en

but this function goes into some of the dirty details...

https://www-01.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.bpxbd00/risasc.htm

I found a nice summary of the state of EBCDIC for c programming over here;

http://www.longpelaexpertise.com/ezine/LostinTranslation1.php