From: Mark Somerville <mark@...>
Date: 2010-10-13T23:18:25+09:00
Subject: [ruby-core:32773] Re: [Ruby 1.9-Feature#3436] Spawn the timer thread lazily


--2B/JsCI69OhZNC5r
Content-Type: multipart/mixed; boundary="AhhlLboLdkugWU4S"
Content-Disposition: inline


--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Oct 10, 2010 at 02:21:41AM +0900, Mark Somerville wrote:
> On Sun, Oct 10, 2010 at 01:27:53AM +0900, Mark Somerville wrote:
> > I've attached a slightly improved patch which fixes a rare case when the
> > timer thread may not be stopped correctly after the last thread has
> > exited.

How can I move this patch forward? Are there any objections to it being
merged? If so, I'd love to try to fix them.

Without the patch, PowerTOP shows that Ruby trunk (-e "sleep 60") is a
bad offender when it comes to CPU wakeups:


        Wakeups-from-idle per second : 127.2    interval: 15.0s
        no ACPI power usage estimate available
        Top causes for wakeups:
          43.1% ( 99.5)              ruby : hrtimer_start_range_ns
        (hrtimer_wakeup)
          22.6% ( 52.1)     <kernel core> : hrtimer_start_range_ns
        (tick_sched_timer)
           5.5% ( 12.8)       <interrupt> : ahci
           5.2% ( 12.0)     <kernel core> : __mod_timer (rh_timer_func)
           4.7% ( 10.7)     <kernel core> : hrtimer_start
	(tick_sched_timer)
           3.8% (  8.8)   chromium-browse : hrtimer_start_range_ns
        (hrtimer_wakeup)
        ...


With the patch applied, Ruby doesn't make it onto the (untruncated) list
at all:


        Wakeups-from-idle per second : 68.9     interval: 15.0s
        no ACPI power usage estimate available
        Top causes for wakeups:
          45.2% ( 50.9)     <kernel core> : hrtimer_start_range_ns
        (tick_sched_timer)
          10.8% ( 12.2)     <kernel core> : __mod_timer (rh_timer_func)
          10.7% ( 12.0)       <interrupt> : ahci
          10.1% ( 11.3)     <kernel core> : hrtimer_start
	(tick_sched_timer)
           3.4% (  3.8)   chromium-browse : __mod_timer
	(process_timeout)
           2.5% (  2.9)   chromium-browse : hrtimer_start_range_ns
        (hrtimer_wakeup)
        ...


I've attached an update that makes the patch apply cleanly again after
some changes were made to trunk.

--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="bug_3436_fix_with_improvements.patch"
Content-Transfer-Encoding: quoted-printable

Index: thread_win32.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- thread_win32.c	(revision 29431)
+++ thread_win32.c	(working copy)
@@ -590,4 +590,10 @@
     }
 }
=20
+static int
+native_timer_thread_is_running(void)
+{
+    return timer_thread_id !=3D 0;
+}
+
 #endif /* THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION */
Index: thread_pthread.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- thread_pthread.c	(revision 29431)
+++ thread_pthread.c	(working copy)
@@ -840,6 +840,12 @@
     timer_thread_id =3D 0;
 }
=20
+static int
+native_timer_thread_is_running(void)
+{
+    return timer_thread_id !=3D 0;
+}
+
 #ifdef HAVE_SIGALTSTACK
 int
 ruby_stack_overflowed_p(const rb_thread_t *th, const void *addr)
Index: vm_core.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- vm_core.h	(revision 29431)
+++ vm_core.h	(working copy)
@@ -635,6 +635,7 @@
 VALUE rb_iseq_eval_main(VALUE iseqval);
 void rb_enable_interrupt(void);
 void rb_disable_interrupt(void);
+int rb_thread_timer_thread_is_running(void);
 #if defined __GNUC__ && __GNUC__ >=3D 4
 #pragma GCC visibility pop
 #endif
Index: thread.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- thread.c	(revision 29431)
+++ thread.c	(working copy)
@@ -562,6 +564,7 @@
 	th->status =3D THREAD_KILLED;
 	rb_raise(rb_eThreadError, "can't create Thread (%d)", err);
     }
+    rb_thread_start_timer_thread();
     return thval;
 }
=20
@@ -577,6 +580,7 @@
 	rb_raise(rb_eThreadError, "uninitialized thread - check `%s#initialize'",
 		 rb_class2name(klass));
     }
+    rb_thread_start_timer_thread();
     return thread;
 }
=20
@@ -711,6 +715,16 @@
     thread_debug("thread_join: success (thid: %p)\n",
 		 (void *)target_th->thread_id);
=20
+    if (rb_thread_alone()) {
+        rb_disable_interrupt();
+        if (rb_signal_buff_size() > 0) {
+            /* process any buffered signals before stopping the timer thre=
ad */
+            rb_threadptr_check_signal(GET_THREAD());
+	 }
+        rb_thread_stop_timer_thread();
+        rb_enable_interrupt();
+    }
+
     if (target_th->errinfo !=3D Qnil) {
 	VALUE err =3D target_th->errinfo;
=20
@@ -2723,9 +2737,17 @@
 rb_thread_start_timer_thread(void)
 {
     system_working =3D 1;
-    rb_thread_create_timer_thread();
+    if (!rb_thread_alone()) {
+        rb_thread_create_timer_thread();
+    }
 }
=20
+int
+rb_thread_timer_thread_is_running(void)
+{
+    return native_timer_thread_is_running();
+}
+
 static int
 clear_coverage_i(st_data_t key, st_data_t val, st_data_t dummy)
 {
@@ -4388,7 +4410,7 @@
 	}
     }
=20
-    rb_thread_create_timer_thread();
+    rb_thread_start_timer_thread();
=20
     (void)native_mutex_trylock;
 }
Index: process.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- process.c	(revision 29431)
+++ process.c	(working copy)
@@ -998,7 +998,7 @@
 #define before_exec() \
     (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_time=
r_thread(), 1)))
 #define after_exec() \
-  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_=
child =3D 0, rb_disable_interrupt())
+  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_=
child =3D 0)
 #define before_fork() before_exec()
 #define after_fork() (GET_THREAD()->thrown_errinfo =3D 0, after_exec())
=20
Index: ext/-test-/thread/timer/timer.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- ext/-test-/thread/timer/timer.c	(revision 0)
+++ ext/-test-/thread/timer/timer.c	(revision 0)
@@ -0,0 +1,15 @@
+#include <ruby.h>
+#include "vm_core.h"
+
+VALUE
+thread_timer_thread_running_p(VALUE klass)
+{
+    return rb_thread_timer_thread_is_running() ? Qtrue : Qfalse;
+}
+
+void
+Init_timer(void)
+{
+    VALUE timer =3D rb_define_module_under(rb_cThread, "Timer");
+    rb_define_module_function(timer, "running?", thread_timer_thread_runni=
ng_p, 0);
+}
Index: ext/-test-/thread/timer/extconf.rb
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- ext/-test-/thread/timer/extconf.rb	(revision 0)
+++ ext/-test-/thread/timer/extconf.rb	(revision 0)
@@ -0,0 +1,3 @@
+require 'mkmf'
+$INCFLAGS << " -I$(top_srcdir)"
+create_makefile('-test-/thread/timer')
Index: test/ruby/test_signal.rb
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- test/ruby/test_signal.rb	(revision 29431)
+++ test/ruby/test_signal.rb	(working copy)
@@ -181,4 +181,20 @@
     w.close
     assert_equal(r.read, "foo")
   end
+
+  def test_signals_before_and_after_timer_thread
+    count =3D 0
+    Signal.trap(:INT) { count +=3D 1 }
+
+    Process.kill :INT, Process.pid
+    assert_equal 1, count
+
+    th =3D Thread.new { sleep 0.5 }
+    Process.kill :INT, Process.pid
+    assert_equal 2, count
+
+    th.join
+    Process.kill :INT, Process.pid
+    assert_equal 3, count
+  end
 end
Index: test/thread/test_timer_thread.rb
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- test/thread/test_timer_thread.rb	(revision 0)
+++ test/thread/test_timer_thread.rb	(revision 0)
@@ -0,0 +1,10 @@
+require 'test/unit'
+require '-test-/thread/timer'
+
+class TestTimerThread < Test::Unit::TestCase
+  def test_timer_is_created_and_destroyed
+    assert !Thread::Timer.running?
+    assert Thread.new {Thread::Timer.running?}.value
+    assert !Thread::Timer.running?
+  end
+end
Index: signal.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- signal.c	(revision 29431)
+++ signal.c	(working copy)
@@ -519,6 +519,10 @@
 #if !defined(BSD_SIGNAL) && !defined(POSIX_SIGNAL)
     ruby_signal(sig, sighandler);
 #endif
+
+    if (rb_thread_alone()) {
+        rb_threadptr_check_signal(GET_THREAD());
+    }
 }
=20
 int

--AhhlLboLdkugWU4S--

--2B/JsCI69OhZNC5r
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAky1v9cACgkQFC+QcFj+6g1GIQCdHFT5ELbiE7idDWvnB1E0mk2b
MHwAoIKyv/MucTa7XH7/ypppIB6YyWDI
=98j8
-----END PGP SIGNATURE-----

--2B/JsCI69OhZNC5r--