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

[Rickard Bäckman]

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