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 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] ReportedBy: [EMAIL PROTECTED] -QAContact: [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 + + + _______________________________________________ mono-bugs maillist - [email protected] http://lists.ximian.com/mailman/listinfo/mono-bugs
