[Mono-bugs] [Bug 65734][Nor] Changed - System.Threading.Timer 20x slower than MSFT's

Tue Aug 1 12:32:07 EDT 2006

Please do not reply to this email- if you want to comment on the bug, go to the
URL shown below and enter your comments there.

Changed by raf at ophion.org.

http://bugzilla.ximian.com/show_bug.cgi?id=65734

--- shadow/65734	2006-07-31 06:36:47.000000000 -0400
+++ shadow/65734.tmp.11188	2006-08-01 12:32:07.000000000 -0400
@@ -7,16 +7,14 @@
 Resolution: 
 Severity: Unknown
 Priority: Normal
 Component: CORLIB
 AssignedTo: mono-bugs at ximian.com                            
 ReportedBy: bmaurer at users.sf.net               
-QAContact: mono-bugs at ximian.com
 TargetMilestone: ---
 URL: 
-Cc: 
 Summary: System.Threading.Timer 20x slower than MSFT's
 
 This test takes 20x longer on Mono than on MSFT's runtime:
 
 using System;
 using System.Threading;
@@ -88,6 +86,98 @@
 that timeout. Pulse the object if a new timer is added". This will
 wake up the CPU far too often, and (for example) waste battery power.
 It is a blocker for this patch.
 
 The sleep is dubious, but it's not polling the cpu like i thought it
 was. It's 3 am here ;-)
+
+------- Additional Comments From raf at ophion.org  2006-08-01 12:32 -------
+Awesome feedback guys... my responses are prefixed by ">>"
+
+* If the callback throws, the scheduler thread is killed.
+>> perfect catch, I"ll add a catch all "catch Exception" in order to
+correct this.
+
+Style wise, this patch needs some work:
+
++			static object sync_obj = new object();
++			static object sync_obj2 = new object();
+
+How are those used?
+
+>> sync_obj is used to prevent race conditions on the "GetInstance"
+call of the singleton TimerScheduler class. sync_obj2 is used to make
+signaling the scheduler class sane so only one signal is sent at a
+time and a decision can be made on the signal so signals are "batched"
+or discarded .With that said, I'm not sure why I have 2 separate sync
+objects, I should be able to combine them without issues.
+
+Why is TimerScheduler a class? it seems like it could be static
+methods in Timer? Also, why a TimerJob class. This is a 1<->1 relation
+with a Timer.
+
+>> Timer scheduler is a singleton class because it has a 1-1 mapping
+with a timer-scheduler thread which is the thread in charge of firing
+all the timer events, so it is a 1 to n mapping between
+timer-scheduler threads and timers. Now with regards to why it needs
+to be a class of its own instead of  static methods in Timer i guess
+it's merely a matter of choice... I believe I chose that route just to
+keep all the scheduler logic in one place, which made it easier for me
+to merge it with the Timer class. Of course, if you guys want, I can
+made the conversion. I believe TimerJob follows under the same logic,
+I just wanted to keep the scheduler specific meta data in one place.
+
+Variables and methods are named with mixed conventions. Make things
+consistent, at the very least. In general, no underscore_names for
+methods. Also, enum values shouldn't be C_CONSTANT_NAMED.
+
+>> I'll correct all that
+
++			readonly int TIME_SLICE = 10 ; // 10 msec
+
+Should be const
+
+>> same here
+
++					log("could not properly signal timer-scheduler, waiting...");
++					Thread.Sleep(5); 
+
+THis is *really* *really* dubious. You need to do something like
+"calcuate the next time you will wake up, and wait on an object with
+that timeout. Pulse the object if a new timer is added". This will
+wake up the CPU far too often, and (for example) waste battery power.
+It is a blocker for this patch.
+
+>> Yeah the logic is slightly counter intuitive but it actually
+performs really well under most conditions. The timer logic is
+actually very very loosely base on Quartz for java (I guess the main
+loop logic is the same). From my testing you start seeing issues once
+the number of number of timers is very large and the period of the
+events is small which puts pressure on the scheduler to run more often
+and doesn't give it a chance to sleep. Also, if the "period" of the
+Timer is shorter than 10 msec (the TIMER_SLICE base), we start having
+issues as well, and that is not as simple of a problem to solve as it
+may seem. I did some performance observations that you might find
+interesting here:
+
+http://ophion.org/index.php?gadget=Blog&action=SingleView&id=9
+
+
++			void log(string str) {
++				if (Environment.GetEnvironmentVariable("MONO_TIMER_DEBUG") != null)
+
+Should probably be cached.
+
+>> agreed
+
+>> I'm going to post a new patch tonight reflecting those changes, I
+just need to test removing the "sync_obj" to make sure it doesn't
+create any problems. Also Miguel had suggested to change the patch to
+not use thread pool threads, so I might make that change as well but
+I'm not sure how that is going to affect performance. 
+
+Once again thanks for the very insightful feedback!
+
+- raf
+
+
+
