[PATCH 0/2] eliminate generic ivars from HttpRequest class

[PATCH 0/2] eliminate generic ivars from HttpRequest class

[Eric Wong]

With the mainline Ruby VM, generic instance variables are implemented as
a st (hash) table for each object costing at least 192 bytes.  This
isn't a huge problem for unicorn as it only ever allocates one
HttpRequest object, but still makes the HttpRequest class less suitable
for other servers.

While generic ivars will be less expensive when Ruby 2.3 is released in
December, we're still better off eliminating them entirely as they're
not going to be cheaper than T_OBJECT instance variables.

With this, I'll probably tag and release 5.0.0-rc1 soon.

Eric Wong (2):
      move the socket into Rack env for hijacking
      http: move response_start_sent into the C ext

 ext/unicorn_http/unicorn_http.rl | 26 +++++++++++++++++++++++---
 lib/unicorn/http_request.rb      |  9 ++++-----
 test/unit/test_http_parser_ng.rb | 11 +++++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)

Note: Yes, [PATCH 1/2] introduces a unicorn-specific field into the
Rack env, but unicorn is not the only server with
`env["#{servername}.socket"]' in the Rack env.
And [PATCH 2/2] isn't useful without [PATCH 1/2]

[PATCH 1/2] move the socket into Rack env for hijacking

[Eric Wong]

This avoids the expensive generic instance variable for @socket
and exposes the socket as `env["unicorn.socket"]' to the Rack
application.

As as nice side-effect, applications may access
`env["unicorn.socket"]' as part of the API may be useful for
3rd-party bits such as Raindrops::TCP_Info for reading the tcp_info
struct on Linux-based systems.

Yes, `env["unicorn.socket"]' is a proprietary API in unicorn!
News at 11!  But then again, unicorn is not the first Rack server
to implement `env["#{servername}.socket"]', either...
---
 lib/unicorn/http_request.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index b60e383..f5c6b5b 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -33,6 +33,7 @@ class Unicorn::HttpParser
   # 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
@@ -99,7 +100,7 @@ class Unicorn::HttpParser
                     NULL_IO : @@input_class.new(socket, self)
 
     # for Rack hijacking in Rack 1.5 and later
-    @socket = socket
+    e[UNICORN_SOCKET] = socket
     e[RACK_HIJACK] = self
 
     e.merge!(DEFAULTS)
@@ -108,7 +109,7 @@ 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] = @socket
+    env[RACK_HIJACK_IO] = env[UNICORN_SOCKET]
   end
 
   def hijacked?
-- 
EW

[PATCH 2/2] http: move response_start_sent into the C ext

[Eric Wong]

Combined with the previous commit to eliminate the `@socket'
instance variable, this eliminates the last instance variable
in the Unicorn::HttpRequest class.

Eliminating the last instance variable avoids the creation of a
internal hash table used for implementing the "generic" instance
variables found in non-pure-Ruby classes.  Method entry overhead
remains the same.

While this change doesn't do a whole lot for unicorn memory usage
where the HttpRequest is a singleton, it helps other HTTP servers
which rely on this code where thousands of clients may be connected.
---
 ext/unicorn_http/unicorn_http.rl | 26 +++++++++++++++++++++++---
 lib/unicorn/http_request.rb      |  4 +---
 test/unit/test_http_parser_ng.rb | 11 +++++++++++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index bd45dd0..a5f069d 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -25,6 +25,7 @@ void init_unicorn_httpdate(void);
 #define UH_FL_KAVERSION 0x80
 #define UH_FL_HASHEADER 0x100
 #define UH_FL_TO_CLEAR 0x200
+#define UH_FL_RESSTART 0x400 /* for check_client_connection */
 
 /* all of these flags need to be set for keepalive to be supported */
 #define UH_FL_KEEPALIVE (UH_FL_KAVERSION | UH_FL_REQEOF | UH_FL_HASHEADER)
@@ -60,7 +61,7 @@ struct http_parser {
   } len;
 };
 
-static ID id_set_backtrace, id_response_start_sent;
+static ID id_set_backtrace;
 
 #ifdef HAVE_RB_HASH_CLEAR /* Ruby >= 2.0 */
 #  define my_hash_clear(h) (void)rb_hash_clear(h)
@@ -597,7 +598,6 @@ static VALUE HttpParser_clear(VALUE self)
 
   http_parser_init(hp);
   my_hash_clear(hp->env);
-  rb_ivar_set(self, id_response_start_sent, Qfalse);
 
   return self;
 }
@@ -880,6 +880,25 @@ static VALUE HttpParser_filter_body(VALUE self, VALUE dst, VALUE src)
   return src;
 }
 
+static VALUE HttpParser_rssset(VALUE self, VALUE boolean)
+{
+  struct http_parser *hp = data_get(self);
+
+  if (RTEST(boolean))
+    HP_FL_SET(hp, RESSTART);
+  else
+    HP_FL_UNSET(hp, RESSTART);
+
+  return boolean; /* ignored by Ruby anyways */
+}
+
+static VALUE HttpParser_rssget(VALUE self)
+{
+  struct http_parser *hp = data_get(self);
+
+  return HP_FL_TEST(hp, RESSTART) ? Qtrue : Qfalse;
+}
+
 #define SET_GLOBAL(var,str) do { \
   var = find_common_field(str, sizeof(str) - 1); \
   assert(!NIL_P(var) && "missed global field"); \
@@ -914,6 +933,8 @@ void Init_unicorn_http(void)
   rb_define_method(cHttpParser, "next?", HttpParser_next, 0);
   rb_define_method(cHttpParser, "buf", HttpParser_buf, 0);
   rb_define_method(cHttpParser, "env", HttpParser_env, 0);
+  rb_define_method(cHttpParser, "response_start_sent=", HttpParser_rssset, 1);
+  rb_define_method(cHttpParser, "response_start_sent", HttpParser_rssget, 0);
 
   /*
    * The maximum size a single chunk when using chunked transfer encoding.
@@ -939,7 +960,6 @@ void Init_unicorn_http(void)
   SET_GLOBAL(g_content_length, "CONTENT_LENGTH");
   SET_GLOBAL(g_http_connection, "CONNECTION");
   id_set_backtrace = rb_intern("set_backtrace");
-  id_response_start_sent = rb_intern("@response_start_sent");
   init_unicorn_httpdate();
 
 #ifndef HAVE_RB_HASH_CLEAR
diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index f5c6b5b..9339bce 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -25,8 +25,6 @@ class Unicorn::HttpParser
   RACK_HIJACK_IO = "rack.hijack_io".freeze
   NULL_IO = StringIO.new("")
 
-  attr_accessor :response_start_sent
-
   # :stopdoc:
   # A frozen format for this is about 15% faster
   # Drop these frozen strings when Ruby 2.2 becomes more prevalent,
@@ -92,7 +90,7 @@ class Unicorn::HttpParser
 
     # detect if the socket is valid by writing a partial response:
     if @@check_client_connection && headers?
-      @response_start_sent = true
+      self.response_start_sent = true
       HTTP_RESPONSE_START.each { |c| socket.write(c) }
     end
 
diff --git a/test/unit/test_http_parser_ng.rb b/test/unit/test_http_parser_ng.rb
index efd82e1..d186f5a 100644
--- a/test/unit/test_http_parser_ng.rb
+++ b/test/unit/test_http_parser_ng.rb
@@ -34,6 +34,17 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal false, @parser.response_start_sent
   end
 
+  def test_response_start_sent
+    assert_equal false, @parser.response_start_sent, "default is false"
+    @parser.response_start_sent = true
+    assert_equal true, @parser.response_start_sent
+    @parser.response_start_sent = false
+    assert_equal false, @parser.response_start_sent
+    @parser.response_start_sent = true
+    @parser.clear
+    assert_equal false, @parser.response_start_sent
+  end
+
   def test_connection_TE
     @parser.buf << "GET / HTTP/1.1\r\nHost: example.com\r\nConnection: TE\r\n"
     @parser.buf << "TE: trailers\r\n\r\n"
-- 
EW