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