DO NOT MERGE: less ref counting

[weissi]

this is a mess, sorry

but

before:

$ .build/release/bench
Benchmark result:
Took 7.7 seconds to run
648582.28 req/sec

after:

$ .build/release/bench
Benchmark result:
Took 5.27 seconds to run
948171.8 req/sec

[helje5]

Using escaping closures instead of a protocol was way slower in my tests.

Unrelated, missing in the original too: the callbacks need the parser as the first argument (like in the original) to provide the necessary context (w/o requiring capturing closures).

[dsperling]

struct http_parser_delegate combined with struct http_parser is indeed faster. I tried protocol http_parser_delegate with struct http_parser and it was slower than using protocol http_parser_delegate and class http_parser due to extra thunking inserted by the compiler.

@helje5, as you point out, the bench.c program does nothing, so it does not measure any penalty referencing a required context. What we should consider is creating a benchmark where the 'C' and 'Swift' version both use the context to perform something more meaningful (like count callbacks and byte counts). We should see the benchmark results get closer.

Another approach would be wrap the 'C' version with a Swift class like Kitura's HTTPParser wrapper and measure the performance with a pure Swift implementation. This would be closer to where we need to eventually be.

[helje5]

I would really like to know why a protocol would be any slower. What is that 'thunking', is that documented somewhere? I would expect a (non objc Swift) protocol to be essentially a plain vtable. That should be way faster than escaping closures.

I don't understand the HTTPParser wrapper you are pointing to. That looks entirely pointless/superfluous to me.

[dsperling]

Here is a performance comparison with the two scenarios I mentioned above. I cannot explain why protocol http_parser_delegate + struct http_parser is the slowest combination, but it seems like extra retain/release calls are involved.

[helje5]

In the original the parser itself doesn't maintain the callbacks, it is kept in a separate struct which is passed to execute(). This makes sense because the parser is reentrant/stateless and the mapping is usually static (only one copy needed). Remember that the idea is to keep the per-connection overhead as small as possible (in the original only 40 bytes per connection!).

[helje5]

This should not be a String in the first place but just an Int num. You never use those cases as Strings anyways but the description property.

[dsperling]

The following function uses the String (StaticString) to provide the name of the error via .rawValue:

/* Return a string name of the given error */
public class func errno_name(_ err: http_errno) -> String {
  return err.rawValue
}

This function is not used in bench.c/.swift so I thought it would not effect performance. However I replaced StaticString with String and the benchmark did slow down. More investigation is required.

[helje5]

Actually I was kinda wrong on this one. I was assuming that a Swift String enum would use Strings at runtime to match cases. That actually doesn't seem to be the case. The enum is always some small integer and it the compiler generates the switch to do the raw value mapping from/to string.

But for the specific function you mention, that kinda works for any enum: "(err)" and matches what the C variant does.

[helje5]

I would @inline(__always) such until the compiler crashes :-)

      (__)
    /  .\/.     ______
   |  /\_|     |      \
   |  |___     |       |
   |   ---@    |_______|
*  |  |   ----   |    |
 \ |  |_____
  \|________|

CompuCow Discovers Bug in Compiler

[dsperling]

I tried this on a number of functions like LOWER(), IS_ALPHA(), IS_NUM(), IS_ALPHANUM() and I did not see any performance improvement in Release mode.

[helje5]

This isn't mutating?

[dsperling]

Mutating is required on 10 or so functions if using struct http_parser. This is not required using class http_parser.

The compiler error is:
Mark method 'mutating' to make 'self' mutable

[helje5]

Not sure what you are trying to say here. This specific function (STRICT_TOKEN) isn't mutating. It is just: return tokens[Int(c)]. If the compiler throws an error on this, it really looks like a bug?

[dsperling]

You are correct. STRICT_TOKEN does not require mutating. My mistake.