celluloid/reel — pull request #18 by slivu (2012-11-21 15:15:30+00:00)
Title: Performance Related Refactoring - Pahse_01

### Before

```
$ httperf --hog --port 9292 --num-conns=1 --num-calls=1000
...
Request rate: 2852.5 req/s (0.4 ms/req)
...
Reply status: 1xx=0 2xx=1000 3xx=0 4xx=0 5xx=0
```

### After

```
$ httperf --hog --port 9292 --num-conns=1 --num-calls=1000
...
Request rate: 4420.1 req/s (0.2 ms/req)
...
Reply status: 1xx=0 2xx=1000 3xx=0 4xx=0 5xx=0
```

### Commenting updates

```
diff --git a/lib/reel/rack_worker.rb b/lib/reel/rack_worker.rb
index a22a70c..ef06b2d 100644
--- a/lib/reel/rack_worker.rb
+++ b/lib/reel/rack_worker.rb
@@ -17,17 +17,13 @@ module Reel
HTTP_1_1 = 'HTTP/1.1'.freeze
CGI_1_1 = 'CGI/1.1'.freeze
REMOTE_ADDR = 'REMOTE_ADDR'.freeze

# REMOTE_HOST not really needed. details below
- REMOTE_HOST = 'REMOTE_HOST'.freeze
CONNECTION = 'HTTP_CONNECTION'.freeze
SCRIPT_NAME = 'SCRIPT_NAME'.freeze
PATH_INFO = 'PATH_INFO'.freeze
REQUEST_METHOD = 'REQUEST_METHOD'.freeze

# REQUEST_PATH, ORIGINAL_FULLPATH, REQUEST_URI not used by Rack
- REQUEST_PATH = 'REQUEST_PATH'.freeze
- ORIGINAL_FULLPATH = 'ORIGINAL_FULLPATH'.freeze
QUERY_STRING = 'QUERY_STRING'.freeze
- REQUEST_URI = 'REQUEST_URI'.freeze

# no more regexing, it is too expensive. details below
- CONTENT_TYPE_RGXP = /^content-type$/i.freeze
- CONTENT_LENGTH_RGXP = /^content-length$/i.freeze
+ CONTENT_TYPE = 'Content-Type'.freeze
+ CONTENT_LENGTH = 'Content-Length'.freeze
HTTP_ = 'HTTP_'.freeze
HOST = 'Host'.freeze

@@ -40,10 +36,18 @@ module Reel
RACK_MULTIPROCESS = 'rack.multiprocess'.freeze
RACK_RUN_ONCE = 'rack.run_once'.freeze
RACK_URL_SCHEME = 'rack.url_scheme'.freeze

# we do not need EM-ish stuff
- ASYNC_CALLBACK = 'async.callback'.freeze
- ASYNC_CLOSE = 'async.close'.freeze

RACK_WEBSOCKET = 'rack.websocket'.freeze

# get request method from a Hash
# rather than use something.to_s.upcase on each request
+ REQUEST_METHODS = {
+ :get => 'GET'.freeze,
+ :post => 'POST'.freeze,
+ :put => 'PUT'.freeze,
+ :head => 'HEAD'.freeze,
+ :delete => 'DELETE'.freeze,
+ :options => 'OPTIONS'.freeze,
+ :patch => 'PATCH'.freeze,
+ }
+
PROTO_RACK_ENV = {
RACK_VERSION => Rack::VERSION,
RACK_ERRORS => STDERR,
@@ -73,8 +77,7 @@ module Reel
end

def handle_request(request, connection)

# splitting env into request_env and websocket_env
- env = rack_env(request, connection)
- status, headers, body_parts = @app.call(env)
+ status, headers, body_parts = @app.call(request_env request, connection)
body = response_body(body_parts)

connection.respond Response.new(status, headers, body)
@@ -84,8 +87,16 @@ module Reel
end

def handle_websocket(request, connection)

# splitting env into request_env and websocket_env
- env = rack_env(request, connection)
- @app.call(env)
+ status, *rest = @app.call(websocket_env request)
+ request.socket.close unless status < 300
+ end
+
+ def request_env request, connection
+ env(request).merge REMOTE_ADDR => connection.remote_ip
+ end
+
+ def websocket_env request
+ env(request).merge REMOTE_ADDR => request.remote_ip, RACK_WEBSOCKET => request
end

def response_body(body_parts)
@@ -98,51 +109,39 @@ module Reel
end
end

# moved to env
- def rack_env(request, connection)
- env = PROTO_RACK_ENV.dup
-
- env[SERVER_NAME] = request[HOST].to_s.split(':').first || @handler[:Host]
- env[SERVER_PORT] = @handler[:port].to_s
+ private

- case request
- when WebSocket
- remote_connection = request
- env[RACK_WEBSOCKET] = request
- when Request
- remote_connection = connection
- end
-
- env[REMOTE_ADDR] = remote_connection.remote_ip

# DNS lookups consumes about 500 rps!
# and this is not really needed.
# in fact really not needed.
# Thin and others surviving well without this.
# there are enough resolvers in userland
# so no need to slow down the server with this.
- env[REMOTE_HOST] = remote_connection.remote_host
-
- env[PATH_INFO] = request.path
- env[REQUEST_METHOD] = request.method.to_s.upcase
+ def env request

# Hash[] is faster than hash#dup by about 50%
+ env = Hash[PROTO_RACK_ENV]

env[RACK_INPUT] = StringIO.new(request.body || INITIAL_BODY)
env[RACK_INPUT].set_encoding(Encoding::BINARY) if env[RACK_INPUT].respond_to?(:set_encoding)

# not really needed so excluding extra overhead
- env[RACK_LOGGER] = @app if Rack::CommonLogger === @app

# if request contains host:port pair
# no need to spend time on @handler[:port].to_s
+ env[SERVER_NAME], env[SERVER_PORT] = (request[HOST]||'').split(':', 2)
+ env[SERVER_PORT] ||= @handler[:port].to_s

# request object does contain HTTP version data.
# trying this and fallback to server protocol
+ env[HTTP_VERSION] = request.version || env[SERVER_PROTOCOL]

# REQUEST_PATH, ORIGINAL_FULLPATH, REQUEST_URI not used by Rack
- env[REQUEST_PATH] = request.path
- env[ORIGINAL_FULLPATH] = request.path
- env[REQUEST_URI] = request.path

# get request method from a Hash
# rather than use something.to_s.upcase on each request
+ env[REQUEST_METHOD] = REQUEST_METHODS[request.method] ||
+ raise(ArgumentError, "unknown request method: %s" % request.method)

# fragment not used by Rack and it will be ignored even if we attach it
- query_string = request.query_string || ''
- query_string << "##{request.fragment}" if request.fragment

+ env[PATH_INFO] = request.path
+ env[QUERY_STRING] = request.query_string || ''

# both moved upper
- env[HTTP_VERSION] ||= env[SERVER_PROTOCOL]
- env[QUERY_STRING] = query_string

# rather than regexing against 2 strings on each header
# simply deleting content type and length from headers.
# with regexing we have a N*2 speed degradation where N is the number of headers
+ # seems headers wont be used anymore so it is safe to alter them.
+ (_ = request.headers.delete CONTENT_TYPE) && (env[CONTENT_TYPE] = _)
+ (_ = request.headers.delete CONTENT_LENGTH) && (env[CONTENT_LENGTH] = _)
- request.headers.each do |key, val|
- next if CONTENT_TYPE_RGXP =~ key
- next if CONTENT_LENGTH_RGXP =~ key
- name = HTTP_ + key
- name.gsub!(/-/o, '_')
- name.upcase!
- env[name] = val

# no need to do expensive repetitive string operations on each request
# instead we can store keys in a class variable and query it.
# this is a lot faster.
+ request.headers.each_pair do |key, val|
+ env[header_keys(key)] = val
end

env
end
+
+ # avoid expensive repetitive string operations on each request.
+ def header_keys key
+ (@@header_keys ||= {})[key] ||= HTTP_ + key.sub('-', '_').upcase
+ end
+
end
end
diff --git a/lib/reel/websocket.rb b/lib/reel/websocket.rb
index 3cdb672..236c76c 100644
--- a/lib/reel/websocket.rb
+++ b/lib/reel/websocket.rb
@@ -5,7 +5,7 @@ module Reel
include RemoteConnection
include URIParts

- attr_reader :url, :headers, :method

# adding version to comply with usual http request object
# so env[HTTP_VERSION] = request.version || env[SERVER_PROTOCOL]
# would work on both request types
+ attr_reader :url, :headers, :method, :version

def initialize(socket, method, url, headers)
@socket, @method, @url, @headers = socket, method, url, headers
diff --git a/spec/reel/rack_worker_spec.rb b/spec/reel/rack_worker_spec.rb
index eafde8e..00987ee 100644
--- a/spec/reel/rack_worker_spec.rb
+++ b/spec/reel/rack_worker_spec.rb
@@ -3,14 +3,13 @@ require 'spec_helper'
describe Reel::RackWorker do
let(:endpoint) { URI(example_url) }

- let(:worker) do
- app = Proc.new do |env|
- [200, {'Content-Type' => 'text/plain'}, ['Hello world!']]
- [200, {'Content-Type' => 'text/plain'}, ['Hello rack world!']]
- end

# needed to pass app through Rack::Lint
+ RackApp = Proc.new do |env|
+ [200, {'Content-Type' => 'text/plain'}, ['Hello rack world!']]
+ end

+ let(:worker) do
handler = Rack::Handler::Reel.new
- handler.options[:app] = app
+ handler.options[:app] = RackApp

Reel::RackWorker.new(handler)
end
@@ -19,7 +18,7 @@ describe Reel::RackWorker do
with_socket_pair do |client, connection|
client << ExampleRequest.new(:get, '/test?hello=true').to_s
request = connection.request
- env = worker.rack_env(request, connection)

# using new env method
+ env = worker.request_env(request, connection)

Reel::RackWorker::PROTO_RACK_ENV.each do |k, v|
env[k].should == v
@@ -30,17 +29,16 @@ describe Reel::RackWorker do
env["REMOTE_ADDR"].should == "127.0.0.1"
env["PATH_INFO"].should == "/test"
env["REQUEST_METHOD"].should == "GET"

# REQUEST_PATH, ORIGINAL_FULLPATH, REQUEST_URI not used by Rack
- env["REQUEST_PATH"].should == "/test"
- env["ORIGINAL_FULLPATH"].should == "/test"
env["QUERY_STRING"].should == "hello=true"
env["HTTP_HOST"].should == 'www.example.com'
env["HTTP_ACCEPT_LANGUAGE"].should == "en-US,en;q=0.8"
- env["REQUEST_URI"].should == '/test'
-

# we do not want to spend time on reverse DNS stuff
- %w(localhost 127.0.0.1).should include env["REMOTE_HOST"]

env["rack.input"].should be_kind_of(StringIO)
env["rack.input"].string.should == ''

# passing app and env through Rack::Lint
# to make sure we fully comply with Rack standards
+ validator = ::Rack::Lint.new(RackApp)
+ status, *rest = validator.call(env)
+ status.should == 200
end
end

@@ -51,7 +49,7 @@ describe Reel::RackWorker do
with_socket_pair do |client, connection|
client << handshake.to_data
request = connection.request
- env = worker.rack_env(request, connection)
+ env = worker.websocket_env(request)

+ env["REMOTE_ADDR"].should == "127.0.0.1"
env["rack.websocket"].should == request
end
```