[PATCH 0/3] reflect changes to Rack::Utils::HTTP_STATUS_CODES

[PATCH 0/3] reflect changes to Rack::Utils::HTTP_STATUS_CODES

[Eric Wong]

A user privately reported (to unicorn@bogomips.org) that they wanted
status text reflected in their responses for an HTTP status code not
included with Rack::Utils::HTTP_STATUS_CODES.

They tried to modify Rack::Utils::HTTP_STATUS_CODES hash directly in
their app, but unicorn loads before their app and already memoized the
status codes internally in the CODES hash to reduce GC pressure, thus
their change was never reflected.

With the first change, users can change Rack::Utils::HTTP_STATUS_CODES
as many times as they want (perhaps even based on time-of-day, weather,
server load, whatever) and the changes will be reflected
instantaneously in responses.

Of course, this slows unicorn down slightly due to increased GC
pressure, but I doubt anybody would notice in Real World usage.
In case they do, patch 2/3 will recover the lost performance
if they're using Ruby 2.2+

I figure anybody who cares about micro-benchmark performance with
unicorn would be using the latest Rubies anyways...

That user is Bcc:-ed for these patches, and can follow any public
discussion at: http://bogomips.org/unicorn-public/
(including the Atom feed at http://bogomips.org/unicorn-public/atom.xml )

* [PATCH 1/3] reflect changes in Rack::Utils::HTTP_STATUS_CODES
  introduces a performance regression

* [PATCH 2/3] reduce constants and optimize for Ruby 2.2
  recover lost performance from [PATCH 1/3] (on Ruby 2.2+),
  further regressions for 2.1+

* [PATCH 3/3] http_response: reduce size of multi-line header path
  bonus reduce code size for common cases, should

 lib/unicorn/http_request.rb  | 19 +++++++------------
 lib/unicorn/http_response.rb | 23 ++++++++++-------------
 test/unit/test_response.rb   | 20 ++++++++++++++++++++
 3 files changed, 37 insertions(+), 25 deletions(-)

[PATCH 1/3] reflect changes in Rack::Utils::HTTP_STATUS_CODES

[Eric Wong]

Applications may want to alter the message associated with HTTP
status codes in Rack::Utils::HTTP_STATUS_CODES.  Avoid memoizing
status lines ahead-of-time

Note: this introduces a minor performance regression, but ought to
be unnoticeable unless you're running "Hello world"-type apps.
---
 lib/unicorn/http_response.rb | 15 +++++++--------
 test/unit/test_response.rb   | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index 016dac8..a42303e 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -10,15 +10,12 @@
 # is the job of Rack, with the exception of the "Date" and "Status" header.
 module Unicorn::HttpResponse
 
-  # Every standard HTTP code mapped to the appropriate message.
-  CODES = Rack::Utils::HTTP_STATUS_CODES.inject({}) { |hash,(code,msg)|
-    hash[code] = "#{code} #{msg}"
-    hash
-  }
   CRLF = "\r\n"
 
+  # internal API, code will always be common-enough-for-even-old-Rack
   def err_response(code, response_start_sent)
-    "#{response_start_sent ? '' : 'HTTP/1.1 '}#{CODES[code]}\r\n\r\n"
+    "#{response_start_sent ? '' : 'HTTP/1.1 '}" \
+      "#{code} #{Rack::Utils::HTTP_STATUS_CODES[code]}\r\n\r\n"
   end
 
   # writes the rack_response to socket as an HTTP response
@@ -26,9 +23,11 @@ module Unicorn::HttpResponse
                           response_start_sent=false)
     hijack = nil
 
-    http_response_start = response_start_sent ? '' : 'HTTP/1.1 '
     if headers
-      buf = "#{http_response_start}#{CODES[status.to_i] || status}\r\n" \
+      code = status.to_i
+      msg = Rack::Utils::HTTP_STATUS_CODES[code]
+      start = response_start_sent ? ''.freeze : 'HTTP/1.1 '.freeze
+      buf = "#{start}#{msg ? %Q(#{code} #{msg}) : status}\r\n" \
             "Date: #{httpdate}\r\n" \
             "Connection: close\r\n"
       headers.each do |key, value|
diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb
index 3478288..1e9d74a 100644
--- a/test/unit/test_response.rb
+++ b/test/unit/test_response.rb
@@ -79,4 +79,24 @@ class ResponseTest < Test::Unit::TestCase
     headers = out.string.split(/\r\n\r\n/).first.split(/\r\n/)
     assert %r{\AHTTP/\d\.\d 666 I AM THE BEAST\z}.match(headers[0])
   end
+
+  def test_modified_rack_http_status_codes_late
+    r, w = IO.pipe
+    pid = fork do
+      r.close
+      # Users may want to globally override the status text associated
+      # with an HTTP status code in their app.
+      Rack::Utils::HTTP_STATUS_CODES[200] = "HI"
+      http_response_write(w, 200, {}, [])
+      w.close
+    end
+    w.close
+    assert_equal "HTTP/1.1 200 HI\r\n", r.gets
+    r.read # just drain the pipe
+    pid, status = Process.waitpid2(pid)
+    assert_predicate status, :success?
+  ensure
+    r.close
+    w.close unless w.closed?
+  end
 end
-- 
EW

[PATCH 2/3] reduce constants and optimize for Ruby 2.2

[Eric Wong]

Ruby (MRI) 2.1 optimizes allocations away on String#freeze with
literal strings.

Furthermore, Ruby 2.2 optimizes away literal string allocations
when they are used as arguments to Hash#[] and Hash#[]=

Thus we can avoid expensive constant lookups and cache overhead
by taking advantage of advancements in Ruby.

Since Ruby 2.2 has been out for 7 months, now; it ought to be safe
to introduce minor performance regressions for folks using older
Rubies (1.9.3+ remains supported) to benefit folks on the latest
Ruby.

This should recover the performance lost in the
"reflect changes in Rack::Utils::HTTP_STATUS_CODES" change
in synthetic benchmarks.
---
 lib/unicorn/http_request.rb  | 19 +++++++------------
 lib/unicorn/http_response.rb |  6 ++----
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index 9339bce..0c1f9bb 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -21,17 +21,12 @@ class Unicorn::HttpParser
     "SERVER_SOFTWARE" => "Unicorn #{Unicorn::Const::UNICORN_VERSION}"
   }
 
-  RACK_HIJACK = "rack.hijack".freeze
-  RACK_HIJACK_IO = "rack.hijack_io".freeze
   NULL_IO = StringIO.new("")
 
   # :stopdoc:
   # A frozen format for this is about 15% faster
   # Drop these frozen strings when Ruby 2.2 becomes more prevalent,
   # 2.2+ optimizes hash assignments when used with literal string keys
-  REMOTE_ADDR = 'REMOTE_ADDR'.freeze
-  RACK_INPUT = 'rack.input'.freeze
-  UNICORN_SOCKET = 'unicorn.socket'.freeze
   HTTP_RESPONSE_START = [ 'HTTP', '/1.1 ']
   @@input_class = Unicorn::TeeInput
   @@check_client_connection = false
@@ -78,7 +73,7 @@ class Unicorn::HttpParser
     #  identify the client for the immediate request to the server;
     #  that client may be a proxy, gateway, or other intermediary
     #  acting on behalf of the actual source client."
-    e[REMOTE_ADDR] = socket.kgio_addr
+    e['REMOTE_ADDR'] = socket.kgio_addr
 
     # short circuit the common case with small GET requests first
     socket.kgio_read!(16384, buf)
@@ -94,12 +89,12 @@ class Unicorn::HttpParser
       HTTP_RESPONSE_START.each { |c| socket.write(c) }
     end
 
-    e[RACK_INPUT] = 0 == content_length ?
-                    NULL_IO : @@input_class.new(socket, self)
+    e['rack.input'] = 0 == content_length ?
+                      NULL_IO : @@input_class.new(socket, self)
 
     # for Rack hijacking in Rack 1.5 and later
-    e[UNICORN_SOCKET] = socket
-    e[RACK_HIJACK] = self
+    e['unicorn.socket'] = socket
+    e['rack.hijack'] = self
 
     e.merge!(DEFAULTS)
   end
@@ -107,10 +102,10 @@ class Unicorn::HttpParser
   # for rack.hijack, we respond to this method so no extra allocation
   # of a proc object
   def call
-    env[RACK_HIJACK_IO] = env[UNICORN_SOCKET]
+    env['rack.hijack_io'] = env['unicorn.socket']
   end
 
   def hijacked?
-    env.include?(RACK_HIJACK_IO)
+    env.include?('rack.hijack_io'.freeze)
   end
 end
diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index a42303e..ec8b7c8 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -10,8 +10,6 @@
 # is the job of Rack, with the exception of the "Date" and "Status" header.
 module Unicorn::HttpResponse
 
-  CRLF = "\r\n"
-
   # internal API, code will always be common-enough-for-even-old-Rack
   def err_response(code, response_start_sent)
     "#{response_start_sent ? '' : 'HTTP/1.1 '}" \
@@ -39,7 +37,7 @@ module Unicorn::HttpResponse
           # key in Rack < 1.5
           hijack = value
         else
-          if value =~ /\n/
+          if value.include?("\n".freeze)
             # avoiding blank, key-only cookies with /\n+/
             buf << value.split(/\n+/).map! { |v| "#{key}: #{v}\r\n" }.join
           else
@@ -47,7 +45,7 @@ module Unicorn::HttpResponse
           end
         end
       end
-      socket.write(buf << CRLF)
+      socket.write(buf << "\r\n".freeze)
     end
 
     if hijack
-- 
EW

[PATCH 3/3] http_response: reduce size of multi-line header path

[Eric Wong]

This should save over 100 bytes of bytecode overhead due to
reduced method dispatch points.  The performance difference
when this is actually hit could go either way depending on
how String#<< and realloc(3) interact, but it's uncommon
enough that nobody is expected to notice either way.
---
 lib/unicorn/http_response.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index ec8b7c8..c1aa738 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -39,7 +39,7 @@ module Unicorn::HttpResponse
         else
           if value.include?("\n".freeze)
             # avoiding blank, key-only cookies with /\n+/
-            buf << value.split(/\n+/).map! { |v| "#{key}: #{v}\r\n" }.join
+            value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
           else
             buf << "#{key}: #{value}\r\n"
           end
-- 
EW