unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / Atom feed
* [RFC] http: TypedData C-API conversion
@ 2014-11-16  8:32 Eric Wong
  2014-12-28  7:06 ` Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2014-11-16  8:32 UTC (permalink / raw)
  To: unicorn-public

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC] http: TypedData C-API conversion
  2014-11-16  8:32 [RFC] http: TypedData C-API conversion Eric Wong
@ 2014-12-28  7:06 ` Eric Wong
  2014-12-28  7:17   ` [PATCH] tmpio: drop the "size" method Eric Wong
  2015-12-24 18:57   ` [RFC] http: TypedData C-API conversion Eric Wong
  0 siblings, 2 replies; 4+ messages in thread
From: Eric Wong @ 2014-12-28  7:06 UTC (permalink / raw)
  To: unicorn-public

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...

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] tmpio: drop the "size" method
  2014-12-28  7:06 ` Eric Wong
@ 2014-12-28  7:17   ` Eric Wong
  2015-12-24 18:57   ` [RFC] http: TypedData C-API conversion Eric Wong
  1 sibling, 0 replies; 4+ messages in thread
From: Eric Wong @ 2014-12-28  7:17 UTC (permalink / raw)
  To: unicorn-public

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC] http: TypedData C-API conversion
  2014-12-28  7:06 ` Eric Wong
  2014-12-28  7:17   ` [PATCH] tmpio: drop the "size" method Eric Wong
@ 2015-12-24 18:57   ` Eric Wong
  1 sibling, 0 replies; 4+ messages in thread
From: Eric Wong @ 2015-12-24 18:57 UTC (permalink / raw)
  To: unicorn-public

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...

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-16  8:32 [RFC] http: TypedData C-API conversion Eric Wong
2014-12-28  7:06 ` Eric Wong
2014-12-28  7:17   ` [PATCH] tmpio: drop the "size" method Eric Wong
2015-12-24 18:57   ` [RFC] http: TypedData C-API conversion Eric Wong

unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git