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 >>>>>> >>>> >>
