[RFC/PATCH] event_machine: lazily set async env keys

[RFC/PATCH] event_machine: lazily set async env keys

[Eric Wong]

This has the small likelyhood of breaking apps that
check via:
   env.include?("async.callback")

Or any of member?/has_key?/key?, as they're all the same method.

>From all the async examples I see, I just see apps just
use env["async.callback"].

Worth it?

>From 40ef059dfc99c7daad1b1829713727b86c8fc9a0 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org>
Date: Thu, 15 Jul 2010 05:48:29 +0000
Subject: [PATCH] event_machine: lazily set async env keys

This is a micro-optimization when running a 'hello world' benchmark and
also reduces the strength of a memory leak under 1.9.2-rc2:

  http://redmine.ruby-lang.org/issues/show/3466
---
 lib/rainbows/ev_core.rb       |    1 -
 lib/rainbows/event_machine.rb |   17 ++++++++++++-----
 lib/rainbows/rev/client.rb    |    1 +
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/lib/rainbows/ev_core.rb b/lib/rainbows/ev_core.rb
index 5ca693b..357042b 100644
--- a/lib/rainbows/ev_core.rb
+++ b/lib/rainbows/ev_core.rb
@@ -16,7 +16,6 @@ module Rainbows
 
     def post_init
       @remote_addr = Rainbows.addr(@_io)
-      @env = {}
       @hp = HttpParser.new
       @state = :headers # [ :body [ :trailers ] ] :app_call :close
       @buf = ""
diff --git a/lib/rainbows/event_machine.rb b/lib/rainbows/event_machine.rb
index 173340e..14da118 100644
--- a/lib/rainbows/event_machine.rb
+++ b/lib/rainbows/event_machine.rb
@@ -54,6 +54,17 @@ module Rainbows
       G = Rainbows::G
 
       def initialize(io)
+        @env = Hash.new do |hash,key|
+          case key
+          when ASYNC_CALLBACK
+            hash[ASYNC_CALLBACK] = method(:em_write_response)
+          when ASYNC_CLOSE
+            # we're not sure if anybody uses this, but Thin sets it, too
+            hash[ASYNC_CLOSE] = EM::DefaultDeferrable.new
+          else
+            nil
+          end
+        end
         @_io = io
         @body = nil
       end
@@ -71,10 +82,6 @@ module Rainbows
         begin
           @env[RACK_INPUT] = @input
           @env[REMOTE_ADDR] = @remote_addr
-          @env[ASYNC_CALLBACK] = method(:em_write_response)
-
-          # we're not sure if anybody uses this, but Thin sets it, too
-          @env[ASYNC_CLOSE] = EM::DefaultDeferrable.new
 
           response = catch(:async) { APP.call(@env.update(RACK_DEFAULTS)) }
 
@@ -143,7 +150,7 @@ module Rainbows
       end
 
       def unbind
-        async_close = @env[ASYNC_CLOSE] and async_close.succeed
+        @env.include?(ASYNC_CLOSE) and @env[ASYNC_CLOSE].succeed
         @body.respond_to?(:fail) and @body.fail
         @_io.close
       end
diff --git a/lib/rainbows/rev/client.rb b/lib/rainbows/rev/client.rb
index d08992b..0df7415 100644
--- a/lib/rainbows/rev/client.rb
+++ b/lib/rainbows/rev/client.rb
@@ -15,6 +15,7 @@ module Rainbows
         CONN[self] = false
         super(io)
         post_init
+        @env = {}
         @deferred_bodies = [] # for (fast) regular files only
       end
 
-- 
Eric Wong
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [RFC/PATCH] event_machine: lazily set async env keys

[James Tucker]

Sorry for the top post, trying to be quick...

Just regarding the async close, it's used primarily when the client disconnects so that you can stop asynchronous operations. It's quite important from a resource standpoint.

Why don't you just use a lambda instead of a method?

lambda { |*args| em_write_response(*args) }

That'd avoid the leak, and provide nearly the same performance profile. In fact on 1.9, lambdas are pretty damn quick.

On 15 Jul 2010, at 07:03, Eric Wong wrote:

> This has the small likelyhood of breaking apps that
> check via:
>  env.include?("async.callback")
> 
> Or any of member?/has_key?/key?, as they're all the same method.
> 
>> From all the async examples I see, I just see apps just
> use env["async.callback"].
> 
> Worth it?
> 
>> From 40ef059dfc99c7daad1b1829713727b86c8fc9a0 Mon Sep 17 00:00:00 2001
> From: Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org>
> Date: Thu, 15 Jul 2010 05:48:29 +0000
> Subject: [PATCH] event_machine: lazily set async env keys
> 
> This is a micro-optimization when running a 'hello world' benchmark and
> also reduces the strength of a memory leak under 1.9.2-rc2:
> 
> http://redmine.ruby-lang.org/issues/show/3466
> ---
> lib/rainbows/ev_core.rb       |    1 -
> lib/rainbows/event_machine.rb |   17 ++++++++++++-----
> lib/rainbows/rev/client.rb    |    1 +
> 3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/rainbows/ev_core.rb b/lib/rainbows/ev_core.rb
> index 5ca693b..357042b 100644
> --- a/lib/rainbows/ev_core.rb
> +++ b/lib/rainbows/ev_core.rb
> @@ -16,7 +16,6 @@ module Rainbows
> 
>    def post_init
>      @remote_addr = Rainbows.addr(@_io)
> -      @env = {}
>      @hp = HttpParser.new
>      @state = :headers # [ :body [ :trailers ] ] :app_call :close
>      @buf = ""
> diff --git a/lib/rainbows/event_machine.rb b/lib/rainbows/event_machine.rb
> index 173340e..14da118 100644
> --- a/lib/rainbows/event_machine.rb
> +++ b/lib/rainbows/event_machine.rb
> @@ -54,6 +54,17 @@ module Rainbows
>      G = Rainbows::G
> 
>      def initialize(io)
> +        @env = Hash.new do |hash,key|
> +          case key
> +          when ASYNC_CALLBACK
> +            hash[ASYNC_CALLBACK] = method(:em_write_response)
> +          when ASYNC_CLOSE
> +            # we're not sure if anybody uses this, but Thin sets it, too
> +            hash[ASYNC_CLOSE] = EM::DefaultDeferrable.new
> +          else
> +            nil
> +          end
> +        end
>        @_io = io
>        @body = nil
>      end
> @@ -71,10 +82,6 @@ module Rainbows
>        begin
>          @env[RACK_INPUT] = @input
>          @env[REMOTE_ADDR] = @remote_addr
> -          @env[ASYNC_CALLBACK] = method(:em_write_response)
> -
> -          # we're not sure if anybody uses this, but Thin sets it, too
> -          @env[ASYNC_CLOSE] = EM::DefaultDeferrable.new
> 
>          response = catch(:async) { APP.call(@env.update(RACK_DEFAULTS)) }
> 
> @@ -143,7 +150,7 @@ module Rainbows
>      end
> 
>      def unbind
> -        async_close = @env[ASYNC_CLOSE] and async_close.succeed
> +        @env.include?(ASYNC_CLOSE) and @env[ASYNC_CLOSE].succeed
>        @body.respond_to?(:fail) and @body.fail
>        @_io.close
>      end
> diff --git a/lib/rainbows/rev/client.rb b/lib/rainbows/rev/client.rb
> index d08992b..0df7415 100644
> --- a/lib/rainbows/rev/client.rb
> +++ b/lib/rainbows/rev/client.rb
> @@ -15,6 +15,7 @@ module Rainbows
>        CONN[self] = false
>        super(io)
>        post_init
> +        @env = {}
>        @deferred_bodies = [] # for (fast) regular files only
>      end
> 
> -- 
> Eric Wong
> _______________________________________________
> Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
> http://rubyforge.org/mailman/listinfo/rainbows-talk
> Do not quote signatures (like this one) or top post when replying

_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [RFC/PATCH] event_machine: lazily set async env keys

[Eric Wong]

James Tucker <jftucker-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 15 Jul 2010, at 07:03, Eric Wong wrote:
> 
> > This has the small likelyhood of breaking apps that
> > check via:
> >  env.include?("async.callback")
> > 
> > Or any of member?/has_key?/key?, as they're all the same method.

> Just regarding the async close, it's used primarily when the client
> disconnects so that you can stop asynchronous operations. It's quite
> important from a resource standpoint.

OK, but the EM::DefaultDeferrable object is never touched (or
even instantiated), then there's no need to close it.

> Why don't you just use a lambda instead of a method?
> 
> lambda { |*args| em_write_response(*args) }
>
> That'd avoid the leak, and provide nearly the same performance
> profile. In fact on 1.9, lambdas are pretty damn quick.

Durr... I didn't think of that :x

Since 1.9.2 is in RC with few users and this bug has a fix (at
http://redmine.ruby-lang.org/issues/show/3466), we might as well leave
the code as-is for now and skip my proposed patch.  Any objections?

I try to avoid working around other projects' bugs (and just helping fix
them).  This is especially the case for things like 1.9.2 which is still
in the very early stages of adoption.

-- 
Eric Wong
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [RFC/PATCH] event_machine: lazily set async env keys

[James Tucker]

On 15 Jul 2010, at 22:05, Eric Wong wrote:

> James Tucker <jftucker-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 15 Jul 2010, at 07:03, Eric Wong wrote:
>> 
>>> This has the small likelyhood of breaking apps that
>>> check via:
>>> env.include?("async.callback")
>>> 
>>> Or any of member?/has_key?/key?, as they're all the same method.
> 
>> Just regarding the async close, it's used primarily when the client
>> disconnects so that you can stop asynchronous operations. It's quite
>> important from a resource standpoint.
> 
> OK, but the EM::DefaultDeferrable object is never touched (or
> even instantiated), then there's no need to close it.
> 
>> Why don't you just use a lambda instead of a method?
>> 
>> lambda { |*args| em_write_response(*args) }
>> 
>> That'd avoid the leak, and provide nearly the same performance
>> profile. In fact on 1.9, lambdas are pretty damn quick.
> 
> Durr... I didn't think of that :x
> 
> Since 1.9.2 is in RC with few users and this bug has a fix (at
> http://redmine.ruby-lang.org/issues/show/3466), we might as well leave
> the code as-is for now and skip my proposed patch.  Any objections?
> 
> I try to avoid working around other projects' bugs (and just helping fix
> them).  This is especially the case for things like 1.9.2 which is still
> in the very early stages of adoption.

I have a similar patchset i'm not putting in for Thin. Also, the EM adjustment I made merely reduces the impact, and also increases performance:

http://github.com/eventmachine/eventmachine/commit/60b6472da952c7cdc59f093f93678dbd1739ab0a

_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying