[RFC] http: TypedData C-API conversion

[RFC] http: TypedData C-API conversion

[Eric Wong]

This provides some extra type safety if combined with other C
extensions, as well as allowing us to account for memory usage of
the HTTP parser in ObjectSpace.

Note: this means we are finally dropping Ruby 1.8 support as
TypedData requires Ruby 1.9 and later.  Future changes will
require Ruby 1.9.2 and later (which is already EOL), but still
in use at some places.  This compiles with warnings on Ruby 1.9.2,
but is warning-free on modern Ruby versions.

This also currently leaks memory under 1.9.2-p330 x86_64-linux if
test_memory_leak is enabled in test/unit/test_http_parser.rb
Since 1.9.2 is EOL and 1.9.3+ all work fine (including trunk),
I'm not going to spend more time with this problem.

Also, keep in mind this type of memory leak wouldn't affect unicorn
as we only ever allocate a single parser.  This leak would only
affect other (concurrent) HTTP servers using this parser, and only
under Ruby 1.9.2.
---
 So with this, I'm strongly considering making unicorn 5 depend
 on Ruby 1.9.3+, and not just Ruby 1.9.2+.

 ext/unicorn_http/unicorn_http.rl | 35 +++++++++++++++++++++++------------
 test/unit/test_http_parser.rb    | 10 ++++++++++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 3294357..16741e0 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -417,11 +417,31 @@ post_exec: /* "_out:" also goes here */
   assert(hp->offset <= len && "offset longer than length");
 }
 
+static void hp_mark(void *ptr)
+{
+  struct http_parser *hp = ptr;
+
+  rb_gc_mark(hp->buf);
+  rb_gc_mark(hp->env);
+  rb_gc_mark(hp->cont);
+}
+
+static size_t hp_memsize(const void *ptr)
+{
+  return sizeof(struct http_parser);
+}
+
+static const rb_data_type_t hp_type = {
+  "unicorn_http",
+  { hp_mark, (void(*))(void *)-1, hp_memsize, /* reserved */ },
+  /* parent, data, [ flags ] */
+};
+
 static struct http_parser *data_get(VALUE self)
 {
   struct http_parser *hp;
 
-  Data_Get_Struct(self, struct http_parser, hp);
+  TypedData_Get_Struct(self, struct http_parser, &hp_type, hp);
   assert(hp && "failed to extract http_parser struct");
   return hp;
 }
@@ -527,21 +547,12 @@ static void finalize_header(struct http_parser *hp)
     rb_hash_aset(hp->env, g_query_string, rb_str_new(NULL, 0));
 }
 
-static void hp_mark(void *ptr)
-{
-  struct http_parser *hp = ptr;
-
-  rb_gc_mark(hp->buf);
-  rb_gc_mark(hp->env);
-  rb_gc_mark(hp->cont);
-}
-
 static VALUE HttpParser_alloc(VALUE klass)
 {
   struct http_parser *hp;
-  return Data_Make_Struct(klass, struct http_parser, hp_mark, -1, hp);
-}
 
+  return TypedData_Make_Struct(klass, struct http_parser, &hp_type, hp);
+}
 
 /**
  * call-seq:
diff --git a/test/unit/test_http_parser.rb b/test/unit/test_http_parser.rb
index 2251dcf..2785df7 100644
--- a/test/unit/test_http_parser.rb
+++ b/test/unit/test_http_parser.rb
@@ -851,4 +851,14 @@ class HttpParserTest < Test::Unit::TestCase
          File.readable?(LINUX_PROC_PID_STATUS) &&
          !defined?(RUBY_ENGINE)
 
+  def test_memsize
+    require 'objspace'
+    if ObjectSpace.respond_to?(:memsize_of)
+      n = ObjectSpace.memsize_of(Unicorn::HttpParser.new)
+      assert_kind_of Integer, n
+      assert_operator n, :<=, 56 # need to update this when 128-bit machines
+    end
+  rescue LoadError
+    # not all Ruby implementations have objspace
+  end
 end
-- 
EW

Re: [RFC] http: TypedData C-API conversion

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Note: this means we are finally dropping Ruby 1.8 support as
> TypedData requires Ruby 1.9 and later.

Unfortunately, TypedData is not an official API, yet,
so I'm hesitant to depend on it until it is official:
  http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/67089

But there ought to be tiny gains made from dropping 1.8 support,
so I'll probably go ahead and do it...

[PATCH] tmpio: drop the "size" method

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> But there ought to be tiny gains made from dropping 1.8 support,
> so I'll probably go ahead and do it...

Tiny stuff like this:
----------------------------8<-------------------------
From: Eric Wong <e@80x24.org>
Subject: [PATCH] tmpio: drop the "size" method

It is redundant given the existence of File#size in Ruby 1.9+

This saves 1440 bytes of bytecode on x86-64 under 2.2.0,
and at least another 120 bytes for the method entry,
hash table entry, and method definition overhead.
---
 lib/unicorn/tmpio.rb | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/lib/unicorn/tmpio.rb b/lib/unicorn/tmpio.rb
index 2da05a2..c97979a 100644
--- a/lib/unicorn/tmpio.rb
+++ b/lib/unicorn/tmpio.rb
@@ -21,9 +21,4 @@ class Unicorn::TmpIO < File
     fp.sync = true
     fp
   end
-
-  # for easier env["rack.input"] compatibility with Rack <= 1.1
-  def size
-    stat.size
-  end unless File.method_defined?(:size)
 end
-- 
EW

Re: [RFC] http: TypedData C-API conversion

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Unfortunately, TypedData is not an official API, yet,
> so I'm hesitant to depend on it until it is official:
>   http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/67089

Ruby 2.3 will be released in a few hours or so, and this API
hasn't changed since 1.9.3; so I just pushed out commit
5b5cf896871efdb110ae831fd7fc34fb78ec2243
to git://bogomips.org/unicorn.git
<http://bogomips.org/unicorn.git/patch/?id=5b5cf896871efd>
<http://bogomips.org/unicorn-public/?x=t&q=TypedData>

Then there's the optional frozen_string_literal stuff in 2.3...