People, I need at least one more reviewer, thanks!
/R On Oct 9, 2012, at 3:00 PM, Rickard Bäckman wrote: > David, > > thanks for your review! > > /R > > On Oct 9, 2012, at 2:01 PM, David Holmes wrote: > >> On 9/10/2012 9:42 PM, Rickard Bäckman wrote: >>> David, >>> see inline. >>> >>> On Oct 9, 2012, at 1:18 PM, David Holmes wrote: >>> >>>> On 9/10/2012 7:36 PM, Rickard Bäckman wrote: >>>>> David, >>>>> >>>>> thanks for your reply! >>>>> I've changed the code according to the suggestions, I've also changed the >>>>> types in PeriodicTask from being a size_t to being a >>>>> jint (see updated webrev for details). >>>> >>>> But now the type changes have been pushed out to the task creators. Most >>>> task creations pass an int already but BiasedLocking uses a size_t. Just >>>> shows how messed up the typing was to begin with. >>> >>> Agreed. Two ways of solving it, 1) change the callers to use an int. 2) Do >>> the cast in the constructor (should be safe since we check the possible >>> interval). >> >> int -> size_t shouldn't cause a warning so callers currently passing int are >> ok. So keeping it as size_t in constructor arg and casting to int before >> storing internally seems okay. >> >>>> >>>> Minor nit: should be an int rather than jint as these are not Java types. >>> >>> Will fix. >>> >>>> >>>>> To prevent the waiting for very long time (which could overflow, etc) >>>>> when we don't have any active task, I added an extra if >>>>> so that if we are waiting while no tasks are available, we reset the >>>>> time_before_wait and consider time_slept to be zero after sleeping. >>>>> That means the first task added will always sleep for the period >>>>> requested. >>>> >>>> Those semantics seem reasonable. >>>> >>>> The only thing that concerns me here is the affect of calling >>>> real_time_tick(0). I can't quite tell what the profiling code does. >>> >>> I could avoid calling that if time_slept = 0 >> >> Ok. You could skip real_time_tick altogether on zero. >> >> I'm generally okay with the approach being taken here, but the changes are >> disruptive enough that I can't see for sure that existing tasks will be >> unaffected. I guess time will tell. And let's see what others might spot. >> >> Thanks, >> David (signing off for the night) >> >>> Thanks >>> /R >>> >>>> >>>> David >>>> ----- >>>> >>>>> Updated webrev: >>>>> http://cr.openjdk.java.net/~rbackman/7127792.1/ >>>>> >>>>> Thanks >>>>> /R >>>>> >>>>> On Oct 9, 2012, at 9:46 AM, David Holmes wrote: >>>>> >>>>>> Sorry Rickard, missed the original RFR :) >>>>>> >>>>>> So to be clear here the synopsis concerns changing the period, while the >>>>>> mechanism implemented is for a more general dynamic disenroll / enroll. >>>>>> So changing a period is effected by removing a task and then adding it >>>>>> with the new period. >>>>>> >>>>>> And for anyone not reading the fine-print when you dynamically enroll a >>>>>> task its first firing is somewhat arbitrary - somewhere between the time >>>>>> of enrollment and that time plus its period. >>>>>> >>>>>> src/share/vm/runtime/thread.hpp >>>>>> >>>>>> Can you add a comment: >>>>>> >>>>>> static void stop(); >>>>>> + // Only allow start once the VM is sufficiently initialized >>>>>> + // Otherwise the first task to enroll will trigger the start >>>>>> + static void make_startable(); >>>>>> >>>>>> --- >>>>>> >>>>>> src/share/vm/runtime/thread.cpp >>>>>> >>>>>> We have a bit of type mixing here: >>>>>> >>>>>> - size_t time_to_wait >>>>>> - jlong time_slept >>>>>> - int remaining = time_to_wait >>>>>> >>>>>> I don't understand why Task is using size_t for time intervals. If you >>>>>> make "remaining" a size_t then it will cause issues when you pass it to >>>>>> wait. But I would expect the initialization of "remaining" to cause an >>>>>> unsigned-to-signed conversion warning, so perhaps an explicit cast to >>>>>> silence that. >>>>>> >>>>>> 1203 bool status = >>>>>> PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag, remaining); >>>>>> 1204 if (status || _should_terminate) { >>>>>> 1205 break; >>>>>> 1206 } >>>>>> >>>>>> Can you rename status to timedout to make the logic more obvious. Also >>>>>> note that you will potentially return with time_slept still at zero, >>>>>> even though you may have slept for an arbitrary amount of time. That >>>>>> seems wrong as zero will then be passed to "tick". >>>>>> >>>>>> 1208 // spurious wakeup of some kind >>>>>> >>>>>> This comment is no longer accurate as you may have been woken up due to >>>>>> a change in the task list, I suggest: >>>>>> >>>>>> // Change to task list or spurious wakeup of some kind >>>>>> >>>>>> 1213 remaining = PeriodicTask::time_to_wait(); >>>>>> 1214 if (remaining == 0) { >>>>>> 1215 continue; >>>>>> 1216 } >>>>>> >>>>>> Can you insert a comment before continue: >>>>>> >>>>>> // Last task was just disenrolled so loop around and wait until >>>>>> // another task gets enrolled >>>>>> continue; >>>>>> >>>>>> >>>>>> 1218 remaining -= time_slept; >>>>>> >>>>>> Again type mixing: subtracting a long from an int. Again a potential >>>>>> warning to get rid of. >>>>>> >>>>>> Also in a long running VM perhaps there have been no periodic tasks for >>>>>> many days and then one turns up. The subtraction could wrap and cause >>>>>> remaining to remain positive. I know you've now documented the >>>>>> uncertainty in the first fire time but this seems somewhat too random. >>>>>> Let's see what others think. :) >>>>>> >>>>>> >>>>>> 1329 PeriodicTask_lock->notify_all(); >>>>>> >>>>>> There is only one thread that waits so notify() will suffice. >>>>>> >>>>>> Thanks, >>>>>> David >>>>>> >>>>>> On 9/10/2012 4:34 PM, Rickard Bäckman wrote: >>>>>>> Trying again, >>>>>>> >>>>>>> can I have a couple of reviews, please? >>>>>>> >>>>>>> /R >>>>>>> >>>>>>> On Oct 4, 2012, at 3:01 PM, Rickard Bäckman wrote: >>>>>>> >>>>>>>> Hi all, >>>>>>>> >>>>>>>> can I please have a couple of reviews on the following change: >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~rbackman/7127792/ >>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127792 >>>>>>>> >>>>>>>> In short the purpose is to enable tasks to change the interval they >>>>>>>> are executed in. We >>>>>>>> would also like to be able to add (and remove) tasks after the >>>>>>>> WatcherThread has started. >>>>>>>> >>>>>>>> Thanks >>>>>>>> /R >>>>>>> >>>>> >>> >
