Performance improvement and benchmark #200

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
@indutny
Member

indutny commented Nov 28, 2014

n/t

cc @bnoordhuis

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Nov 28, 2014

Contributor

base benchmarks (as of 3f1a05a)

$ rm -rf bench; make clean; make -j8; make bench; ./bench
rm -f *.o *.a tags test test_fast test_g \
        http_parser.tar libhttp_parser.so.* \
        url_parser url_parser_g parsertrace parsertrace_g
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c http_parser.c -o http_parser_g.o
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c test.c -o test_g.o
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c http_parser.c
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c test.c -o test.o
cc -Wall -Wextra -Werror -O0 -g   http_parser_g.o test_g.o -o test_g
cc -Wall -Wextra -Werror -O3   http_parser.o test.o -o test_fast
./test_g
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
./test_fast
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -Wno-unused-parameter -c bench.c -o bench.o
cc -Wall -Wextra -Werror -O3  -Wno-unused-parameter  http_parser.o bench.o -o bench
Benchmark result:
Took 17.883043 seconds to run
279594.468750 req/sec

bench as of 9bf763e

$ rm -rf bench; make clean; make -j8; make bench; ./bench
rm -f *.o *.a tags test test_fast test_g \
        http_parser.tar libhttp_parser.so.* \
        url_parser url_parser_g parsertrace parsertrace_g
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c http_parser.c -o http_parser_g.o
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c test.c -o test_g.o
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c http_parser.c
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c test.c -o test.o
cc -Wall -Wextra -Werror -O0 -g   http_parser_g.o test_g.o -o test_g
cc -Wall -Wextra -Werror -O3   http_parser.o test.o -o test_fast
./test_g
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
./test_fast
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -Wno-unused-parameter -c bench.c -o bench.o
cc -Wall -Wextra -Werror -O3  -Wno-unused-parameter  http_parser.o bench.o -o bench
Benchmark result:
Took 5.048899 seconds to run
990314.875000 req/sec
Contributor

chrisdickinson commented Nov 28, 2014

base benchmarks (as of 3f1a05a)

$ rm -rf bench; make clean; make -j8; make bench; ./bench
rm -f *.o *.a tags test test_fast test_g \
        http_parser.tar libhttp_parser.so.* \
        url_parser url_parser_g parsertrace parsertrace_g
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c http_parser.c -o http_parser_g.o
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c test.c -o test_g.o
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c http_parser.c
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c test.c -o test.o
cc -Wall -Wextra -Werror -O0 -g   http_parser_g.o test_g.o -o test_g
cc -Wall -Wextra -Werror -O3   http_parser.o test.o -o test_fast
./test_g
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
./test_fast
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -Wno-unused-parameter -c bench.c -o bench.o
cc -Wall -Wextra -Werror -O3  -Wno-unused-parameter  http_parser.o bench.o -o bench
Benchmark result:
Took 17.883043 seconds to run
279594.468750 req/sec

bench as of 9bf763e

$ rm -rf bench; make clean; make -j8; make bench; ./bench
rm -f *.o *.a tags test test_fast test_g \
        http_parser.tar libhttp_parser.so.* \
        url_parser url_parser_g parsertrace parsertrace_g
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c http_parser.c -o http_parser_g.o
cc -I. -DHTTP_PARSER_STRICT=1  -Wall -Wextra -Werror -O0 -g  -c test.c -o test_g.o
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c http_parser.c
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -c test.c -o test.o
cc -Wall -Wextra -Werror -O0 -g   http_parser_g.o test_g.o -o test_g
cc -Wall -Wextra -Werror -O3   http_parser.o test.o -o test_fast
./test_g
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
./test_fast
http_parser v2.3.0 (0x020300)
sizeof(http_parser) = 32
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
cc -I. -DHTTP_PARSER_STRICT=0  -Wall -Wextra -Werror -O3  -Wno-unused-parameter -c bench.c -o bench.o
cc -Wall -Wextra -Werror -O3  -Wno-unused-parameter  http_parser.o bench.o -o bench
Benchmark result:
Took 5.048899 seconds to run
990314.875000 req/sec
http_parser.c
+
+ pos = memchr(p, CR, data + len - p);
+ if (pos == NULL)
+ pos = memchr(p, LF, data + len - p);

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 28, 2014

Member

I believe this will parse e.g. "Foo: bar\nBar: baz\r\n" incorrectly. You should probably scan forward one character at a time until you find CR or LF.

@bnoordhuis

bnoordhuis Nov 28, 2014

Member

I believe this will parse e.g. "Foo: bar\nBar: baz\r\n" incorrectly. You should probably scan forward one character at a time until you find CR or LF.

This comment has been minimized.

@indutny

indutny Nov 28, 2014

Member

Oooh, right. I guess it should be enough to just search for \r, as it is a general case.

@indutny

indutny Nov 28, 2014

Member

Oooh, right. I guess it should be enough to just search for \r, as it is a general case.

This comment has been minimized.

@indutny

indutny Nov 28, 2014

Member

Wait, I don't see any problems with this. It goes back to s_header_value anyway after finding either \r or \n.

@indutny

indutny Nov 28, 2014

Member

Wait, I don't see any problems with this. It goes back to s_header_value anyway after finding either \r or \n.

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 28, 2014

Member

I'm not sure I follow. With "Foo: bar\nBar: baz\r\n" as input, this code would skip to the '\r' character and miss the Baz header altogether, right?

@bnoordhuis

bnoordhuis Nov 28, 2014

Member

I'm not sure I follow. With "Foo: bar\nBar: baz\r\n" as input, this code would skip to the '\r' character and miss the Baz header altogether, right?

This comment has been minimized.

@marcomorain

marcomorain Dec 2, 2014

Contributor

Sounds like a good test-case to add?

On Fri, Nov 28, 2014 at 3:48 PM, Ben Noordhuis notifications@github.com
wrote:

In http_parser.c:

  •    c = LOWER(ch);
    
  •      switch (h_state) {
    
  •        case h_general:
    
  •        {
    
  •          const char\* pos;
    
  •          pos = memchr(p, CR, data + len - p);
    
  •          if (pos == NULL)
    
  •            pos = memchr(p, LF, data + len - p);
    

I'm not sure I follow. With "Foo: bar\nBar: baz\r\n" as input, this code
would skip to the '\r' character and miss the Baz header altogether,
right?


Reply to this email directly or view it on GitHub
https://github.com/joyent/http-parser/pull/200/files#r21046830.

@marcomorain

marcomorain Dec 2, 2014

Contributor

Sounds like a good test-case to add?

On Fri, Nov 28, 2014 at 3:48 PM, Ben Noordhuis notifications@github.com
wrote:

In http_parser.c:

  •    c = LOWER(ch);
    
  •      switch (h_state) {
    
  •        case h_general:
    
  •        {
    
  •          const char\* pos;
    
  •          pos = memchr(p, CR, data + len - p);
    
  •          if (pos == NULL)
    
  •            pos = memchr(p, LF, data + len - p);
    

I'm not sure I follow. With "Foo: bar\nBar: baz\r\n" as input, this code
would skip to the '\r' character and miss the Baz header altogether,
right?


Reply to this email directly or view it on GitHub
https://github.com/joyent/http-parser/pull/200/files#r21046830.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 28, 2014

Member

Needs some clarification about end-of-line scanning but apart from that LGTM and nice work. As discussed on IRC, scanning for known headers could probably be optimized some more using e.g. Boyer-Moore skip tables.

Member

bnoordhuis commented Nov 28, 2014

Needs some clarification about end-of-line scanning but apart from that LGTM and nice work. As discussed on IRC, scanning for known headers could probably be optimized some more using e.g. Boyer-Moore skip tables.

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Nov 29, 2014

Member

@bnoordhuis please take a look at latest commit, it should fix the issue with end-of-line scan.

Member

indutny commented Nov 29, 2014

@bnoordhuis please take a look at latest commit, it should fix the issue with end-of-line scan.

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Nov 29, 2014

Member

All landed in 3f1a05a, 6132d1f, 0cb0ee6, 2630060, c6097e1, 0097de5, 959f4cb. Thank you!

Member

indutny commented Nov 29, 2014

All landed in 3f1a05a, 6132d1f, 0cb0ee6, 2630060, c6097e1, 0097de5, 959f4cb. Thank you!

@indutny indutny closed this Nov 29, 2014

@indutny indutny deleted the indutny:feature/perf-improvement branch Nov 29, 2014

@indutny indutny referenced this pull request in h2o/picohttpparser Nov 29, 2014

Closed

joyent/http-parser #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment