Rev talk - Nov 2009 - [PATCH 2/2] Make Rev::Loop#run_once less intrusive with 1.8 threads

Hi,

Under 1.8, the RevThreadSpawn concurrency model of Rainbows! was performing
horribly for processing large uploads.  I narrowed it down to the
blocking-for-10ms behavior of Rev under 1.8.  Since the Thread running the
Rev::Loop always gets scheduled, unconditionally blocking to do nothing for 10ms
becomes very bad.

Now, instead of blocking for 10ms unconditionally, we now detect if
we''re the
lone thread in the process and run in non-blocking mode if there are any other
threads.  This change does not affect 1.9.

With the current rainbows.git[1], the times for the following test goes
from ~73 seconds to ~6 seconds.  I''m not missing any zeros there :)

  # note(1), run "make -C t random_blob" first for more accurate
timings
  # note(2), I don''t use Rubygems for development, so setting up
RUBYLIB
  # by hand to include rack, unicorn (and rev+iobuffer, of course) is
  # required
  make -C t models=RevThreadSpawn t0100-rack-input-hammer.sh

The same test under 1.9 is around ~2.5 seconds.

[1] 921a961e91276522c134813052a7b7e06b00ef2b git://git.bogomips.org/rainbows.git

Both of these are pushed out to the usual location at git://yhbt.net/rev

Eric Wong (2):
      make Rev::Loop#run_nonblock signal-safe for 1.8
      Make Rev::Loop#run_once less intrusive with 1.8 threads

Even though this method is currently not used anywhere, it may
be useful in the next change(s) I make to make Rev + Threads
run better under 1.8
---
 ext/rev/rev_loop.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/ext/rev/rev_loop.c b/ext/rev/rev_loop.c
index 686e1a7..86dad37 100644
--- a/ext/rev/rev_loop.c
+++ b/ext/rev/rev_loop.c
@@ -260,7 +260,9 @@ static VALUE Rev_Loop_run_nonblock(VALUE self)
 
   assert(loop_data->ev_loop && !loop_data->events_received);
 
+  TRAP_BEG;
   RUN_LOOP(loop_data, EVLOOP_NONBLOCK);  
+  TRAP_END;
   Rev_Loop_dispatch_events(loop_data);
   
   nevents = INT2NUM(loop_data->events_received);
-- 
1.6.5.3.300.g9be30

Instead of blocking for 10ms when other threads are present,
run in non-blocking mode if we''re not the only thread in the
process.  This change does not negatively impact 1.9.
---
 ext/rev/extconf.rb |    4 ++++
 ext/rev/rev_loop.c |   37 ++++++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/ext/rev/extconf.rb b/ext/rev/extconf.rb
index 9b48dd9..2422cbc 100644
--- a/ext/rev/extconf.rb
+++ b/ext/rev/extconf.rb
@@ -8,6 +8,10 @@ if have_func(''rb_thread_blocking_region'')
   $defs << ''-DHAVE_RB_THREAD_BLOCKING_REGION''
 end
 
+if have_func(''rb_thread_alone'')
+  $defs << ''-DHAVE_RB_THREAD_ALONE''
+end
+
 if have_func(''rb_str_set_len'')
   $defs << ''-DHAVE_RB_STR_SET_LEN''
 end
diff --git a/ext/rev/rev_loop.c b/ext/rev/rev_loop.c
index 86dad37..bde0021 100644
--- a/ext/rev/rev_loop.c
+++ b/ext/rev/rev_loop.c
@@ -12,6 +12,14 @@
 
 #include "rev.h"
 
+#if defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#  define Rev_Loop_may_block_safely() (1)
+#elif defined(HAVE_RB_THREAD_ALONE)
+#  define Rev_Loop_may_block_safely() (rb_thread_alone())
+#else /* just in case Ruby changes: */
+#  define Rev_Loop_may_block_safely() (0)
+#endif
+
 static VALUE mRev = Qnil;
 static VALUE cRev_Loop = Qnil;
 
@@ -175,19 +183,26 @@ void Rev_Loop_process_event(VALUE watcher, int revents)
  */
 static VALUE Rev_Loop_run_once(VALUE self)
 {
-  struct Rev_Loop *loop_data;
   VALUE nevents;
-  
-  Data_Get_Struct(self, struct Rev_Loop, loop_data);
 
-  assert(loop_data->ev_loop && !loop_data->events_received);
-  
-  Rev_Loop_ev_loop_oneshot(loop_data);  
-  Rev_Loop_dispatch_events(loop_data);
-  
-  nevents = INT2NUM(loop_data->events_received);
-  loop_data->events_received = 0;
-  
+  if (Rev_Loop_may_block_safely()) {
+    struct Rev_Loop *loop_data;
+
+    Data_Get_Struct(self, struct Rev_Loop, loop_data);
+
+    assert(loop_data->ev_loop && !loop_data->events_received);
+
+    Rev_Loop_ev_loop_oneshot(loop_data);
+    Rev_Loop_dispatch_events(loop_data);
+
+    nevents = INT2NUM(loop_data->events_received);
+    loop_data->events_received = 0;
+
+  } else {
+    nevents = Rev_Loop_run_nonblock(self);
+    rb_thread_schedule();
+  }
+
   return nevents;
 }
 
-- 
1.6.5.3.300.g9be30

Nice, this is an optimization I considered doing myself but never got around
to.

On Thu, Nov 26, 2009 at 11:49 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Instead of blocking for 10ms when other threads are present,
> run in non-blocking mode if we''re not the only thread in the
> process.  This change does not negatively impact 1.9.
> ---
>  ext/rev/extconf.rb |    4 ++++
>  ext/rev/rev_loop.c |   37 ++++++++++++++++++++++++++-----------
>  2 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/ext/rev/extconf.rb b/ext/rev/extconf.rb
> index 9b48dd9..2422cbc 100644
> --- a/ext/rev/extconf.rb
> +++ b/ext/rev/extconf.rb
> @@ -8,6 +8,10 @@ if
have_func(''rb_thread_blocking_region'')
>   $defs << ''-DHAVE_RB_THREAD_BLOCKING_REGION''
>  end
>
> +if have_func(''rb_thread_alone'')
> +  $defs << ''-DHAVE_RB_THREAD_ALONE''
> +end
> +
>  if have_func(''rb_str_set_len'')
>   $defs << ''-DHAVE_RB_STR_SET_LEN''
>  end
> diff --git a/ext/rev/rev_loop.c b/ext/rev/rev_loop.c
> index 86dad37..bde0021 100644
> --- a/ext/rev/rev_loop.c
> +++ b/ext/rev/rev_loop.c
> @@ -12,6 +12,14 @@
>
>  #include "rev.h"
>
> +#if defined(HAVE_RB_THREAD_BLOCKING_REGION)
> +#  define Rev_Loop_may_block_safely() (1)
> +#elif defined(HAVE_RB_THREAD_ALONE)
> +#  define Rev_Loop_may_block_safely() (rb_thread_alone())
> +#else /* just in case Ruby changes: */
> +#  define Rev_Loop_may_block_safely() (0)
> +#endif
> +
>  static VALUE mRev = Qnil;
>  static VALUE cRev_Loop = Qnil;
>
> @@ -175,19 +183,26 @@ void Rev_Loop_process_event(VALUE watcher, int
> revents)
>  */
>  static VALUE Rev_Loop_run_once(VALUE self)
>  {
> -  struct Rev_Loop *loop_data;
>   VALUE nevents;
> -
> -  Data_Get_Struct(self, struct Rev_Loop, loop_data);
>
> -  assert(loop_data->ev_loop && !loop_data->events_received);
> -
> -  Rev_Loop_ev_loop_oneshot(loop_data);
> -  Rev_Loop_dispatch_events(loop_data);
> -
> -  nevents = INT2NUM(loop_data->events_received);
> -  loop_data->events_received = 0;
> -
> +  if (Rev_Loop_may_block_safely()) {
> +    struct Rev_Loop *loop_data;
> +
> +    Data_Get_Struct(self, struct Rev_Loop, loop_data);
> +
> +    assert(loop_data->ev_loop &&
!loop_data->events_received);
> +
> +    Rev_Loop_ev_loop_oneshot(loop_data);
> +    Rev_Loop_dispatch_events(loop_data);
> +
> +    nevents = INT2NUM(loop_data->events_received);
> +    loop_data->events_received = 0;
> +
> +  } else {
> +    nevents = Rev_Loop_run_nonblock(self);
> +    rb_thread_schedule();
> +  }
> +
>   return nevents;
>  }
>
> --
> 1.6.5.3.300.g9be30
>
> _______________________________________________
> Rev-talk mailing list
> Rev-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rev-talk
>


-- 
Tony Arcieri
Medioh/Nagravision
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rev-talk/attachments/20091128/f561de89/attachment-0001.html>

This is likewise awesome.

On Thu, Nov 26, 2009 at 11:49 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Hi,
>
> Under 1.8, the RevThreadSpawn concurrency model of Rainbows! was performing
> horribly for processing large uploads.  I narrowed it down to the
> blocking-for-10ms behavior of Rev under 1.8.  Since the Thread running the
> Rev::Loop always gets scheduled, unconditionally blocking to do nothing for
> 10ms
> becomes very bad.
>
> Now, instead of blocking for 10ms unconditionally, we now detect if
we''re
> the
> lone thread in the process and run in non-blocking mode if there are any
> other
> threads.  This change does not affect 1.9.
>
> With the current rainbows.git[1], the times for the following test goes
> from ~73 seconds to ~6 seconds.  I''m not missing any zeros there
:)
>
>  # note(1), run "make -C t random_blob" first for more accurate
timings
>  # note(2), I don''t use Rubygems for development, so setting up
RUBYLIB
>  # by hand to include rack, unicorn (and rev+iobuffer, of course) is
>  # required
>  make -C t models=RevThreadSpawn t0100-rack-input-hammer.sh
>
> The same test under 1.9 is around ~2.5 seconds.
>
> [1] 921a961e91276522c134813052a7b7e06b00ef2b git://
> git.bogomips.org/rainbows.git
>
> Both of these are pushed out to the usual location at git://yhbt.net/rev
>
> Eric Wong (2):
>      make Rev::Loop#run_nonblock signal-safe for 1.8
>      Make Rev::Loop#run_once less intrusive with 1.8 threads
> _______________________________________________
> Rev-talk mailing list
> Rev-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rev-talk
>


-- 
Tony Arcieri
Medioh/Nagravision
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rev-talk/attachments/20091128/970f8199/attachment.html>