Mongrel development - Feb 2008 - [PATCH] http11: ~6% performance increase in header parsing

Allocate one string object and avoid appending to it causing it
to be resized.  Additionally, optimize the string toupper copy
so that it''s done in a single pass.

Also, use an inline, locale-independent toupper() implementation
which should be more predictable for users with exotic locales
(HTTP header names are always ASCII).

The following test script was used:
require ''mongrel''
include Mongrel

parser = HttpParser.new
req = "GET /ruby HTTP/1.1\r\n" \
      "User-Agent: curl/7.12.3\r\n" \
      "Host: bogomips.org\r\n" \
      "Accept: */*\r\n" \
      "\r\n".freeze
hash = Hash.new
100000.times do
  parser.execute(hash, req, 0)
  parser.reset
  hash.clear
end
---
 ext/http11/http11.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/ext/http11/http11.c b/ext/http11/http11.c
index b0cd42e..90d9b40 100644
--- a/ext/http11/http11.c
+++ b/ext/http11/http11.c
@@ -7,7 +7,6 @@
 #include <assert.h>
 #include <string.h>
 #include "http11_parser.h"
-#include <ctype.h>
 
 static VALUE mMongrel;
 static VALUE cHttpParser;
@@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
 
 void http_field(void *data, const char *field, size_t flen, const char *value,
size_t vlen)
 {
-  char *ch, *end;
+  char *ch;
+  const char *fch;
+  int i;
   VALUE req = (VALUE)data;
   VALUE v = Qnil;
   VALUE f = Qnil;
@@ -71,15 +72,30 @@ void http_field(void *data, const char *field, size_t flen,
const char *value, s
   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
 
   v = rb_str_new(value, vlen);
-  f = rb_str_dup(global_http_prefix);
-  f = rb_str_buf_cat(f, field, flen); 
 
-  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch < end;
ch++) {
-    if(*ch == ''-'') {
+  /*
+   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
+   * in my testing, because: there''s no minimum allocation length (and
+   * no check for it, either), RSTRING(f)->len does not need to be
+   * written twice, and and RSTRING(f)->ptr[len] will already be
+   * null-terminated for us.
+   */
+  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
+  memcpy(RSTRING(f)->ptr,
+         RSTRING(global_http_prefix)->ptr,
+         RSTRING(global_http_prefix)->len);
+
+  i = (int)flen;
+  fch = field;
+  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
+  while(--i >= 0) {
+    if(*fch == ''-'') {
       *ch = ''_'';
     } else {
-      *ch = toupper(*ch);
+      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
     }
+    ++ch;
+    ++fch;
   }
 
   rb_hash_aset(req, f, v);
-- 
Eric Wong

Hey Eric, thanks!

Someone is testing patches from Eric? (this one and one earlier this
week).

If not i can test and merge it to head if ok :)

cheers,

filipe {
  @              icewall.org
  GPG         1024D/A6BA423E
  http://filipe.icewall.org/
}


On Thu, 28 Feb 2008, Eric Wong wrote:
> Allocate one string object and avoid appending to it causing it
> to be resized.  Additionally, optimize the string toupper copy
> so that it''s done in a single pass.
>
> Also, use an inline, locale-independent toupper() implementation
> which should be more predictable for users with exotic locales
> (HTTP header names are always ASCII).
>
> The following test script was used:
> require ''mongrel''
> include Mongrel
>
> parser = HttpParser.new
> req = "GET /ruby HTTP/1.1\r\n" \
>      "User-Agent: curl/7.12.3\r\n" \
>      "Host: bogomips.org\r\n" \
>      "Accept: */*\r\n" \
>      "\r\n".freeze
> hash = Hash.new
> 100000.times do
>  parser.execute(hash, req, 0)
>  parser.reset
>  hash.clear
> end
> ---
> ext/http11/http11.c |   30 +++++++++++++++++++++++-------
> 1 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/ext/http11/http11.c b/ext/http11/http11.c
> index b0cd42e..90d9b40 100644
> --- a/ext/http11/http11.c
> +++ b/ext/http11/http11.c
> @@ -7,7 +7,6 @@
> #include <assert.h>
> #include <string.h>
> #include "http11_parser.h"
> -#include <ctype.h>
>
> static VALUE mMongrel;
> static VALUE cHttpParser;
> @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>
> void http_field(void *data, const char *field, size_t flen, const char
*value, size_t vlen)
> {
> -  char *ch, *end;
> +  char *ch;
> +  const char *fch;
> +  int i;
>   VALUE req = (VALUE)data;
>   VALUE v = Qnil;
>   VALUE f = Qnil;
> @@ -71,15 +72,30 @@ void http_field(void *data, const char *field, size_t
flen, const char *value, s
>   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>
>   v = rb_str_new(value, vlen);
> -  f = rb_str_dup(global_http_prefix);
> -  f = rb_str_buf_cat(f, field, flen);
>
> -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch < end;
ch++) {
> -    if(*ch == ''-'') {
> +  /*
> +   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
> +   * in my testing, because: there''s no minimum allocation length
(and
> +   * no check for it, either), RSTRING(f)->len does not need to be
> +   * written twice, and and RSTRING(f)->ptr[len] will already be
> +   * null-terminated for us.
> +   */
> +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
> +  memcpy(RSTRING(f)->ptr,
> +         RSTRING(global_http_prefix)->ptr,
> +         RSTRING(global_http_prefix)->len);
> +
> +  i = (int)flen;
> +  fch = field;
> +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
> +  while(--i >= 0) {
> +    if(*fch == ''-'') {
>       *ch = ''_'';
>     } else {
> -      *ch = toupper(*ch);
> +      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
>     }
> +    ++ch;
> +    ++fch;
>   }
>
>   rb_hash_aset(req, f, v);
> -- 
> Eric Wong
> _______________________________________________
> Mongrel-development mailing list
> Mongrel-development at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-development
>

That would be great; I haven''t gotten to it. Can you merge it against
branches/stable_1-2?

Also make sure to add a test if it changes any API or adds any functionality.

Thanks

Evan

On Thu, Feb 28, 2008 at 6:30 PM, Filipe <filipe at icewall.org>
wrote:
>
>  Hey Eric, thanks!
>
>  Someone is testing patches from Eric? (this one and one earlier this
>  week).
>
>  If not i can test and merge it to head if ok :)
>
>  cheers,
>
>  filipe {
>   @              icewall.org
>   GPG         1024D/A6BA423E
>   http://filipe.icewall.org/
>
>
> }
>
>
>  On Thu, 28 Feb 2008, Eric Wong wrote:
>
>  > Allocate one string object and avoid appending to it causing it
>  > to be resized.  Additionally, optimize the string toupper copy
>  > so that it''s done in a single pass.
>  >
>  > Also, use an inline, locale-independent toupper() implementation
>  > which should be more predictable for users with exotic locales
>  > (HTTP header names are always ASCII).
>  >
>  > The following test script was used:
>  > require ''mongrel''
>  > include Mongrel
>  >
>  > parser = HttpParser.new
>  > req = "GET /ruby HTTP/1.1\r\n" \
>  >      "User-Agent: curl/7.12.3\r\n" \
>  >      "Host: bogomips.org\r\n" \
>  >      "Accept: */*\r\n" \
>  >      "\r\n".freeze
>  > hash = Hash.new
>  > 100000.times do
>  >  parser.execute(hash, req, 0)
>  >  parser.reset
>  >  hash.clear
>  > end
>  > ---
>  > ext/http11/http11.c |   30 +++++++++++++++++++++++-------
>  > 1 files changed, 23 insertions(+), 7 deletions(-)
>  >
>  > diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>  > index b0cd42e..90d9b40 100644
>  > --- a/ext/http11/http11.c
>  > +++ b/ext/http11/http11.c
>  > @@ -7,7 +7,6 @@
>  > #include <assert.h>
>  > #include <string.h>
>  > #include "http11_parser.h"
>  > -#include <ctype.h>
>  >
>  > static VALUE mMongrel;
>  > static VALUE cHttpParser;
>  > @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>  >
>  > void http_field(void *data, const char *field, size_t flen, const
char *value, size_t vlen)
>  > {
>  > -  char *ch, *end;
>  > +  char *ch;
>  > +  const char *fch;
>  > +  int i;
>  >   VALUE req = (VALUE)data;
>  >   VALUE v = Qnil;
>  >   VALUE f = Qnil;
>  > @@ -71,15 +72,30 @@ void http_field(void *data, const char *field,
size_t flen, const char *value, s
>  >   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>  >
>  >   v = rb_str_new(value, vlen);
>  > -  f = rb_str_dup(global_http_prefix);
>  > -  f = rb_str_buf_cat(f, field, flen);
>  >
>  > -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch
< end; ch++) {
>  > -    if(*ch == ''-'') {
>  > +  /*
>  > +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
>  > +   * in my testing, because: there''s no minimum allocation
length (and
>  > +   * no check for it, either), RSTRING(f)->len does not need to
be
>  > +   * written twice, and and RSTRING(f)->ptr[len] will already be
>  > +   * null-terminated for us.
>  > +   */
>  > +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
>  > +  memcpy(RSTRING(f)->ptr,
>  > +         RSTRING(global_http_prefix)->ptr,
>  > +         RSTRING(global_http_prefix)->len);
>  > +
>  > +  i = (int)flen;
>  > +  fch = field;
>  > +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
>  > +  while(--i >= 0) {
>  > +    if(*fch == ''-'') {
>  >       *ch = ''_'';
>  >     } else {
>  > -      *ch = toupper(*ch);
>  > +      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
>  >     }
>  > +    ++ch;
>  > +    ++fch;
>  >   }
>  >
>  >   rb_hash_aset(req, f, v);
>  > --
>  > Eric Wong
>  > _______________________________________________
>  > Mongrel-development mailing list
>  > Mongrel-development at rubyforge.org
>  > http://rubyforge.org/mailman/listinfo/mongrel-development
>  >
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Ok, will do it. Will let you know when done!

filipe {
  @              icewall.org
  GPG         1024D/A6BA423E
  http://filipe.icewall.org/
}


On Thu, 28 Feb 2008, Evan Weaver wrote:
> That would be great; I haven''t gotten to it. Can you merge it
against
> branches/stable_1-2?
>
> Also make sure to add a test if it changes any API or adds any
functionality.
>
> Thanks
>
> Evan
>
> On Thu, Feb 28, 2008 at 6:30 PM, Filipe <filipe at icewall.org>
wrote:
>>
>>  Hey Eric, thanks!
>>
>>  Someone is testing patches from Eric? (this one and one earlier this
>>  week).
>>
>>  If not i can test and merge it to head if ok :)
>>
>>  cheers,
>>
>>  filipe {
>>   @              icewall.org
>>   GPG         1024D/A6BA423E
>>   http://filipe.icewall.org/
>>
>>
>> }
>>
>>
>>  On Thu, 28 Feb 2008, Eric Wong wrote:
>>
>> > Allocate one string object and avoid appending to it causing it
>> > to be resized.  Additionally, optimize the string toupper copy
>> > so that it''s done in a single pass.
>> >
>> > Also, use an inline, locale-independent toupper() implementation
>> > which should be more predictable for users with exotic locales
>> > (HTTP header names are always ASCII).
>> >
>> > The following test script was used:
>> > require ''mongrel''
>> > include Mongrel
>> >
>> > parser = HttpParser.new
>> > req = "GET /ruby HTTP/1.1\r\n" \
>> >      "User-Agent: curl/7.12.3\r\n" \
>> >      "Host: bogomips.org\r\n" \
>> >      "Accept: */*\r\n" \
>> >      "\r\n".freeze
>> > hash = Hash.new
>> > 100000.times do
>> >  parser.execute(hash, req, 0)
>> >  parser.reset
>> >  hash.clear
>> > end
>> > ---
>> > ext/http11/http11.c |   30 +++++++++++++++++++++++-------
>> > 1 files changed, 23 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>> > index b0cd42e..90d9b40 100644
>> > --- a/ext/http11/http11.c
>> > +++ b/ext/http11/http11.c
>> > @@ -7,7 +7,6 @@
>> > #include <assert.h>
>> > #include <string.h>
>> > #include "http11_parser.h"
>> > -#include <ctype.h>
>> >
>> > static VALUE mMongrel;
>> > static VALUE cHttpParser;
>> > @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>> >
>> > void http_field(void *data, const char *field, size_t flen, const
char *value, size_t vlen)
>> > {
>> > -  char *ch, *end;
>> > +  char *ch;
>> > +  const char *fch;
>> > +  int i;
>> >   VALUE req = (VALUE)data;
>> >   VALUE v = Qnil;
>> >   VALUE f = Qnil;
>> > @@ -71,15 +72,30 @@ void http_field(void *data, const char *field,
size_t flen, const char *value, s
>> >   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>> >
>> >   v = rb_str_new(value, vlen);
>> > -  f = rb_str_dup(global_http_prefix);
>> > -  f = rb_str_buf_cat(f, field, flen);
>> >
>> > -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch
< end; ch++) {
>> > -    if(*ch == ''-'') {
>> > +  /*
>> > +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
>> > +   * in my testing, because: there''s no minimum
allocation length (and
>> > +   * no check for it, either), RSTRING(f)->len does not need
to be
>> > +   * written twice, and and RSTRING(f)->ptr[len] will already
be
>> > +   * null-terminated for us.
>> > +   */
>> > +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len +
flen);
>> > +  memcpy(RSTRING(f)->ptr,
>> > +         RSTRING(global_http_prefix)->ptr,
>> > +         RSTRING(global_http_prefix)->len);
>> > +
>> > +  i = (int)flen;
>> > +  fch = field;
>> > +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
>> > +  while(--i >= 0) {
>> > +    if(*fch == ''-'') {
>> >       *ch = ''_'';
>> >     } else {
>> > -      *ch = toupper(*ch);
>> > +      *ch = (*fch >= ''a'' && *fch
<= ''z'') ? (*fch & ~0x20) : *fch;
>> >     }
>> > +    ++ch;
>> > +    ++fch;
>> >   }
>> >
>> >   rb_hash_aset(req, f, v);
>> > --
>> > Eric Wong
>> > _______________________________________________
>> > Mongrel-development mailing list
>> > Mongrel-development at rubyforge.org
>> > http://rubyforge.org/mailman/listinfo/mongrel-development
>> >
>>  _______________________________________________
>>  Mongrel-development mailing list
>>  Mongrel-development at rubyforge.org
>>  http://rubyforge.org/mailman/listinfo/mongrel-development
>>
>
>
>
> -- 
> Evan Weaver
> Cloudburst, LLC
> _______________________________________________
> Mongrel-development mailing list
> Mongrel-development at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-development
>

This aught to use RSTRING_PTR(str) and RSTRING_LEN(str) e.g.
    for(ch = RSTRING_PTR(f), end = ch + RSTRING_LEN(f); ch < end; ch++) {
for 1.9 compatibility.

ry

On Fri, Feb 29, 2008 at 2:53 AM, Eric Wong <normalperson at yhbt.net>
wrote:
> Allocate one string object and avoid appending to it causing it
>  to be resized.  Additionally, optimize the string toupper copy
>  so that it''s done in a single pass.
>
>  Also, use an inline, locale-independent toupper() implementation
>  which should be more predictable for users with exotic locales
>  (HTTP header names are always ASCII).
>
>  The following test script was used:
>  require ''mongrel''
>  include Mongrel
>
>  parser = HttpParser.new
>  req = "GET /ruby HTTP/1.1\r\n" \
>       "User-Agent: curl/7.12.3\r\n" \
>       "Host: bogomips.org\r\n" \
>       "Accept: */*\r\n" \
>       "\r\n".freeze
>  hash = Hash.new
>  100000.times do
>   parser.execute(hash, req, 0)
>   parser.reset
>   hash.clear
>  end
>  ---
>   ext/http11/http11.c |   30 +++++++++++++++++++++++-------
>   1 files changed, 23 insertions(+), 7 deletions(-)
>
>  diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>  index b0cd42e..90d9b40 100644
>  --- a/ext/http11/http11.c
>  +++ b/ext/http11/http11.c
>  @@ -7,7 +7,6 @@
>   #include <assert.h>
>   #include <string.h>
>   #include "http11_parser.h"
>  -#include <ctype.h>
>
>   static VALUE mMongrel;
>   static VALUE cHttpParser;
>  @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>
>   void http_field(void *data, const char *field, size_t flen, const char
*value, size_t vlen)
>   {
>  -  char *ch, *end;
>  +  char *ch;
>  +  const char *fch;
>  +  int i;
>    VALUE req = (VALUE)data;
>    VALUE v = Qnil;
>    VALUE f = Qnil;
>  @@ -71,15 +72,30 @@ void http_field(void *data, const char *field, size_t
flen, const char *value, s
>    VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>
>    v = rb_str_new(value, vlen);
>  -  f = rb_str_dup(global_http_prefix);
>  -  f = rb_str_buf_cat(f, field, flen);
>
>  -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch <
end; ch++) {
>  -    if(*ch == ''-'') {
>  +  /*
>  +   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
>  +   * in my testing, because: there''s no minimum allocation
length (and
>  +   * no check for it, either), RSTRING(f)->len does not need to be
>  +   * written twice, and and RSTRING(f)->ptr[len] will already be
>  +   * null-terminated for us.
>  +   */
>  +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
>  +  memcpy(RSTRING(f)->ptr,
>  +         RSTRING(global_http_prefix)->ptr,
>  +         RSTRING(global_http_prefix)->len);
>  +
>  +  i = (int)flen;
>  +  fch = field;
>  +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
>  +  while(--i >= 0) {
>  +    if(*fch == ''-'') {
>        *ch = ''_'';
>      } else {
>  -      *ch = toupper(*ch);
>  +      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
>      }
>  +    ++ch;
>  +    ++fch;
>    }
>
>    rb_hash_aset(req, f, v);
>  --
>  Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>

while i''m nitpicking...:P

#define ASCII_UPPER(ch) (''a'' <= ch && ch <=
''z'' ? ch - ''a'' + ''A'' : ch)

    memcpy( RSTRING_PTR(f)
          , RSTRING_PTR(global_http_prefix)
          , RSTRING_LEN(global_http_prefix)
          );
    for(i = 0; i < length; i++) {
      ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) + i;
      if(field[i] == ''-'') {
        *ch = ''_'';
      } else {
        *ch = ASCII_UPPER(field[i]);
      }
    }


On Fri, Feb 29, 2008 at 11:03 AM, ry dahl <ry at tinyclouds.org>
wrote:
> This aught to use RSTRING_PTR(str) and RSTRING_LEN(str) e.g.
>     for(ch = RSTRING_PTR(f), end = ch + RSTRING_LEN(f); ch < end; ch++)
{
>  for 1.9 compatibility.
>
>  ry
>
>
>
>  On Fri, Feb 29, 2008 at 2:53 AM, Eric Wong <normalperson at
yhbt.net> wrote:
>  > Allocate one string object and avoid appending to it causing it
>  >  to be resized.  Additionally, optimize the string toupper copy
>  >  so that it''s done in a single pass.
>  >
>  >  Also, use an inline, locale-independent toupper() implementation
>  >  which should be more predictable for users with exotic locales
>  >  (HTTP header names are always ASCII).
>  >
>  >  The following test script was used:
>  >  require ''mongrel''
>  >  include Mongrel
>  >
>  >  parser = HttpParser.new
>  >  req = "GET /ruby HTTP/1.1\r\n" \
>  >       "User-Agent: curl/7.12.3\r\n" \
>  >       "Host: bogomips.org\r\n" \
>  >       "Accept: */*\r\n" \
>  >       "\r\n".freeze
>  >  hash = Hash.new
>  >  100000.times do
>  >   parser.execute(hash, req, 0)
>  >   parser.reset
>  >   hash.clear
>  >  end
>  >  ---
>  >   ext/http11/http11.c |   30 +++++++++++++++++++++++-------
>  >   1 files changed, 23 insertions(+), 7 deletions(-)
>  >
>  >  diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>  >  index b0cd42e..90d9b40 100644
>  >  --- a/ext/http11/http11.c
>  >  +++ b/ext/http11/http11.c
>  >  @@ -7,7 +7,6 @@
>  >   #include <assert.h>
>  >   #include <string.h>
>  >   #include "http11_parser.h"
>  >  -#include <ctype.h>
>  >
>  >   static VALUE mMongrel;
>  >   static VALUE cHttpParser;
>  >  @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>  >
>  >   void http_field(void *data, const char *field, size_t flen, const
char *value, size_t vlen)
>  >   {
>  >  -  char *ch, *end;
>  >  +  char *ch;
>  >  +  const char *fch;
>  >  +  int i;
>  >    VALUE req = (VALUE)data;
>  >    VALUE v = Qnil;
>  >    VALUE f = Qnil;
>  >  @@ -71,15 +72,30 @@ void http_field(void *data, const char *field,
size_t flen, const char *value, s
>  >    VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>  >
>  >    v = rb_str_new(value, vlen);
>  >  -  f = rb_str_dup(global_http_prefix);
>  >  -  f = rb_str_buf_cat(f, field, flen);
>  >
>  >  -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch
< end; ch++) {
>  >  -    if(*ch == ''-'') {
>  >  +  /*
>  >  +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
>  >  +   * in my testing, because: there''s no minimum allocation
length (and
>  >  +   * no check for it, either), RSTRING(f)->len does not need to
be
>  >  +   * written twice, and and RSTRING(f)->ptr[len] will already be
>  >  +   * null-terminated for us.
>  >  +   */
>  >  +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
>  >  +  memcpy(RSTRING(f)->ptr,
>  >  +         RSTRING(global_http_prefix)->ptr,
>  >  +         RSTRING(global_http_prefix)->len);
>  >  +
>  >  +  i = (int)flen;
>  >  +  fch = field;
>  >  +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
>  >  +  while(--i >= 0) {
>  >  +    if(*fch == ''-'') {
>  >        *ch = ''_'';
>  >      } else {
>  >  -      *ch = toupper(*ch);
>  >  +      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
>  >      }
>  >  +    ++ch;
>  >  +    ++fch;
>  >    }
>  >
>  >    rb_hash_aset(req, f, v);
>  >  --
>  >  Eric Wong
>  >  _______________________________________________
>  >  Mongrel-development mailing list
>  >  Mongrel-development at rubyforge.org
>  >  http://rubyforge.org/mailman/listinfo/mongrel-development
>  >
>

ry dahl <ry at tinyclouds.org> wrote:
> while i''m nitpicking...:P
> 
> #define ASCII_UPPER(ch) (''a'' <= ch && ch <=
''z'' ? ch - ''a'' + ''A'' : ch)
> 
>     memcpy( RSTRING_PTR(f)
>           , RSTRING_PTR(global_http_prefix)
>           , RSTRING_LEN(global_http_prefix)
>           );
>     for(i = 0; i < length; i++) {
>       ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) + i;
>       if(field[i] == ''-'') {
>         *ch = ''_'';
>       } else {
>         *ch = ASCII_UPPER(field[i]);
>       }
>     }
The ASCII_UPPER helps readability, thanks.

I haven''t had a chance to benchmark your for loop vs my while loop I
wrote, but given the wide variety of compilers, versions and options, I
expect my while loop to be slightly faster in general.

Additionally, reordering the if/else condition to put the more common
case first didn''t seem to help in my case (gcc 4.1.1-21 from Debian,
-O2), so I left it alone.

However, I haven''t tried __builtin_expect() here, either (fast_xs did
benefit from __builtin_expect() in my tests).

Perhaps rolling the checks together would be fastest, eliminating
a redundant check for ''-'' if it''s lowercase, still:

  *ch = (''a'' <= *fch && *fch <=
''z'') ?
        (*fch - ''a'' + ''A'') :
        (*fch != ''-'' ? *fch : ''_'');

Not that other inefficiencies drastically overshadow the increases
we get here :)
> On Fri, Feb 29, 2008 at 11:03 AM, ry dahl <ry at tinyclouds.org>
wrote:
> > This aught to use RSTRING_PTR(str) and RSTRING_LEN(str) e.g.
> >     for(ch = RSTRING_PTR(f), end = ch + RSTRING_LEN(f); ch < end;
ch++) {
> >  for 1.9 compatibility.
Sure.  I have a partial set of compatibility macros from fast_xs if we
don''t have them already so <=1.8.5 doesn''t break:

http://bogomips.org/ruby/fast_xs-0.6/ext/fast_xs/ruby_1_9_compat.h
> >  On Fri, Feb 29, 2008 at 2:53 AM, Eric Wong <normalperson at
yhbt.net> wrote:
> >  > Allocate one string object and avoid appending to it causing it
> >  >  to be resized.  Additionally, optimize the string toupper copy
> >  >  so that it''s done in a single pass.
> >  >
> >  >  Also, use an inline, locale-independent toupper()
implementation
> >  >  which should be more predictable for users with exotic locales
> >  >  (HTTP header names are always ASCII).
> >  >
> >  >  The following test script was used:
> >  >  require ''mongrel''
> >  >  include Mongrel
> >  >
> >  >  parser = HttpParser.new
> >  >  req = "GET /ruby HTTP/1.1\r\n" \
> >  >       "User-Agent: curl/7.12.3\r\n" \
> >  >       "Host: bogomips.org\r\n" \
> >  >       "Accept: */*\r\n" \
> >  >       "\r\n".freeze
> >  >  hash = Hash.new
> >  >  100000.times do
> >  >   parser.execute(hash, req, 0)
> >  >   parser.reset
> >  >   hash.clear
> >  >  end
> >  >  ---
> >  >   ext/http11/http11.c |   30 +++++++++++++++++++++++-------
> >  >   1 files changed, 23 insertions(+), 7 deletions(-)
> >  >
> >  >  diff --git a/ext/http11/http11.c b/ext/http11/http11.c
> >  >  index b0cd42e..90d9b40 100644
> >  >  --- a/ext/http11/http11.c
> >  >  +++ b/ext/http11/http11.c
> >  >  @@ -7,7 +7,6 @@
> >  >   #include <assert.h>
> >  >   #include <string.h>
> >  >   #include "http11_parser.h"
> >  >  -#include <ctype.h>
> >  >
> >  >   static VALUE mMongrel;
> >  >   static VALUE cHttpParser;
> >  >  @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
> >  >
> >  >   void http_field(void *data, const char *field, size_t flen,
const char *value, size_t vlen)
> >  >   {
> >  >  -  char *ch, *end;
> >  >  +  char *ch;
> >  >  +  const char *fch;
> >  >  +  int i;
> >  >    VALUE req = (VALUE)data;
> >  >    VALUE v = Qnil;
> >  >    VALUE f = Qnil;
> >  >  @@ -71,15 +72,30 @@ void http_field(void *data, const char
*field, size_t flen, const char *value, s
> >  >    VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
> >  >
> >  >    v = rb_str_new(value, vlen);
> >  >  -  f = rb_str_dup(global_http_prefix);
> >  >  -  f = rb_str_buf_cat(f, field, flen);
> >  >
> >  >  -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len;
ch < end; ch++) {
> >  >  -    if(*ch == ''-'') {
> >  >  +  /*
> >  >  +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
> >  >  +   * in my testing, because: there''s no minimum
allocation length (and
> >  >  +   * no check for it, either), RSTRING(f)->len does not
need to be
> >  >  +   * written twice, and and RSTRING(f)->ptr[len] will
already be
> >  >  +   * null-terminated for us.
> >  >  +   */
> >  >  +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len +
flen);
> >  >  +  memcpy(RSTRING(f)->ptr,
> >  >  +         RSTRING(global_http_prefix)->ptr,
> >  >  +         RSTRING(global_http_prefix)->len);
> >  >  +
> >  >  +  i = (int)flen;
> >  >  +  fch = field;
> >  >  +  ch = RSTRING(f)->ptr +
RSTRING(global_http_prefix)->len;
> >  >  +  while(--i >= 0) {
> >  >  +    if(*fch == ''-'') {
> >  >        *ch = ''_'';
> >  >      } else {
> >  >  -      *ch = toupper(*ch);
> >  >  +      *ch = (*fch >= ''a'' && *fch
<= ''z'') ? (*fch & ~0x20) : *fch;
> >  >      }
> >  >  +    ++ch;
> >  >  +    ++fch;
> >  >    }
> >  >
> >  >    rb_hash_aset(req, f, v);
> >  >  --
> >  >  Eric Wong
> >  >  _______________________________________________
> >  >  Mongrel-development mailing list
> >  >  Mongrel-development at rubyforge.org
> >  >  http://rubyforge.org/mailman/listinfo/mongrel-development
> >  >
> >
> _______________________________________________
> Mongrel-development mailing list
> Mongrel-development at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-development
-- 
Eric Wong

Filipe <filipe at icewall.org> wrote:
> 
> Hey Eric, thanks!
 
You''re welcome :)
> Someone is testing patches from Eric? (this one and one earlier this
> week).
> 
> If not i can test and merge it to head if ok :)
I''d also like the patch I submitted for ticket #14 looked at (and
hopefully in 1.1.4), too:

  http://mongrel.rubyforge.org/ticket/14

I actually consider the patch for ticket #14 to be more important than
either of the patches I submitted here.

-- 
Eric Wong

We will need to target 1.2 for these since 1.1.4 is already out. That
makes more sense anyway since performance work is on the roadmap for
1.2.

Also, please, if you can make sure the changes are exercised by some
test. You may have to add it because our coverage is not totally
stellar in all places. If not, though add a ticket about the lack of
the test and assign it to me.

Evan

On Fri, Feb 29, 2008 at 5:27 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> ry dahl <ry at tinyclouds.org> wrote:
>
> > while i''m nitpicking...:P
>  >
>  > #define ASCII_UPPER(ch) (''a'' <= ch && ch
<= ''z'' ? ch - ''a'' + ''A''
: ch)
>  >
>  >     memcpy( RSTRING_PTR(f)
>  >           , RSTRING_PTR(global_http_prefix)
>  >           , RSTRING_LEN(global_http_prefix)
>  >           );
>  >     for(i = 0; i < length; i++) {
>  >       ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) + i;
>  >       if(field[i] == ''-'') {
>  >         *ch = ''_'';
>  >       } else {
>  >         *ch = ASCII_UPPER(field[i]);
>  >       }
>  >     }
>
>  The ASCII_UPPER helps readability, thanks.
>
>  I haven''t had a chance to benchmark your for loop vs my while
loop I
>  wrote, but given the wide variety of compilers, versions and options, I
>  expect my while loop to be slightly faster in general.
>
>  Additionally, reordering the if/else condition to put the more common
>  case first didn''t seem to help in my case (gcc 4.1.1-21 from
Debian,
>  -O2), so I left it alone.
>
>  However, I haven''t tried __builtin_expect() here, either (fast_xs
did
>  benefit from __builtin_expect() in my tests).
>
>  Perhaps rolling the checks together would be fastest, eliminating
>  a redundant check for ''-'' if it''s lowercase,
still:
>
>   *ch = (''a'' <= *fch && *fch <=
''z'') ?
>         (*fch - ''a'' + ''A'') :
>         (*fch != ''-'' ? *fch : ''_'');
>
>  Not that other inefficiencies drastically overshadow the increases
>  we get here :)
>
>
>  > On Fri, Feb 29, 2008 at 11:03 AM, ry dahl <ry at
tinyclouds.org> wrote:
>  > > This aught to use RSTRING_PTR(str) and RSTRING_LEN(str) e.g.
>  > >     for(ch = RSTRING_PTR(f), end = ch + RSTRING_LEN(f); ch <
end; ch++) {
>  > >  for 1.9 compatibility.
>
>  Sure.  I have a partial set of compatibility macros from fast_xs if we
>  don''t have them already so <=1.8.5 doesn''t break:
>
>  http://bogomips.org/ruby/fast_xs-0.6/ext/fast_xs/ruby_1_9_compat.h
>
>
>
>  > >  On Fri, Feb 29, 2008 at 2:53 AM, Eric Wong <normalperson at
yhbt.net> wrote:
>  > >  > Allocate one string object and avoid appending to it
causing it
>  > >  >  to be resized.  Additionally, optimize the string toupper
copy
>  > >  >  so that it''s done in a single pass.
>  > >  >
>  > >  >  Also, use an inline, locale-independent toupper()
implementation
>  > >  >  which should be more predictable for users with exotic
locales
>  > >  >  (HTTP header names are always ASCII).
>  > >  >
>  > >  >  The following test script was used:
>  > >  >  require ''mongrel''
>  > >  >  include Mongrel
>  > >  >
>  > >  >  parser = HttpParser.new
>  > >  >  req = "GET /ruby HTTP/1.1\r\n" \
>  > >  >       "User-Agent: curl/7.12.3\r\n" \
>  > >  >       "Host: bogomips.org\r\n" \
>  > >  >       "Accept: */*\r\n" \
>  > >  >       "\r\n".freeze
>  > >  >  hash = Hash.new
>  > >  >  100000.times do
>  > >  >   parser.execute(hash, req, 0)
>  > >  >   parser.reset
>  > >  >   hash.clear
>  > >  >  end
>  > >  >  ---
>  > >  >   ext/http11/http11.c |   30
+++++++++++++++++++++++-------
>  > >  >   1 files changed, 23 insertions(+), 7 deletions(-)
>  > >  >
>  > >  >  diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>  > >  >  index b0cd42e..90d9b40 100644
>  > >  >  --- a/ext/http11/http11.c
>  > >  >  +++ b/ext/http11/http11.c
>  > >  >  @@ -7,7 +7,6 @@
>  > >  >   #include <assert.h>
>  > >  >   #include <string.h>
>  > >  >   #include "http11_parser.h"
>  > >  >  -#include <ctype.h>
>  > >  >
>  > >  >   static VALUE mMongrel;
>  > >  >   static VALUE cHttpParser;
>  > >  >  @@ -62,7 +61,9 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 +
32)));
>  > >  >
>  > >  >   void http_field(void *data, const char *field, size_t
flen, const char *value, size_t vlen)
>  > >  >   {
>  > >  >  -  char *ch, *end;
>  > >  >  +  char *ch;
>  > >  >  +  const char *fch;
>  > >  >  +  int i;
>  > >  >    VALUE req = (VALUE)data;
>  > >  >    VALUE v = Qnil;
>  > >  >    VALUE f = Qnil;
>  > >  >  @@ -71,15 +72,30 @@ void http_field(void *data, const
char *field, size_t flen, const char *value, s
>  > >  >    VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>  > >  >
>  > >  >    v = rb_str_new(value, vlen);
>  > >  >  -  f = rb_str_dup(global_http_prefix);
>  > >  >  -  f = rb_str_buf_cat(f, field, flen);
>  > >  >
>  > >  >  -  for(ch = RSTRING(f)->ptr, end = ch +
RSTRING(f)->len; ch < end; ch++) {
>  > >  >  -    if(*ch == ''-'') {
>  > >  >  +  /*
>  > >  >  +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
>  > >  >  +   * in my testing, because: there''s no minimum
allocation length (and
>  > >  >  +   * no check for it, either), RSTRING(f)->len does
not need to be
>  > >  >  +   * written twice, and and RSTRING(f)->ptr[len] will
already be
>  > >  >  +   * null-terminated for us.
>  > >  >  +   */
>  > >  >  +  f = rb_str_new(NULL,
RSTRING(global_http_prefix)->len + flen);
>  > >  >  +  memcpy(RSTRING(f)->ptr,
>  > >  >  +         RSTRING(global_http_prefix)->ptr,
>  > >  >  +         RSTRING(global_http_prefix)->len);
>  > >  >  +
>  > >  >  +  i = (int)flen;
>  > >  >  +  fch = field;
>  > >  >  +  ch = RSTRING(f)->ptr +
RSTRING(global_http_prefix)->len;
>  > >  >  +  while(--i >= 0) {
>  > >  >  +    if(*fch == ''-'') {
>  > >  >        *ch = ''_'';
>  > >  >      } else {
>  > >  >  -      *ch = toupper(*ch);
>  > >  >  +      *ch = (*fch >= ''a'' &&
*fch <= ''z'') ? (*fch & ~0x20) : *fch;
>  > >  >      }
>  > >  >  +    ++ch;
>  > >  >  +    ++fch;
>  > >  >    }
>  > >  >
>  > >  >    rb_hash_aset(req, f, v);
>  > >  >  --
>  > >  >  Eric Wong
>  > >  >  _______________________________________________
>  > >  >  Mongrel-development mailing list
>  > >  >  Mongrel-development at rubyforge.org
>  > >  >  http://rubyforge.org/mailman/listinfo/mongrel-development
>  > >  >
>  > >
>  > _______________________________________________
>  > Mongrel-development mailing list
>  > Mongrel-development at rubyforge.org
>  > http://rubyforge.org/mailman/listinfo/mongrel-development
>
>  --
>
>
> Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Tests all run correctly on the stable_1-2 branch now.
---

  Since I''m looking at the tests... :)

  Evan Weaver <evan at cloudbur.st> wrote:
  > That would be great; I haven''t gotten to it. Can you merge it
against
  > branches/stable_1-2?
  > 
  > Also make sure to add a test if it changes any API or adds any
functionality.
  > 
  > Thanks
  > 
  > Evan

 test/unit/test_ws.rb |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/unit/test_ws.rb b/test/unit/test_ws.rb
index d239e18..68a79a8 100644
--- a/test/unit/test_ws.rb
+++ b/test/unit/test_ws.rb
@@ -4,7 +4,7 @@
 # Additional work donated by contributors.  See
http://mongrel.rubyforge.org/attributions.html
 # for more information.
 
-require ''test/testhelp''
+require ''test/test_helper''
 
 include Mongrel
 
-- 
Eric Wong

Applied, thanks.

Do you want a commit bit? What''s your RubyForge name?

Evan

On Sat, Mar 1, 2008 at 4:38 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Tests all run correctly on the stable_1-2 branch now.
>  ---
>
>   Since I''m looking at the tests... :)
>
>   Evan Weaver <evan at cloudbur.st> wrote:
>   > That would be great; I haven''t gotten to it. Can you merge
it against
>   > branches/stable_1-2?
>   >
>   > Also make sure to add a test if it changes any API or adds any
functionality.
>   >
>   > Thanks
>   >
>   > Evan
>
>   test/unit/test_ws.rb |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
>  diff --git a/test/unit/test_ws.rb b/test/unit/test_ws.rb
>  index d239e18..68a79a8 100644
>  --- a/test/unit/test_ws.rb
>  +++ b/test/unit/test_ws.rb
>  @@ -4,7 +4,7 @@
>   # Additional work donated by contributors.  See
http://mongrel.rubyforge.org/attributions.html
>   # for more information.
>
>  -require ''test/testhelp''
>  +require ''test/test_helper''
>
>   include Mongrel
>
>  --
>  Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Evan Weaver <evan at cloudbur.st> wrote:
> Applied, thanks.
> 
> Do you want a commit bit? What''s your RubyForge name?
Sure, I''m normalperson: http://rubyforge.org/users/normalperson/

Thanks!
> Evan
> 
> On Sat, Mar 1, 2008 at 4:38 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> > Tests all run correctly on the stable_1-2 branch now.
> >  ---
> >
> >   Since I''m looking at the tests... :)
> >
> >   Evan Weaver <evan at cloudbur.st> wrote:
> >   > That would be great; I haven''t gotten to it. Can you
merge it against
> >   > branches/stable_1-2?
> >   >
> >   > Also make sure to add a test if it changes any API or adds any
functionality.
> >   >
> >   > Thanks
> >   >
> >   > Evan
> >
> >   test/unit/test_ws.rb |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >  diff --git a/test/unit/test_ws.rb b/test/unit/test_ws.rb
> >  index d239e18..68a79a8 100644
> >  --- a/test/unit/test_ws.rb
> >  +++ b/test/unit/test_ws.rb
> >  @@ -4,7 +4,7 @@
> >   # Additional work donated by contributors.  See
http://mongrel.rubyforge.org/attributions.html
> >   # for more information.
> >
> >  -require ''test/testhelp''
> >  +require ''test/test_helper''
> >
> >   include Mongrel
> >
> >  --
> >  Eric Wong

Added you.

Evan

On Sat, Mar 1, 2008 at 5:24 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Evan Weaver <evan at cloudbur.st> wrote:
>
> > Applied, thanks.
>  >
>  > Do you want a commit bit? What''s your RubyForge name?
>
>  Sure, I''m normalperson: http://rubyforge.org/users/normalperson/
>
>  Thanks!
>
>
>
>  > Evan
>  >
>  > On Sat, Mar 1, 2008 at 4:38 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>  > > Tests all run correctly on the stable_1-2 branch now.
>  > >  ---
>  > >
>  > >   Since I''m looking at the tests... :)
>  > >
>  > >   Evan Weaver <evan at cloudbur.st> wrote:
>  > >   > That would be great; I haven''t gotten to it. Can
you merge it against
>  > >   > branches/stable_1-2?
>  > >   >
>  > >   > Also make sure to add a test if it changes any API or
adds any functionality.
>  > >   >
>  > >   > Thanks
>  > >   >
>  > >   > Evan
>  > >
>  > >   test/unit/test_ws.rb |    2 +-
>  > >   1 files changed, 1 insertions(+), 1 deletions(-)
>  > >
>  > >  diff --git a/test/unit/test_ws.rb b/test/unit/test_ws.rb
>  > >  index d239e18..68a79a8 100644
>  > >  --- a/test/unit/test_ws.rb
>  > >  +++ b/test/unit/test_ws.rb
>  > >  @@ -4,7 +4,7 @@
>  > >   # Additional work donated by contributors.  See
http://mongrel.rubyforge.org/attributions.html
>  > >   # for more information.
>  > >
>  > >  -require ''test/testhelp''
>  > >  +require ''test/test_helper''
>  > >
>  > >   include Mongrel
>  > >
>  > >  --
>  > >  Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Eric Wong <normalperson at yhbt.net> wrote:
> ry dahl <ry at tinyclouds.org> wrote:
> > while i''m nitpicking...:P
> > 
> > #define ASCII_UPPER(ch) (''a'' <= ch && ch
<= ''z'' ? ch - ''a'' + ''A''
: ch)
> > 
> >     memcpy( RSTRING_PTR(f)
> >           , RSTRING_PTR(global_http_prefix)
> >           , RSTRING_LEN(global_http_prefix)
> >           );
> >     for(i = 0; i < length; i++) {
> >       ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) + i;
> >       if(field[i] == ''-'') {
> >         *ch = ''_'';
> >       } else {
> >         *ch = ASCII_UPPER(field[i]);
> >       }
> >     }
> 
> The ASCII_UPPER helps readability, thanks.
> 
> I haven''t had a chance to benchmark your for loop vs my while loop
I
> wrote, but given the wide variety of compilers, versions and options, I
> expect my while loop to be slightly faster in general.
My version is about 0.3% faster :)
> Additionally, reordering the if/else condition to put the more common
> case first didn''t seem to help in my case (gcc 4.1.1-21 from
Debian,
> -O2), so I left it alone.
> 
> However, I haven''t tried __builtin_expect() here, either (fast_xs
did
> benefit from __builtin_expect() in my tests).
__builtin_expect didn''t help, probably because the common cases
(lowercase chars) for HTTP headers don''t overwhelmingly favor the less
common cases (non-lowercase chars).
> Perhaps rolling the checks together would be fastest, eliminating
> a redundant check for ''-'' if it''s lowercase,
still:
> 
>   *ch = (''a'' <= *fch && *fch <=
''z'') ?
>         (*fch - ''a'' + ''A'') :
>         (*fch != ''-'' ? *fch : ''_'');
 
> Not that other inefficiencies drastically overshadow the increases
> we get here :)
Here''s my updated version, I''ve removed the extra
''i'' variable and made
the code a little more compact (and hopefully more readable) (with no
measurable performance impact from my original).

I kept the bitwise operation for uppercasing instead of relying on
subtraction since it''s typically faster (no sign/overflow checks)
although hard to measure in real-world performance here (my compiler
(gcc 4.1.1-21 -O2 on Debian/x86) preserved the subb instruction).

diff --git a/ext/http11/ext_help.h b/ext/http11/ext_help.h
index 8b4d754..1017c64 100644
--- a/ext/http11/ext_help.h
+++ b/ext/http11/ext_help.h
@@ -4,6 +4,8 @@
 #define RAISE_NOT_NULL(T) if(T == NULL) rb_raise(rb_eArgError, "NULL found
for " # T " when shouldn''t be.");
 #define DATA_GET(from,type,name) Data_Get_Struct(from,type,name);
RAISE_NOT_NULL(name);
 #define REQUIRE_TYPE(V, T) if(TYPE(V) != T) rb_raise(rb_eTypeError, "Wrong
argument type for " # V " required " # T);
+#define ASCII_UPCASE_CHAR(ch) (ch & ~0x20)
+
 
 #ifdef DEBUG
 #define TRACE()  fprintf(stderr, "> %s:%d:%s\n", __FILE__,
__LINE__, __FUNCTION__)
diff --git a/ext/http11/http11.c b/ext/http11/http11.c
index 2982467..8e46e4c 100644
--- a/ext/http11/http11.c
+++ b/ext/http11/http11.c
@@ -7,7 +7,6 @@
 #include <assert.h>
 #include <string.h>
 #include "http11_parser.h"
-#include <ctype.h>
 
 static VALUE mMongrel;
 static VALUE cHttpParser;
@@ -62,7 +61,8 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
 
 void http_field(void *data, const char *field, size_t flen, const char *value,
size_t vlen)
 {
-  char *ch, *end;
+  char *ch;
+  const char *fch;
   VALUE req = (VALUE)data;
   VALUE v = Qnil;
   VALUE f = Qnil;
@@ -71,15 +71,24 @@ void http_field(void *data, const char *field, size_t flen,
const char *value, s
   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
 
   v = rb_str_new(value, vlen);
-  f = rb_str_dup(global_http_prefix);
-  f = rb_str_buf_cat(f, field, flen); 
 
-  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch < end;
ch++) {
-    if(*ch == ''-'') {
-      *ch = ''_'';
-    } else {
-      *ch = toupper(*ch);
-    }
+  /*
+   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
+   * in my testing, because: there''s no minimum allocation length (and
+   * no check for it, either), RSTRING(f)->len does not need to be
+   * written twice, and and RSTRING(f)->ptr[len] will already be
+   * null-terminated for us.
+   */
+  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
+  memcpy(RSTRING(f)->ptr,
+         RSTRING(global_http_prefix)->ptr,
+         RSTRING(global_http_prefix)->len);
+
+  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
+  for(fch = field; flen-- != 0; ++fch) {
+    *ch++ = (*fch >= ''a'' && *fch <=
''z'') ?
+            ASCII_UPCASE_CHAR(*fch) :
+            (*fch == ''-'' ? ''_'' : *fch);
   }
 
   rb_hash_aset(req, f, v);
-- 
Eric Wong

Hey Eric! as now you have commit permission, please apply the patches
yourself to head. after testing it on head we can add this to branches,
ok?

Cheers,

filipe {
  @              icewall.org
  GPG         1024D/A6BA423E
  http://filipe.icewall.org/
}


On Sat, 1 Mar 2008, Eric Wong wrote:
> Eric Wong <normalperson at yhbt.net> wrote:
>> ry dahl <ry at tinyclouds.org> wrote:
>>> while i''m nitpicking...:P
>>>
>>> #define ASCII_UPPER(ch) (''a'' <= ch &&
ch <= ''z'' ? ch - ''a'' +
''A'' : ch)
>>>
>>>     memcpy( RSTRING_PTR(f)
>>>           , RSTRING_PTR(global_http_prefix)
>>>           , RSTRING_LEN(global_http_prefix)
>>>           );
>>>     for(i = 0; i < length; i++) {
>>>       ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) + i;
>>>       if(field[i] == ''-'') {
>>>         *ch = ''_'';
>>>       } else {
>>>         *ch = ASCII_UPPER(field[i]);
>>>       }
>>>     }
>>
>> The ASCII_UPPER helps readability, thanks.
>>
>> I haven''t had a chance to benchmark your for loop vs my while
loop I
>> wrote, but given the wide variety of compilers, versions and options, I
>> expect my while loop to be slightly faster in general.
>
> My version is about 0.3% faster :)
>
>> Additionally, reordering the if/else condition to put the more common
>> case first didn''t seem to help in my case (gcc 4.1.1-21 from
Debian,
>> -O2), so I left it alone.
>>
>> However, I haven''t tried __builtin_expect() here, either
(fast_xs did
>> benefit from __builtin_expect() in my tests).
>
> __builtin_expect didn''t help, probably because the common cases
> (lowercase chars) for HTTP headers don''t overwhelmingly favor the
less
> common cases (non-lowercase chars).
>
>> Perhaps rolling the checks together would be fastest, eliminating
>> a redundant check for ''-'' if it''s lowercase,
still:
>>
>>   *ch = (''a'' <= *fch && *fch <=
''z'') ?
>>         (*fch - ''a'' + ''A'') :
>>         (*fch != ''-'' ? *fch : ''_'');
>
>> Not that other inefficiencies drastically overshadow the increases
>> we get here :)
>
> Here''s my updated version, I''ve removed the extra
''i'' variable and made
> the code a little more compact (and hopefully more readable) (with no
> measurable performance impact from my original).
>
> I kept the bitwise operation for uppercasing instead of relying on
> subtraction since it''s typically faster (no sign/overflow checks)
> although hard to measure in real-world performance here (my compiler
> (gcc 4.1.1-21 -O2 on Debian/x86) preserved the subb instruction).
>
> diff --git a/ext/http11/ext_help.h b/ext/http11/ext_help.h
> index 8b4d754..1017c64 100644
> --- a/ext/http11/ext_help.h
> +++ b/ext/http11/ext_help.h
> @@ -4,6 +4,8 @@
> #define RAISE_NOT_NULL(T) if(T == NULL) rb_raise(rb_eArgError, "NULL
found for " # T " when shouldn''t be.");
> #define DATA_GET(from,type,name) Data_Get_Struct(from,type,name);
RAISE_NOT_NULL(name);
> #define REQUIRE_TYPE(V, T) if(TYPE(V) != T) rb_raise(rb_eTypeError,
"Wrong argument type for " # V " required " # T);
> +#define ASCII_UPCASE_CHAR(ch) (ch & ~0x20)
> +
>
> #ifdef DEBUG
> #define TRACE()  fprintf(stderr, "> %s:%d:%s\n", __FILE__,
__LINE__, __FUNCTION__)
> diff --git a/ext/http11/http11.c b/ext/http11/http11.c
> index 2982467..8e46e4c 100644
> --- a/ext/http11/http11.c
> +++ b/ext/http11/http11.c
> @@ -7,7 +7,6 @@
> #include <assert.h>
> #include <string.h>
> #include "http11_parser.h"
> -#include <ctype.h>
>
> static VALUE mMongrel;
> static VALUE cHttpParser;
> @@ -62,7 +61,8 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>
> void http_field(void *data, const char *field, size_t flen, const char
*value, size_t vlen)
> {
> -  char *ch, *end;
> +  char *ch;
> +  const char *fch;
>   VALUE req = (VALUE)data;
>   VALUE v = Qnil;
>   VALUE f = Qnil;
> @@ -71,15 +71,24 @@ void http_field(void *data, const char *field, size_t
flen, const char *value, s
>   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>
>   v = rb_str_new(value, vlen);
> -  f = rb_str_dup(global_http_prefix);
> -  f = rb_str_buf_cat(f, field, flen);
>
> -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch < end;
ch++) {
> -    if(*ch == ''-'') {
> -      *ch = ''_'';
> -    } else {
> -      *ch = toupper(*ch);
> -    }
> +  /*
> +   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
> +   * in my testing, because: there''s no minimum allocation length
(and
> +   * no check for it, either), RSTRING(f)->len does not need to be
> +   * written twice, and and RSTRING(f)->ptr[len] will already be
> +   * null-terminated for us.
> +   */
> +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
> +  memcpy(RSTRING(f)->ptr,
> +         RSTRING(global_http_prefix)->ptr,
> +         RSTRING(global_http_prefix)->len);
> +
> +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
> +  for(fch = field; flen-- != 0; ++fch) {
> +    *ch++ = (*fch >= ''a'' && *fch <=
''z'') ?
> +            ASCII_UPCASE_CHAR(*fch) :
> +            (*fch == ''-'' ? ''_'' :
*fch);
>   }
>
>   rb_hash_aset(req, f, v);
> -- 
> Eric Wong
> _______________________________________________
> Mongrel-development mailing list
> Mongrel-development at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-development
>

Filipe <filipe at icewall.org> wrote:
> 
> Hey Eric! as now you have commit permission, please apply the patches
> yourself to head. after testing it on head we can add this to branches,
> ok?
All three applied to trunk:

r989 | http11: ~6% performance increase in header parsing
r988 | mongrel: avoid needless syscall when num_processors limit is reached
r987 | mongrel_rails: support -n/--num-procs command-line option

I also noticed there was a general performance decrease in the trunk
HTTP parser versus stable-1-1 and 1-2.  I haven''t had time to dig
into this, yet.  r989 seems to bring it inline with the performance
of the unpatched stable-1-2 branch...
> On Sat, 1 Mar 2008, Eric Wong wrote:
> 
> > Eric Wong <normalperson at yhbt.net> wrote:
> >> ry dahl <ry at tinyclouds.org> wrote:
> >>> while i''m nitpicking...:P
> >>>
> >>> #define ASCII_UPPER(ch) (''a'' <= ch
&& ch <= ''z'' ? ch - ''a'' +
''A'' : ch)
> >>>
> >>>     memcpy( RSTRING_PTR(f)
> >>>           , RSTRING_PTR(global_http_prefix)
> >>>           , RSTRING_LEN(global_http_prefix)
> >>>           );
> >>>     for(i = 0; i < length; i++) {
> >>>       ch = RSTRING_PTR(f) + RSTRING_LEN(global_http_prefix) +
i;
> >>>       if(field[i] == ''-'') {
> >>>         *ch = ''_'';
> >>>       } else {
> >>>         *ch = ASCII_UPPER(field[i]);
> >>>       }
> >>>     }
> >>
> >> The ASCII_UPPER helps readability, thanks.
> >>
> >> I haven''t had a chance to benchmark your for loop vs my
while loop I
> >> wrote, but given the wide variety of compilers, versions and
options, I
> >> expect my while loop to be slightly faster in general.
> >
> > My version is about 0.3% faster :)
> >
> >> Additionally, reordering the if/else condition to put the more
common
> >> case first didn''t seem to help in my case (gcc 4.1.1-21
from Debian,
> >> -O2), so I left it alone.
> >>
> >> However, I haven''t tried __builtin_expect() here, either
(fast_xs did
> >> benefit from __builtin_expect() in my tests).
> >
> > __builtin_expect didn''t help, probably because the common
cases
> > (lowercase chars) for HTTP headers don''t overwhelmingly favor
the less
> > common cases (non-lowercase chars).
> >
> >> Perhaps rolling the checks together would be fastest, eliminating
> >> a redundant check for ''-'' if it''s
lowercase, still:
> >>
> >>   *ch = (''a'' <= *fch && *fch <=
''z'') ?
> >>         (*fch - ''a'' + ''A'') :
> >>         (*fch != ''-'' ? *fch :
''_'');
> >
> >> Not that other inefficiencies drastically overshadow the increases
> >> we get here :)
> >
> > Here''s my updated version, I''ve removed the extra
''i'' variable and made
> > the code a little more compact (and hopefully more readable) (with no
> > measurable performance impact from my original).
> >
> > I kept the bitwise operation for uppercasing instead of relying on
> > subtraction since it''s typically faster (no sign/overflow
checks)
> > although hard to measure in real-world performance here (my compiler
> > (gcc 4.1.1-21 -O2 on Debian/x86) preserved the subb instruction).
> >
> > diff --git a/ext/http11/ext_help.h b/ext/http11/ext_help.h
> > index 8b4d754..1017c64 100644
> > --- a/ext/http11/ext_help.h
> > +++ b/ext/http11/ext_help.h
> > @@ -4,6 +4,8 @@
> > #define RAISE_NOT_NULL(T) if(T == NULL) rb_raise(rb_eArgError,
"NULL found for " # T " when shouldn''t be.");
> > #define DATA_GET(from,type,name) Data_Get_Struct(from,type,name);
RAISE_NOT_NULL(name);
> > #define REQUIRE_TYPE(V, T) if(TYPE(V) != T) rb_raise(rb_eTypeError,
"Wrong argument type for " # V " required " # T);
> > +#define ASCII_UPCASE_CHAR(ch) (ch & ~0x20)
> > +
> >
> > #ifdef DEBUG
> > #define TRACE()  fprintf(stderr, "> %s:%d:%s\n",
__FILE__, __LINE__, __FUNCTION__)
> > diff --git a/ext/http11/http11.c b/ext/http11/http11.c
> > index 2982467..8e46e4c 100644
> > --- a/ext/http11/http11.c
> > +++ b/ext/http11/http11.c
> > @@ -7,7 +7,6 @@
> > #include <assert.h>
> > #include <string.h>
> > #include "http11_parser.h"
> > -#include <ctype.h>
> >
> > static VALUE mMongrel;
> > static VALUE cHttpParser;
> > @@ -62,7 +61,8 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
> >
> > void http_field(void *data, const char *field, size_t flen, const char
*value, size_t vlen)
> > {
> > -  char *ch, *end;
> > +  char *ch;
> > +  const char *fch;
> >   VALUE req = (VALUE)data;
> >   VALUE v = Qnil;
> >   VALUE f = Qnil;
> > @@ -71,15 +71,24 @@ void http_field(void *data, const char *field,
size_t flen, const char *value, s
> >   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
> >
> >   v = rb_str_new(value, vlen);
> > -  f = rb_str_dup(global_http_prefix);
> > -  f = rb_str_buf_cat(f, field, flen);
> >
> > -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch <
end; ch++) {
> > -    if(*ch == ''-'') {
> > -      *ch = ''_'';
> > -    } else {
> > -      *ch = toupper(*ch);
> > -    }
> > +  /*
> > +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
> > +   * in my testing, because: there''s no minimum allocation
length (and
> > +   * no check for it, either), RSTRING(f)->len does not need to be
> > +   * written twice, and and RSTRING(f)->ptr[len] will already be
> > +   * null-terminated for us.
> > +   */
> > +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
> > +  memcpy(RSTRING(f)->ptr,
> > +         RSTRING(global_http_prefix)->ptr,
> > +         RSTRING(global_http_prefix)->len);
> > +
> > +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
> > +  for(fch = field; flen-- != 0; ++fch) {
> > +    *ch++ = (*fch >= ''a'' && *fch <=
''z'') ?
> > +            ASCII_UPCASE_CHAR(*fch) :
> > +            (*fch == ''-'' ? ''_'' :
*fch);
> >   }
> >
> >   rb_hash_aset(req, f, v);
> > -- 
> > Eric Wong

Trunk has the Ruby 1.9 compatibility changes, which might have
something to do with it. Those need to get backported to stable_1-2 at
some point. Trunk also has a new logger that Wayne wrote. That could
be degrading the parser performance somehow.

Evan

On Sat, Mar 1, 2008 at 9:39 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Filipe <filipe at icewall.org> wrote:
>  >
>
> > Hey Eric! as now you have commit permission, please apply the patches
>  > yourself to head. after testing it on head we can add this to
branches,
>  > ok?
>
>  All three applied to trunk:
>
>  r989 | http11: ~6% performance increase in header parsing
>  r988 | mongrel: avoid needless syscall when num_processors limit is
reached
>  r987 | mongrel_rails: support -n/--num-procs command-line option
>
>  I also noticed there was a general performance decrease in the trunk
>  HTTP parser versus stable-1-1 and 1-2.  I haven''t had time to dig
>  into this, yet.  r989 seems to bring it inline with the performance
>  of the unpatched stable-1-2 branch...
>
>
>
>  > On Sat, 1 Mar 2008, Eric Wong wrote:
>  >
>  > > Eric Wong <normalperson at yhbt.net> wrote:
>  > >> ry dahl <ry at tinyclouds.org> wrote:
>  > >>> while i''m nitpicking...:P
>  > >>>
>  > >>> #define ASCII_UPPER(ch) (''a'' <= ch
&& ch <= ''z'' ? ch - ''a'' +
''A'' : ch)
>  > >>>
>  > >>>     memcpy( RSTRING_PTR(f)
>  > >>>           , RSTRING_PTR(global_http_prefix)
>  > >>>           , RSTRING_LEN(global_http_prefix)
>  > >>>           );
>  > >>>     for(i = 0; i < length; i++) {
>  > >>>       ch = RSTRING_PTR(f) +
RSTRING_LEN(global_http_prefix) + i;
>  > >>>       if(field[i] == ''-'') {
>  > >>>         *ch = ''_'';
>  > >>>       } else {
>  > >>>         *ch = ASCII_UPPER(field[i]);
>  > >>>       }
>  > >>>     }
>  > >>
>  > >> The ASCII_UPPER helps readability, thanks.
>  > >>
>  > >> I haven''t had a chance to benchmark your for loop
vs my while loop I
>  > >> wrote, but given the wide variety of compilers, versions and
options, I
>  > >> expect my while loop to be slightly faster in general.
>  > >
>  > > My version is about 0.3% faster :)
>  > >
>  > >> Additionally, reordering the if/else condition to put the
more common
>  > >> case first didn''t seem to help in my case (gcc
4.1.1-21 from Debian,
>  > >> -O2), so I left it alone.
>  > >>
>  > >> However, I haven''t tried __builtin_expect() here,
either (fast_xs did
>  > >> benefit from __builtin_expect() in my tests).
>  > >
>  > > __builtin_expect didn''t help, probably because the
common cases
>  > > (lowercase chars) for HTTP headers don''t overwhelmingly
favor the less
>  > > common cases (non-lowercase chars).
>  > >
>  > >> Perhaps rolling the checks together would be fastest,
eliminating
>  > >> a redundant check for ''-'' if it''s
lowercase, still:
>  > >>
>  > >>   *ch = (''a'' <= *fch && *fch
<= ''z'') ?
>  > >>         (*fch - ''a'' +
''A'') :
>  > >>         (*fch != ''-'' ? *fch :
''_'');
>  > >
>  > >> Not that other inefficiencies drastically overshadow the
increases
>  > >> we get here :)
>  > >
>  > > Here''s my updated version, I''ve removed the
extra ''i'' variable and made
>  > > the code a little more compact (and hopefully more readable)
(with no
>  > > measurable performance impact from my original).
>  > >
>  > > I kept the bitwise operation for uppercasing instead of relying
on
>  > > subtraction since it''s typically faster (no
sign/overflow checks)
>  > > although hard to measure in real-world performance here (my
compiler
>  > > (gcc 4.1.1-21 -O2 on Debian/x86) preserved the subb
instruction).
>  > >
>  > > diff --git a/ext/http11/ext_help.h b/ext/http11/ext_help.h
>  > > index 8b4d754..1017c64 100644
>  > > --- a/ext/http11/ext_help.h
>  > > +++ b/ext/http11/ext_help.h
>  > > @@ -4,6 +4,8 @@
>  > > #define RAISE_NOT_NULL(T) if(T == NULL) rb_raise(rb_eArgError,
"NULL found for " # T " when shouldn''t be.");
>  > > #define DATA_GET(from,type,name)
Data_Get_Struct(from,type,name); RAISE_NOT_NULL(name);
>  > > #define REQUIRE_TYPE(V, T) if(TYPE(V) != T)
rb_raise(rb_eTypeError, "Wrong argument type for " # V " required
" # T);
>  > > +#define ASCII_UPCASE_CHAR(ch) (ch & ~0x20)
>  > > +
>  > >
>  > > #ifdef DEBUG
>  > > #define TRACE()  fprintf(stderr, "> %s:%d:%s\n",
__FILE__, __LINE__, __FUNCTION__)
>  > > diff --git a/ext/http11/http11.c b/ext/http11/http11.c
>  > > index 2982467..8e46e4c 100644
>  > > --- a/ext/http11/http11.c
>  > > +++ b/ext/http11/http11.c
>  > > @@ -7,7 +7,6 @@
>  > > #include <assert.h>
>  > > #include <string.h>
>  > > #include "http11_parser.h"
>  > > -#include <ctype.h>
>  > >
>  > > static VALUE mMongrel;
>  > > static VALUE cHttpParser;
>  > > @@ -62,7 +61,8 @@ DEF_MAX_LENGTH(HEADER, (1024 * (80 + 32)));
>  > >
>  > > void http_field(void *data, const char *field, size_t flen,
const char *value, size_t vlen)
>  > > {
>  > > -  char *ch, *end;
>  > > +  char *ch;
>  > > +  const char *fch;
>  > >   VALUE req = (VALUE)data;
>  > >   VALUE v = Qnil;
>  > >   VALUE f = Qnil;
>  > > @@ -71,15 +71,24 @@ void http_field(void *data, const char
*field, size_t flen, const char *value, s
>  > >   VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>  > >
>  > >   v = rb_str_new(value, vlen);
>  > > -  f = rb_str_dup(global_http_prefix);
>  > > -  f = rb_str_buf_cat(f, field, flen);
>  > >
>  > > -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len;
ch < end; ch++) {
>  > > -    if(*ch == ''-'') {
>  > > -      *ch = ''_'';
>  > > -    } else {
>  > > -      *ch = toupper(*ch);
>  > > -    }
>  > > +  /*
>  > > +   * using rb_str_new(NULL, len) here is faster than
rb_str_buf_new(len)
>  > > +   * in my testing, because: there''s no minimum
allocation length (and
>  > > +   * no check for it, either), RSTRING(f)->len does not need
to be
>  > > +   * written twice, and and RSTRING(f)->ptr[len] will
already be
>  > > +   * null-terminated for us.
>  > > +   */
>  > > +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len +
flen);
>  > > +  memcpy(RSTRING(f)->ptr,
>  > > +         RSTRING(global_http_prefix)->ptr,
>  > > +         RSTRING(global_http_prefix)->len);
>  > > +
>  > > +  ch = RSTRING(f)->ptr +
RSTRING(global_http_prefix)->len;
>  > > +  for(fch = field; flen-- != 0; ++fch) {
>  > > +    *ch++ = (*fch >= ''a'' && *fch
<= ''z'') ?
>  > > +            ASCII_UPCASE_CHAR(*fch) :
>  > > +            (*fch == ''-'' ?
''_'' : *fch);
>  > >   }
>  > >
>  > >   rb_hash_aset(req, f, v);
>  > > --
>  > > Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

"Zed A. Shaw" <zedshaw at zedshaw.com>
wrote:
> On Thu, 28 Feb 2008 17:53:09 -0800
> Eric Wong <normalperson at yhbt.net> wrote:
> 
> > Allocate one string object and avoid appending to it causing it
> > to be resized.  Additionally, optimize the string toupper copy
> > so that it''s done in a single pass.
> 
> Not bad, but I have some code style comments on your C code:
> 
> * I think the RSTRING macro is going away in the new ruby.  Ran into
> that recently.
Yup.  The version I committed to trunk uses the 1.9-compatible
RSTRING_{PTR,LEN} macros
> * Your inner loop isn''t that readable and too clever.  The
original
> for loop is clear, and should have the same or more speed when
> handed to a modern compiler.  Since you either already know the length
> of the target string, or it''s a ruby string that ends in \0, just
use a
> regular pointer loop to go through the string.  No point starting at
> the back and decrementing a counter that also increment other pointers.
> Additionally, the compiler should make it faster with pointers directly
> instead and there''s CPU operations that do this natively anyway,
but
> nothing using the non-standard backward-increment-forward-dual-pointers
> thing you have.
I actually shortened the one committed to trunk a bit (but used nested
ternary operators, so I may rewrite that for better readability).

It modifies flen directly instead of copying it to a temporary variable.
I don''t think Ragel actually terminates field with
''\0'', but I''ll double
check later.
> * After your memcpy you don''t clamp the string with a \0 since
you''re
> relying on the backwards loop to do it.  This is kind of too clever to
> be useful, so I''d do it the regular way and memcpy then set
''\0''.
> That''s also safer.
rb_str_new() actually does it, the loop shouldn''t overwrite it.  But I
understand if you want me to clamp it again manually for easier
auditing.
> * You
> should also double check the code in ruby to make sure that
> any other RString internals don''t need to be updated.  I believe
> there''s a few other things that have to be touched to keep ruby
sane
> for some operations.  Look at how the ruby dup operations are done.
> * You should get some real HTTP test samples, the worse the better.
> All you''ve done is shown that it''s marginally faster for
a single run
> of a simple set of headers.  I''d validate other combinations
before
> putting in code this complex for just a small boost like that.
> * Also, now that I think about it, if you don''t care that the
original
> string is modified in place then you can just have ragel do all of this
> as it goes.  Simply modify the parser to have it do this transform on
> the header chars using the existing pointer.  That''d probably be
> alright since people don''t usually keep the input headers around
when
> using the mongrel parser.
I''ll look into all of those three.  I think I saw rb_str_modify() but
either forgot to use it or figured I didn''t need it :X
> Give those suggestions a try and see what you get.  Thanks for helping
> out Eric.
Thanks for the feedback, Zed.

There''s also something else that occured to me is why we always create
a
new RString object in the first place if it''s getting set into a Hash
(and frozen).  I suspect we''ll get a much bigger performance increase
by
avoiding redundant object creation when we get common header names and
directly using global objects to represent them (like global_http_host).


Additionally, my understanding is that frozen objects never get GC-ed,
and since strings get frozen when being put into a hash, this could
be a memory leak if somebody unleashes a random header generator.

Maybe we should keep a global store of predefined HTTP headers we always
accept (all those mentioned in HTTP RFCs), and leave 100 or so slots
open for some random headers clients may put in;
letting those weird clients fill those slots on a first-come basis.

Afterwards, any new headers we haven''t seen and don''t know
about would
either raise an exception or get silently dropped ...

This would prevent memory leaks if my understanding of frozen objects
is correct...


Way past my bedtime to test these things now, though.

-- 
Eric Wong

> This would prevent memory leaks if my understanding of frozen objects
> is correct...
I don''t think the GC treats frozen objects in any special way.

Evan

On Sun, Mar 2, 2008 at 7:37 AM, Eric Wong <normalperson at yhbt.net>
wrote:
> "Zed A. Shaw" <zedshaw at zedshaw.com> wrote:
>  > On Thu, 28 Feb 2008 17:53:09 -0800
>  > Eric Wong <normalperson at yhbt.net> wrote:
>  >
>  > > Allocate one string object and avoid appending to it causing it
>  > > to be resized.  Additionally, optimize the string toupper copy
>  > > so that it''s done in a single pass.
>  >
>  > Not bad, but I have some code style comments on your C code:
>  >
>  > * I think the RSTRING macro is going away in the new ruby.  Ran into
>  > that recently.
>
>  Yup.  The version I committed to trunk uses the 1.9-compatible
>  RSTRING_{PTR,LEN} macros
>
>
>  > * Your inner loop isn''t that readable and too clever.  The
original
>  > for loop is clear, and should have the same or more speed when
>  > handed to a modern compiler.  Since you either already know the
length
>  > of the target string, or it''s a ruby string that ends in \0,
just use a
>  > regular pointer loop to go through the string.  No point starting at
>  > the back and decrementing a counter that also increment other
pointers.
>  > Additionally, the compiler should make it faster with pointers
directly
>  > instead and there''s CPU operations that do this natively
anyway, but
>  > nothing using the non-standard
backward-increment-forward-dual-pointers
>  > thing you have.
>
>  I actually shortened the one committed to trunk a bit (but used nested
>  ternary operators, so I may rewrite that for better readability).
>
>  It modifies flen directly instead of copying it to a temporary variable.
>  I don''t think Ragel actually terminates field with
''\0'', but I''ll double
>  check later.
>
>
>  > * After your memcpy you don''t clamp the string with a \0
since you''re
>  > relying on the backwards loop to do it.  This is kind of too clever
to
>  > be useful, so I''d do it the regular way and memcpy then set
''\0''.
>  > That''s also safer.
>
>  rb_str_new() actually does it, the loop shouldn''t overwrite it. 
But I
>  understand if you want me to clamp it again manually for easier
>  auditing.
>
>
>  > * You
>  > should also double check the code in ruby to make sure that
>  > any other RString internals don''t need to be updated.  I
believe
>  > there''s a few other things that have to be touched to keep
ruby sane
>  > for some operations.  Look at how the ruby dup operations are done.
>  > * You should get some real HTTP test samples, the worse the better.
>  > All you''ve done is shown that it''s marginally
faster for a single run
>  > of a simple set of headers.  I''d validate other combinations
before
>  > putting in code this complex for just a small boost like that.
>  > * Also, now that I think about it, if you don''t care that
the original
>  > string is modified in place then you can just have ragel do all of
this
>  > as it goes.  Simply modify the parser to have it do this transform on
>  > the header chars using the existing pointer.  That''d
probably be
>  > alright since people don''t usually keep the input headers
around when
>  > using the mongrel parser.
>
>  I''ll look into all of those three.  I think I saw rb_str_modify()
but
>  either forgot to use it or figured I didn''t need it :X
>
>
>  > Give those suggestions a try and see what you get.  Thanks for
helping
>  > out Eric.
>
>  Thanks for the feedback, Zed.
>
>  There''s also something else that occured to me is why we always
create a
>  new RString object in the first place if it''s getting set into a
Hash
>  (and frozen).  I suspect we''ll get a much bigger performance
increase by
>  avoiding redundant object creation when we get common header names and
>  directly using global objects to represent them (like global_http_host).
>
>
>  Additionally, my understanding is that frozen objects never get GC-ed,
>  and since strings get frozen when being put into a hash, this could
>  be a memory leak if somebody unleashes a random header generator.
>
>  Maybe we should keep a global store of predefined HTTP headers we always
>  accept (all those mentioned in HTTP RFCs), and leave 100 or so slots
>  open for some random headers clients may put in;
>  letting those weird clients fill those slots on a first-come basis.
>
>  Afterwards, any new headers we haven''t seen and don''t
know about would
>  either raise an exception or get silently dropped ...
>
>  This would prevent memory leaks if my understanding of frozen objects
>  is correct...
>
>
>  Way past my bedtime to test these things now, though.
>
>
>
>  --
>  Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Evan Weaver <evan at cloudbur.st> wrote:
> > This would prevent memory leaks if my understanding of frozen objects
> > is correct...
> 
> I don''t think the GC treats frozen objects in any special way.
Oops, it was symbols that leaked, not frozen objects in general.  Phew.

-- 
Eric Wong

On Thu, 28 Feb 2008 17:53:09 -0800
Eric Wong <normalperson at yhbt.net> wrote:
> Allocate one string object and avoid appending to it causing it
> to be resized.  Additionally, optimize the string toupper copy
> so that it''s done in a single pass.
Not bad, but I have some code style comments on your C code:

* I think the RSTRING macro is going away in the new ruby.  Ran into
that recently.
* Your inner loop isn''t that readable and too clever.  The original
for loop is clear, and should have the same or more speed when
handed to a modern compiler.  Since you either already know the length
of the target string, or it''s a ruby string that ends in \0, just use a
regular pointer loop to go through the string.  No point starting at
the back and decrementing a counter that also increment other pointers.
Additionally, the compiler should make it faster with pointers directly
instead and there''s CPU operations that do this natively anyway, but
nothing using the non-standard backward-increment-forward-dual-pointers
thing you have.
* After your memcpy you don''t clamp the string with a \0 since
you''re
relying on the backwards loop to do it.  This is kind of too clever to
be useful, so I''d do it the regular way and memcpy then set
''\0''.
That''s also safer.
* You
should also double check the code in ruby to make sure that
any other RString internals don''t need to be updated.  I believe
there''s a few other things that have to be touched to keep ruby sane
for some operations.  Look at how the ruby dup operations are done.
* You should get some real HTTP test samples, the worse the better.
All you''ve done is shown that it''s marginally faster for a
single run
of a simple set of headers.  I''d validate other combinations before
putting in code this complex for just a small boost like that.
* Also, now that I think about it, if you don''t care that the original
string is modified in place then you can just have ragel do all of this
as it goes.  Simply modify the parser to have it do this transform on
the header chars using the existing pointer.  That''d probably be
alright since people don''t usually keep the input headers around when
using the mongrel parser.

Give those suggestions a try and see what you get.  Thanks for helping
out Eric.

Zed
>  void http_field(void *data, const char *field, size_t flen, const char
*value, size_t vlen)
>  {
> -  char *ch, *end;
> +  char *ch;
> +  const char *fch;
> +  int i;
>    VALUE req = (VALUE)data;
>    VALUE v = Qnil;
>    VALUE f = Qnil;
> @@ -71,15 +72,30 @@ void http_field(void *data, const char *field, size_t
flen, const char *value, s
>    VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);
>  
>    v = rb_str_new(value, vlen);
> -  f = rb_str_dup(global_http_prefix);
> -  f = rb_str_buf_cat(f, field, flen); 
>  
> -  for(ch = RSTRING(f)->ptr, end = ch + RSTRING(f)->len; ch < end;
ch++) {
> -    if(*ch == ''-'') {
> +  /*
> +   * using rb_str_new(NULL, len) here is faster than rb_str_buf_new(len)
> +   * in my testing, because: there''s no minimum allocation length
(and
> +   * no check for it, either), RSTRING(f)->len does not need to be
> +   * written twice, and and RSTRING(f)->ptr[len] will already be
> +   * null-terminated for us.
> +   */
> +  f = rb_str_new(NULL, RSTRING(global_http_prefix)->len + flen);
> +  memcpy(RSTRING(f)->ptr,
> +         RSTRING(global_http_prefix)->ptr,
> +         RSTRING(global_http_prefix)->len);
> +
> +  i = (int)flen;
> +  fch = field;
> +  ch = RSTRING(f)->ptr + RSTRING(global_http_prefix)->len;
> +  while(--i >= 0) {
> +    if(*fch == ''-'') {
>        *ch = ''_'';
>      } else {
> -      *ch = toupper(*ch);
> +      *ch = (*fch >= ''a'' && *fch <=
''z'') ? (*fch & ~0x20) : *fch;
>      }
> +    ++ch;
> +    ++fch;
>    }
>  
>    rb_hash_aset(req, f, v);
> -- 
> Eric Wong
> _______________________________________________
> Mongrel-development mailing list
> Mongrel-development at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-development

-- 
Zed A. Shaw
- Hate: http://savingtheinternetwithhate.com/
- Good: http://www.zedshaw.com/
- Evil: http://yearofevil.com/

Eric Wong <normalperson at yhbt.net> wrote:
> "Zed A. Shaw" <zedshaw at zedshaw.com> wrote:
> > On Thu, 28 Feb 2008 17:53:09 -0800
> > Eric Wong <normalperson at yhbt.net> wrote:
> > * You
> > should also double check the code in ruby to make sure that
> > any other RString internals don''t need to be updated.  I
believe
> > there''s a few other things that have to be touched to keep
ruby sane
> > for some operations.  Look at how the ruby dup operations are done.
rb_str_modify() only needs to be called for duplicated strings.
For new strings, it''s safe to write directly to the pointer after
rb_str_new(0, len) is called (the 1.8.6.111 source has several
examples of this).
> > * You should get some real HTTP test samples, the worse the better.
> > All you''ve done is shown that it''s marginally faster
for a single run
> > of a simple set of headers.  I''d validate other combinations
before
> > putting in code this complex for just a small boost like that.
I still need to get to that Friday or this weekend and test with Rfuzz,
too.  I''ve added some more unit tests to ensure my code works for now.

r992 is a fairly big change (with pretty good benefits, see below), but
I''ve coded that in as fool-proof a way as I possibly could.
> > * Also, now that I think about it, if you don''t care that the
original
> > string is modified in place then you can just have ragel do all of
this
> > as it goes.  Simply modify the parser to have it do this transform on
> > the header chars using the existing pointer.  That''d probably
be
> > alright since people don''t usually keep the input headers
around when
> > using the mongrel parser.
done in r990
> I''ll look into all of those three.  I think I saw rb_str_modify()
but
> either forgot to use it or figured I didn''t need it :X
> 
> > Give those suggestions a try and see what you get.  Thanks for helping
> > out Eric.
> 
> Thanks for the feedback, Zed.
> 
> There''s also something else that occured to me is why we always
create a
> new RString object in the first place if it''s getting set into a
Hash
> (and frozen).  I suspect we''ll get a much bigger performance
increase by
> avoiding redundant object creation when we get common header names and
> directly using global objects to represent them (like global_http_host).
I''m getting an even bigger (~22%) performance improvement by
predefining
some common HTTP headers as global frozen strings upfront (r992)
> Additionally, my understanding is that frozen objects never get GC-ed,
> and since strings get frozen when being put into a hash, this could
> be a memory leak if somebody unleashes a random header generator.
Thankfully, I was wrong about frozen strings :)

-- 
Eric Wong

Hi Eric,
>  I''m getting an even bigger (~22%) performance
> improvement by predefining
>  some common HTTP headers as global frozen
> strings upfront (r992)
Why don''t you do this in Ragel? It will be faster and you
don''t need
to depend on bsearch. I pull out content-length header in ragel in
ebb:
 http://github.com/ry/ebb/tree/master/src/parser.rl
(although there is a bug with this because (content_length |
message_header ) needs some priorities set so message_header isn''t
matching content-length too. should be
   (content_length >2 | message_header >1 )
or something).

Also - the state machine should be upgraded to compile with ragel 6.
this basically involves removing %write eof;

ry

ry dahl <ry at tinyclouds.org> wrote:
> Hi Eric,
> 
> >  I''m getting an even bigger (~22%) performance
> > improvement by predefining
> >  some common HTTP headers as global frozen
> > strings upfront (r992)
> 
> Why don''t you do this in Ragel? It will be faster and you
don''t need
> to depend on bsearch. I pull out content-length header in ragel in
> ebb:
>  http://github.com/ry/ebb/tree/master/src/parser.rl
> (although there is a bug with this because (content_length |
> message_header ) needs some priorities set so message_header isn''t
> matching content-length too. should be
>    (content_length >2 | message_header >1 )
> or something).
Then I''d have to define a new C function for every header I wanted to
optimize for, and then also point to that function inside the Ragel file
for each header.

Unless we use something like ERB to generate this code for both Ragel
and C, I''m not sure it''s worth the effort to go through with
all the
extra code.

Currently, with my C/CPP code, I can add or remove headers to memoize
strings for by adding or removing one line per header in the C file.
Pretty much as easy as it gets maintenance-wise.
> Also - the state machine should be upgraded to compile with ragel 6.
> this basically involves removing %write eof;
Yes.  At the same time, I''m not sure if I should force every other
developer to upgrade... Evan?  Zed?

-- 
Eric Wong

Upgrading ragel is fine with me.

Evan

On Sat, Mar 8, 2008 at 3:12 AM, Eric Wong <normalperson at yhbt.net>
wrote:
> ry dahl <ry at tinyclouds.org> wrote:
>
> > Hi Eric,
>  >
>  > >  I''m getting an even bigger (~22%) performance
>  > > improvement by predefining
>  > >  some common HTTP headers as global frozen
>  > > strings upfront (r992)
>  >
>  > Why don''t you do this in Ragel? It will be faster and you
don''t need
>  > to depend on bsearch. I pull out content-length header in ragel in
>  > ebb:
>  >  http://github.com/ry/ebb/tree/master/src/parser.rl
>  > (although there is a bug with this because (content_length |
>  > message_header ) needs some priorities set so message_header
isn''t
>  > matching content-length too. should be
>  >    (content_length >2 | message_header >1 )
>  > or something).
>
>  Then I''d have to define a new C function for every header I
wanted to
>  optimize for, and then also point to that function inside the Ragel file
>  for each header.
>
>  Unless we use something like ERB to generate this code for both Ragel
>  and C, I''m not sure it''s worth the effort to go through
with all the
>  extra code.
>
>  Currently, with my C/CPP code, I can add or remove headers to memoize
>  strings for by adding or removing one line per header in the C file.
>  Pretty much as easy as it gets maintenance-wise.
>
>
>  > Also - the state machine should be upgraded to compile with ragel 6.
>  > this basically involves removing %write eof;
>
>  Yes.  At the same time, I''m not sure if I should force every
other
>  developer to upgrade... Evan?  Zed?
>
>  --
>  Eric Wong
>
>
> _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

On Sat, Mar 8, 2008 at 6:12 AM, Eric Wong <normalperson at yhbt.net>
wrote:
>
>  Unless we use something like ERB to generate this code for both Ragel
>  and C, I''m not sure it''s worth the effort to go through
with all the
>  extra code.
>
I think is not worth the effort, and also add complexity to the code
generation, which should be the simpler as possible.
>  Yes.  At the same time, I''m not sure if I should force every
other
>  developer to upgrade... Evan?  Zed?
Well, the Ragel compilation is not run every time, but only when the
parser get updated, so I can say will be fine the most common cases
(developers playing with mongrel code already handle the ragel
compiled code).

-- 
Luis Lavena
Multimedia systems
-
A common mistake that people make when trying to design
something completely foolproof is to underestimate
the ingenuity of complete fools.
Douglas Adams

>  Then I''d have to define a new C function for every header I
wanted to
>  optimize for, and then also point to that function inside the Ragel file
>  for each header.
Nah, you can just define

enum header_types {
 MONGREL_CONTENT_LENGTH,
 MONGREL_CONTENT_TYPE,
 etc
}

and define a single callback to handle them

void (*field_cb)(http_parser*, int header_type, char *at, int len)

ry

Evan Weaver <evan at cloudbur.st> wrote:
> Upgrading ragel is fine with me.
Here''s my work-in-progress.  The changes below are pretty much copied
from ry''s changes in ebb.

I haven''t been able to figure out why
test_horrible_queries(HttpParserTest) is failing, so any help here would
be appreciated.

Thanks.

diff --git a/Rakefile b/Rakefile
index f47d7a2..07a89c2 100644
--- a/Rakefile
+++ b/Rakefile
@@ -55,13 +55,15 @@ task :ragel do
   Dir.chdir "ext/http11" do
     target = "http11_parser.c"
     File.unlink target if File.exist? target
-    sh "ragel http11_parser.rl | rlgen-cd -G2 -o #{target}"
+    # sh "ragel http11_parser.rl | rlgen-cd -G2 -o #{target}" # ragel
5.x
+    sh "ragel -G2 http11_parser.rl" # ragel 6.0
     raise "Failed to build C source" unless File.exist? target
   end
   Dir.chdir "ext/http11" do
     target =
"../../ext/http11_java/org/jruby/mongrel/Http11Parser.java"
     File.unlink target if File.exist? target
-    sh "ragel -J http11_parser.java.rl | rlgen-java -o #{target}"
+    # sh "ragel -J http11_parser.java.rl | rlgen-java -o #{target}" #
ragel 5.x
+    sh "ragel -J -o #{target} http11_parser.java.rl" # ragel 6.0
     raise "Failed to build Java source" unless File.exist? target
   end
 end
diff --git a/ext/http11/http11_parser.rl b/ext/http11/http11_parser.rl
index a418605..0c4e2d4 100644
--- a/ext/http11/http11_parser.rl
+++ b/ext/http11/http11_parser.rl
@@ -114,7 +114,7 @@ size_t http_parser_execute(http_parser *parser, const char
*buffer, size_t len,
   p = buffer+off;
   pe = buffer+len;
 
-  assert(*pe == ''\0'' && "pointer does not end on
NUL");
+  /* assert(*pe == ''\0'' && "pointer does not end
on NUL"); */
   assert(pe - p == len - off && "pointers aren''t same
distance");
 
 
@@ -130,23 +130,11 @@ size_t http_parser_execute(http_parser *parser, const char
*buffer, size_t len,
   assert(parser->field_len <= len && "field has length
longer than whole buffer");
   assert(parser->field_start < len && "field starts after
buffer end");
 
-  if(parser->body_start) {
-    /* final \r\n combo encountered so stop right here */
-    %%write eof;
-    parser->nread++;
-  }
-
   return(parser->nread);
 }
 
 int http_parser_finish(http_parser *parser)
 {
-  int cs = parser->cs;
-
-  %%write eof;
-
-  parser->cs = cs;
-
   if (http_parser_has_error(parser) ) {
     return -1;
   } else if (http_parser_is_finished(parser) ) {
@@ -161,5 +149,5 @@ int http_parser_has_error(http_parser *parser) {
 }
 
 int http_parser_is_finished(http_parser *parser) {
-  return parser->cs == http_parser_first_final;
+  return parser->cs >= http_parser_first_final;
 }
-- 
Eric Wong

Hi all,

I backported the trunk parser changes and applied the Ragel fix to
branches/stable_1-2 . Two tests are currently failing... could someone
(Eric, Ry?) take a look at these? I am not good enough with Ragel to
be able to say whether or not the eof change is related to the test
failures.

Thanks,

Evan

On Sat, Mar 8, 2008 at 6:02 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Evan Weaver <evan at cloudbur.st> wrote:
>
> > Upgrading ragel is fine with me.
>
>  Here''s my work-in-progress.  The changes below are pretty much
copied
>  from ry''s changes in ebb.
>
>  I haven''t been able to figure out why
>  test_horrible_queries(HttpParserTest) is failing, so any help here would
>  be appreciated.
>
>  Thanks.
>
>  diff --git a/Rakefile b/Rakefile
>  index f47d7a2..07a89c2 100644
>  --- a/Rakefile
>  +++ b/Rakefile
>  @@ -55,13 +55,15 @@ task :ragel do
>    Dir.chdir "ext/http11" do
>      target = "http11_parser.c"
>      File.unlink target if File.exist? target
>  -    sh "ragel http11_parser.rl | rlgen-cd -G2 -o #{target}"
>  +    # sh "ragel http11_parser.rl | rlgen-cd -G2 -o #{target}" #
ragel 5.x
>  +    sh "ragel -G2 http11_parser.rl" # ragel 6.0
>      raise "Failed to build C source" unless File.exist? target
>    end
>    Dir.chdir "ext/http11" do
>      target =
"../../ext/http11_java/org/jruby/mongrel/Http11Parser.java"
>      File.unlink target if File.exist? target
>  -    sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}"
>  +    # sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}" # ragel 5.x
>  +    sh "ragel -J -o #{target} http11_parser.java.rl" # ragel
6.0
>      raise "Failed to build Java source" unless File.exist?
target
>    end
>   end
>  diff --git a/ext/http11/http11_parser.rl b/ext/http11/http11_parser.rl
>  index a418605..0c4e2d4 100644
>  --- a/ext/http11/http11_parser.rl
>  +++ b/ext/http11/http11_parser.rl
>  @@ -114,7 +114,7 @@ size_t http_parser_execute(http_parser *parser, const
char *buffer, size_t len,
>    p = buffer+off;
>    pe = buffer+len;
>
>  -  assert(*pe == ''\0'' && "pointer does not
end on NUL");
>  +  /* assert(*pe == ''\0'' && "pointer does
not end on NUL"); */
>    assert(pe - p == len - off && "pointers aren''t
same distance");
>
>
>  @@ -130,23 +130,11 @@ size_t http_parser_execute(http_parser *parser,
const char *buffer, size_t len,
>    assert(parser->field_len <= len && "field has length
longer than whole buffer");
>    assert(parser->field_start < len && "field starts
after buffer end");
>
>  -  if(parser->body_start) {
>  -    /* final \r\n combo encountered so stop right here */
>  -    %%write eof;
>  -    parser->nread++;
>  -  }
>  -
>    return(parser->nread);
>   }
>
>   int http_parser_finish(http_parser *parser)
>   {
>  -  int cs = parser->cs;
>  -
>  -  %%write eof;
>  -
>  -  parser->cs = cs;
>  -
>    if (http_parser_has_error(parser) ) {
>      return -1;
>    } else if (http_parser_is_finished(parser) ) {
>  @@ -161,5 +149,5 @@ int http_parser_has_error(http_parser *parser) {
>   }
>
>   int http_parser_is_finished(http_parser *parser) {
>  -  return parser->cs == http_parser_first_final;
>  +  return parser->cs >= http_parser_first_final;
>   }
>  --
>
>
> Eric Wong
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

Evan Weaver <evan at cloudbur.st> wrote:
> Hi all,
> 
> I backported the trunk parser changes and applied the Ragel fix to
> branches/stable_1-2 . Two tests are currently failing... could someone
> (Eric, Ry?) take a look at these? I am not good enough with Ragel to
> be able to say whether or not the eof change is related to the test
> failures.
Hi Evan,

I never committed the Ragel 6 upgrade to trunk because I couldn''t
figure
out why it was broken.  I was hoping someone else would step in and fix
it for us :)

Reverting my attempt at upgrading to Ragel 6 fixes one test...



As for the other test that fails, you uncommented the nasty_pound_header
test in the commit for multi-line.  This test doesn''t work in
stable-1_1 nor trunk, and I''ve never made any effort into getting it
working (nor was it my fault it''s broken in the first place :)

Digging into the revision history (with git :), the nasty_pound_header
test was committed in its commented form in r361 and has never changed
until now.

The Mongrel HTTP parser code does not appear to handle multi-line
HTTP headers at all, so I''m not surprised that test fails.  I would
suggest disabling the nasty_pound_header test again until we can
successfully parse multi-line headers.
> Thanks,
> 
> Evan
> 
> On Sat, Mar 8, 2008 at 6:02 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> > Evan Weaver <evan at cloudbur.st> wrote:
> >
> > > Upgrading ragel is fine with me.
> >
> >  Here''s my work-in-progress.  The changes below are pretty
much copied
> >  from ry''s changes in ebb.
> >
> >  I haven''t been able to figure out why
> >  test_horrible_queries(HttpParserTest) is failing, so any help here
would
> >  be appreciated.
> >
> >  Thanks.
> >
> >  diff --git a/Rakefile b/Rakefile
> >  index f47d7a2..07a89c2 100644
> >  --- a/Rakefile
> >  +++ b/Rakefile
> >  @@ -55,13 +55,15 @@ task :ragel do
> >    Dir.chdir "ext/http11" do
> >      target = "http11_parser.c"
> >      File.unlink target if File.exist? target
> >  -    sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}"
> >  +    # sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}" # ragel 5.x
> >  +    sh "ragel -G2 http11_parser.rl" # ragel 6.0
> >      raise "Failed to build C source" unless File.exist?
target
> >    end
> >    Dir.chdir "ext/http11" do
> >      target =
"../../ext/http11_java/org/jruby/mongrel/Http11Parser.java"
> >      File.unlink target if File.exist? target
> >  -    sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}"
> >  +    # sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}" # ragel 5.x
> >  +    sh "ragel -J -o #{target} http11_parser.java.rl" #
ragel 6.0
> >      raise "Failed to build Java source" unless File.exist?
target
> >    end
> >   end
> >  diff --git a/ext/http11/http11_parser.rl
b/ext/http11/http11_parser.rl
> >  index a418605..0c4e2d4 100644
> >  --- a/ext/http11/http11_parser.rl
> >  +++ b/ext/http11/http11_parser.rl
> >  @@ -114,7 +114,7 @@ size_t http_parser_execute(http_parser *parser,
const char *buffer, size_t len,
> >    p = buffer+off;
> >    pe = buffer+len;
> >
> >  -  assert(*pe == ''\0'' && "pointer does
not end on NUL");
> >  +  /* assert(*pe == ''\0'' && "pointer
does not end on NUL"); */
> >    assert(pe - p == len - off && "pointers
aren''t same distance");
> >
> >
> >  @@ -130,23 +130,11 @@ size_t http_parser_execute(http_parser *parser,
const char *buffer, size_t len,
> >    assert(parser->field_len <= len && "field has
length longer than whole buffer");
> >    assert(parser->field_start < len && "field
starts after buffer end");
> >
> >  -  if(parser->body_start) {
> >  -    /* final \r\n combo encountered so stop right here */
> >  -    %%write eof;
> >  -    parser->nread++;
> >  -  }
> >  -
> >    return(parser->nread);
> >   }
> >
> >   int http_parser_finish(http_parser *parser)
> >   {
> >  -  int cs = parser->cs;
> >  -
> >  -  %%write eof;
> >  -
> >  -  parser->cs = cs;
> >  -
> >    if (http_parser_has_error(parser) ) {
> >      return -1;
> >    } else if (http_parser_is_finished(parser) ) {
> >  @@ -161,5 +149,5 @@ int http_parser_has_error(http_parser *parser) {
> >   }
> >
> >   int http_parser_is_finished(http_parser *parser) {
> >  -  return parser->cs == http_parser_first_final;
> >  +  return parser->cs >= http_parser_first_final;
> >   }
> >  --
> >
> >
> > Eric Wong
> -- 
> Evan Weaver
> Cloudburst, LLC

>  I never committed the Ragel 6 upgrade to trunk because I couldn''t
figure
>  out why it was broken.  I was hoping someone else would step in and fix
>  it for us :)
Maybe Ry knows. Otherwise, I''ll try to hack through it. I think
something might have to happen in the state transitions to make up for
the inability to explicitly close the output (is that even remotely
close?).
>  Digging into the revision history (with git :), the nasty_pound_header
>  test was committed in its commented form in r361 and has never changed
>  until now.
I''m not surprised, but there was no comment as to why it was disabled,
so I enabled it and let it fail. We should maybe remove that test
entirely and document multiline headers as not supported.

Thanks for looking in to this.

Evan


>  >
>  > On Sat, Mar 8, 2008 at 6:02 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>  > > Evan Weaver <evan at cloudbur.st> wrote:
>  > >
>  > > > Upgrading ragel is fine with me.
>  > >
>  > >  Here''s my work-in-progress.  The changes below are
pretty much copied
>  > >  from ry''s changes in ebb.
>  > >
>  > >  I haven''t been able to figure out why
>  > >  test_horrible_queries(HttpParserTest) is failing, so any help
here would
>  > >  be appreciated.
>  > >
>  > >  Thanks.
>  > >
>  > >  diff --git a/Rakefile b/Rakefile
>  > >  index f47d7a2..07a89c2 100644
>  > >  --- a/Rakefile
>  > >  +++ b/Rakefile
>  > >  @@ -55,13 +55,15 @@ task :ragel do
>  > >    Dir.chdir "ext/http11" do
>  > >      target = "http11_parser.c"
>  > >      File.unlink target if File.exist? target
>  > >  -    sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}"
>  > >  +    # sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}" # ragel 5.x
>  > >  +    sh "ragel -G2 http11_parser.rl" # ragel 6.0
>  > >      raise "Failed to build C source" unless
File.exist? target
>  > >    end
>  > >    Dir.chdir "ext/http11" do
>  > >      target =
"../../ext/http11_java/org/jruby/mongrel/Http11Parser.java"
>  > >      File.unlink target if File.exist? target
>  > >  -    sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}"
>  > >  +    # sh "ragel -J http11_parser.java.rl | rlgen-java -o
#{target}" # ragel 5.x
>  > >  +    sh "ragel -J -o #{target} http11_parser.java.rl"
# ragel 6.0
>  > >      raise "Failed to build Java source" unless
File.exist? target
>  > >    end
>  > >   end
>  > >  diff --git a/ext/http11/http11_parser.rl
b/ext/http11/http11_parser.rl
>  > >  index a418605..0c4e2d4 100644
>  > >  --- a/ext/http11/http11_parser.rl
>  > >  +++ b/ext/http11/http11_parser.rl
>  > >  @@ -114,7 +114,7 @@ size_t http_parser_execute(http_parser
*parser, const char *buffer, size_t len,
>  > >    p = buffer+off;
>  > >    pe = buffer+len;
>  > >
>  > >  -  assert(*pe == ''\0'' &&
"pointer does not end on NUL");
>  > >  +  /* assert(*pe == ''\0'' &&
"pointer does not end on NUL"); */
>  > >    assert(pe - p == len - off && "pointers
aren''t same distance");
>  > >
>  > >
>  > >  @@ -130,23 +130,11 @@ size_t http_parser_execute(http_parser
*parser, const char *buffer, size_t len,
>  > >    assert(parser->field_len <= len && "field
has length longer than whole buffer");
>  > >    assert(parser->field_start < len && "field
starts after buffer end");
>  > >
>  > >  -  if(parser->body_start) {
>  > >  -    /* final \r\n combo encountered so stop right here */
>  > >  -    %%write eof;
>  > >  -    parser->nread++;
>  > >  -  }
>  > >  -
>  > >    return(parser->nread);
>  > >   }
>  > >
>  > >   int http_parser_finish(http_parser *parser)
>  > >   {
>  > >  -  int cs = parser->cs;
>  > >  -
>  > >  -  %%write eof;
>  > >  -
>  > >  -  parser->cs = cs;
>  > >  -
>  > >    if (http_parser_has_error(parser) ) {
>  > >      return -1;
>  > >    } else if (http_parser_is_finished(parser) ) {
>  > >  @@ -161,5 +149,5 @@ int http_parser_has_error(http_parser
*parser) {
>  > >   }
>  > >
>  > >   int http_parser_is_finished(http_parser *parser) {
>  > >  -  return parser->cs == http_parser_first_final;
>  > >  +  return parser->cs >= http_parser_first_final;
>  > >   }
>  > >  --
>  > >
>  > >
>  > > Eric Wong
>  > --
>  > Evan Weaver
>  > Cloudburst, LLC
>
>
> _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>


-- 
Evan Weaver
Cloudburst, LLC

This is broken in Ebb too. I''ll put some time into it again today and
see if I can fix it.
ry

On Tue, Mar 25, 2008 at 5:53 AM, Evan Weaver <evan at cloudbur.st>
wrote:
> >  I never committed the Ragel 6 upgrade to trunk because I
couldn''t figure
>  >  out why it was broken.  I was hoping someone else would step in and
fix
>  >  it for us :)
>
>  Maybe Ry knows. Otherwise, I''ll try to hack through it. I think
>  something might have to happen in the state transitions to make up for
>  the inability to explicitly close the output (is that even remotely
>  close?).
>
>
>  >  Digging into the revision history (with git :), the
nasty_pound_header
>  >  test was committed in its commented form in r361 and has never
changed
>  >  until now.
>
>  I''m not surprised, but there was no comment as to why it was
disabled,
>  so I enabled it and let it fail. We should maybe remove that test
>  entirely and document multiline headers as not supported.
>
>  Thanks for looking in to this.
>
>
>
>  Evan
>
>
>
>  >  >
>  >  > On Sat, Mar 8, 2008 at 6:02 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>  >  > > Evan Weaver <evan at cloudbur.st> wrote:
>  >  > >
>  >  > > > Upgrading ragel is fine with me.
>  >  > >
>  >  > >  Here''s my work-in-progress.  The changes below
are pretty much copied
>  >  > >  from ry''s changes in ebb.
>  >  > >
>  >  > >  I haven''t been able to figure out why
>  >  > >  test_horrible_queries(HttpParserTest) is failing, so any
help here would
>  >  > >  be appreciated.
>  >  > >
>  >  > >  Thanks.
>  >  > >
>  >  > >  diff --git a/Rakefile b/Rakefile
>  >  > >  index f47d7a2..07a89c2 100644
>  >  > >  --- a/Rakefile
>  >  > >  +++ b/Rakefile
>  >  > >  @@ -55,13 +55,15 @@ task :ragel do
>  >  > >    Dir.chdir "ext/http11" do
>  >  > >      target = "http11_parser.c"
>  >  > >      File.unlink target if File.exist? target
>  >  > >  -    sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}"
>  >  > >  +    # sh "ragel http11_parser.rl | rlgen-cd -G2 -o
#{target}" # ragel 5.x
>  >  > >  +    sh "ragel -G2 http11_parser.rl" # ragel
6.0
>  >  > >      raise "Failed to build C source" unless
File.exist? target
>  >  > >    end
>  >  > >    Dir.chdir "ext/http11" do
>  >  > >      target =
"../../ext/http11_java/org/jruby/mongrel/Http11Parser.java"
>  >  > >      File.unlink target if File.exist? target
>  >  > >  -    sh "ragel -J http11_parser.java.rl | rlgen-java
-o #{target}"
>  >  > >  +    # sh "ragel -J http11_parser.java.rl |
rlgen-java -o #{target}" # ragel 5.x
>  >  > >  +    sh "ragel -J -o #{target}
http11_parser.java.rl" # ragel 6.0
>  >  > >      raise "Failed to build Java source" unless
File.exist? target
>  >  > >    end
>  >  > >   end
>  >  > >  diff --git a/ext/http11/http11_parser.rl
b/ext/http11/http11_parser.rl
>  >  > >  index a418605..0c4e2d4 100644
>  >  > >  --- a/ext/http11/http11_parser.rl
>  >  > >  +++ b/ext/http11/http11_parser.rl
>  >  > >  @@ -114,7 +114,7 @@ size_t
http_parser_execute(http_parser *parser, const char *buffer, size_t len,
>  >  > >    p = buffer+off;
>  >  > >    pe = buffer+len;
>  >  > >
>  >  > >  -  assert(*pe == ''\0'' &&
"pointer does not end on NUL");
>  >  > >  +  /* assert(*pe == ''\0'' &&
"pointer does not end on NUL"); */
>  >  > >    assert(pe - p == len - off && "pointers
aren''t same distance");
>  >  > >
>  >  > >
>  >  > >  @@ -130,23 +130,11 @@ size_t
http_parser_execute(http_parser *parser, const char *buffer, size_t len,
>  >  > >    assert(parser->field_len <= len &&
"field has length longer than whole buffer");
>  >  > >    assert(parser->field_start < len &&
"field starts after buffer end");
>  >  > >
>  >  > >  -  if(parser->body_start) {
>  >  > >  -    /* final \r\n combo encountered so stop right here
*/
>  >  > >  -    %%write eof;
>  >  > >  -    parser->nread++;
>  >  > >  -  }
>  >  > >  -
>  >  > >    return(parser->nread);
>  >  > >   }
>  >  > >
>  >  > >   int http_parser_finish(http_parser *parser)
>  >  > >   {
>  >  > >  -  int cs = parser->cs;
>  >  > >  -
>  >  > >  -  %%write eof;
>  >  > >  -
>  >  > >  -  parser->cs = cs;
>  >  > >  -
>  >  > >    if (http_parser_has_error(parser) ) {
>  >  > >      return -1;
>  >  > >    } else if (http_parser_is_finished(parser) ) {
>  >  > >  @@ -161,5 +149,5 @@ int http_parser_has_error(http_parser
*parser) {
>  >  > >   }
>  >  > >
>  >  > >   int http_parser_is_finished(http_parser *parser) {
>  >  > >  -  return parser->cs == http_parser_first_final;
>  >  > >  +  return parser->cs >= http_parser_first_final;
>  >  > >   }
>  >  > >  --
>  >  > >
>  >  > >
>  >  > > Eric Wong
>  >  > --
>  >  > Evan Weaver
>  >  > Cloudburst, LLC
>  >
>  >
>  > _______________________________________________
>  >  Mongrel-development mailing list
>  >  Mongrel-development at rubyforge.org
>  >  http://rubyforge.org/mailman/listinfo/mongrel-development
>  >
>
>
>
>  --
>  Evan Weaver
>  Cloudburst, LLC
>  _______________________________________________
>  Mongrel-development mailing list
>  Mongrel-development at rubyforge.org
>  http://rubyforge.org/mailman/listinfo/mongrel-development
>