RFR S: 7127792 Add the ability to change an existing PeriodicTask's execution interval

[David Holmes]

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
>